[postgis-devel] ForceRHR doesn't?

[Michael Fuhr]

On Sat, Mar 11, 2006 at 02:29:15AM -0700, Michael Fuhr wrote:
> ForceRHR does a reverse instead of forcing a particular point order.
> Am I misunderstanding something or is that broken?

To clarify: ForceRHR() does an unconditional reverse instead of
reversing only when necessary.  Am I correct in understanding that
ForceRHR() should return points in counter-clockwise order?

I changed lwgeom_forceRHR() to call lwpoly_forceRHR() instead of
lwpoly_reverse() and it appears to work the way I expect:

SELECT AsText(ForceRHR('POLYGON((0 0,10 0,5 5,0 0))'));
           astext            
-----------------------------
 POLYGON((0 0,10 0,5 5,0 0))
(1 row)

SELECT AsText(ForceRHR('POLYGON((0 0,5 5,10 0,0 0))'));
           astext            
-----------------------------
 POLYGON((0 0,10 0,5 5,0 0))
(1 row)

However, the "fixed" code returns interior rings in clockwise order.
Is that correct?

SELECT AsText(ForceRHR('POLYGON((0 0,10 0,5 5,0 0),(1 1,9 1,4 4,1 1))'));
                    astext                     
-----------------------------------------------
 POLYGON((0 0,10 0,5 5,0 0),(1 1,4 4,9 1,1 1))
(1 row)

This is happening because lwpoly_forceRHR() uses opposite tests for
the exterior and interior rings:

    if ( ptarray_isccw(poly->rings[0]) )
    {
            ptarray_reverse(poly->rings[0]);
    }

    for (i=1; i<poly->nrings; i++)
    {
            if ( ! ptarray_isccw(poly->rings[i]) )
            {
                    ptarray_reverse(poly->rings[i]);
            }
    }

Since this code does do the right thing (as I understand it) for
the exterior ring and the wrong thing for the interior rings,
ptarray_isccw() must be returning true when the points are clockwise
and false when they're counter-clockwise, which is contrary to what
its name suggests (I'm assuming "isccw" means "is counter-clockwise").
Shouldn't ptarray_isccw() return true when the points are counter-
clockwise?  And if so then shouldn't the exterior ring's test be
"if ( ! ptarray_isccw(poly->rings[0]) )"?

I've attached a patch that makes ForceRHR() work the way I was
expecting: return the points of the exterior and interior rings in
counter-clockwise order.  If I've misunderstood what ForceRHR() is
supposed to do then somebody please correct me.  Thanks.

-- 
Michael Fuhr
-------------- next part --------------
Index: lwgeom/lwgeom.c
===================================================================
RCS file: /home/cvs/postgis/postgis/lwgeom/lwgeom.c,v
retrieving revision 1.31
diff -c -r1.31 lwgeom.c
*** lwgeom/lwgeom.c	29 Jan 2006 13:54:38 -0000	1.31
--- lwgeom/lwgeom.c	12 Mar 2006 03:39:03 -0000
***************
*** 133,139 ****
  	switch (TYPE_GETTYPE(lwgeom->type))
  	{
  		case POLYGONTYPE:
! 			lwpoly_reverse((LWPOLY *)lwgeom);
  			return;
  
  		case MULTIPOLYGONTYPE:
--- 133,139 ----
  	switch (TYPE_GETTYPE(lwgeom->type))
  	{
  		case POLYGONTYPE:
! 			lwpoly_forceRHR((LWPOLY *)lwgeom);
  			return;
  
  		case MULTIPOLYGONTYPE:
Index: lwgeom/lwgeom_api.c
===================================================================
RCS file: /home/cvs/postgis/postgis/lwgeom/lwgeom_api.c,v
retrieving revision 1.85
diff -c -r1.85 lwgeom_api.c
*** lwgeom/lwgeom_api.c	19 Jan 2006 18:16:17 -0000	1.85
--- lwgeom/lwgeom_api.c	12 Mar 2006 03:39:03 -0000
***************
*** 1869,1876 ****
  		getPoint2d_p(pa, i+1, &p2);
  		area += (p1.x * p2.y) - (p1.y * p2.x);
  	}
! 	if ( area > 0 ) return 0;
! 	else return 1;
  }
  
  /*
--- 1869,1876 ----
  		getPoint2d_p(pa, i+1, &p2);
  		area += (p1.x * p2.y) - (p1.y * p2.x);
  	}
! 	if ( area > 0 ) return 1;
! 	else return 0;
  }
  
  /*
Index: lwgeom/lwpoly.c
===================================================================
RCS file: /home/cvs/postgis/postgis/lwgeom/lwpoly.c,v
retrieving revision 1.21
diff -c -r1.21 lwpoly.c
*** lwgeom/lwpoly.c	30 Dec 2005 17:40:37 -0000	1.21
--- lwgeom/lwpoly.c	12 Mar 2006 03:39:03 -0000
***************
*** 463,469 ****
  {
  	int i;
  
! 	if ( ptarray_isccw(poly->rings[0]) )
  	{
  		ptarray_reverse(poly->rings[0]);
  	}
--- 463,469 ----
  {
  	int i;
  
! 	if ( ! ptarray_isccw(poly->rings[0]) )
  	{
  		ptarray_reverse(poly->rings[0]);
  	}