Add methods to determine orientation of PolyLine #1411

Merged
merged 1 commit into from Oct 21, 2016

Projects

None yet

2 participants

@drewish
Contributor
drewish commented Apr 2, 2016 edited

I'd been using CGAL's Polygon_2 to determine the orientation but realized the math isn't that complicated and would be a nice addition to the PolyLine class.

The algorithm is based of this answer the FAQ for comp.graphics.algorithms:

Subject 2.07: How do I find the orientation of a simple polygon?

Compute the signed area (Subject 2.01).  The orientation is 
counter-clockwise if this area is positive.  

A slightly faster method is based on the observation that it isn't
necessary to compute the area.  Find the lowest vertex (or, if 
there is more than one vertex with the same lowest coordinate, 
the rightmost of those vertices) and then take the cross product 
of the edges fore and aft of it.  Both methods are O(n) for n vertices, 
but it does seem a waste to add up the total area when a single cross
product (of just the right edges) suffices.  Code for this is
available at ftp://cs.smith.edu/pub/code/polyorient.C (2K).

The reason that the lowest, rightmost (or any other such extreme) point
works is that the internal angle at this vertex is necessarily convex, 
strictly less than pi (even if there are several equally-lowest points).

And some other details I found on the Wikipedia Curve orientation page.

The code is kind of klunky but works well. It includes some basic unit tests.

@drewish drewish changed the title from Add methods to determin orientation of PolyLine to Add methods to determine orientation of PolyLine Apr 2, 2016
@drewish
Contributor
drewish commented Oct 12, 2016

I think this is pretty solid at this point. I've been using it along with some CGAL code that would barf if the orientation wasn't always CCW so It helped me track down a couple of small bugs in this. Those issues helped inspire some of the test cases.

@andrewfb andrewfb self-assigned this Oct 18, 2016
@andrewfb andrewfb added the geom label Oct 18, 2016
@andrewfb
Collaborator

Thanks for your work on this and apologies for the delay. The implementation looks good to me, minus a few formatting things I can easily fix. It's a 100% subjective but I'd like to propose an interface tweak. For the most part we avoid function-specific enums in Cinder. To that end, I'd propose we ditch the Orientation enum in favor of an interface like:

//! Returns \c true if PolyLine is clockwise-oriented. If \a isColinear is non-null, it receives \c true if all points are colinear
bool  isClockwise( bool *isColinear = nullptr );
//! Returns \c true if PolyLine is counterclockwise-oriented. If \a isColinear is non-null, it receives \c true if all points are colinear
bool  isCounterclockwise( bool *isColinear = nullptr );

Again, I realize this is mostly just a different flavor of imperfect, but this would be a bit more consistent with Cinder at large.

@drewish
Contributor
drewish commented Oct 18, 2016

I'd just followed the pattern that CGAL was using so I'm happy to make those changes. Feel free to point out any other spots with formatting issues.

@drewish
Contributor
drewish commented Oct 18, 2016

@andrewfb while i've got your attention could you give #1034 and #1412 a look too?

@andrewfb
Collaborator

Hey - the formatting stuff I'm noticing are mostly extra spaces, like
if ( mPoints.size() < 3 )
should be
if( mPoints.size() < 3 )

It's not all-encompassing but most of the important stuff is in the Contributing guide.

And I will take a look at the other two as well while we're in here.

@drewish
Contributor
drewish commented Oct 18, 2016

@andrewfb Ah, thanks... I'll give that a closer read. So are you proposing dropping the orientation method? Or would that remain and just return -1, 0, +1 to avoid the enum?

@andrewfb
Collaborator

I'd propose dropping it.

bool colinear;
if( p.isCounterclockwise( &colinear ) )
    cout << "Counterclockwise";
else if( colinear )
   cout << "Colinear";
else
   cout << "Clockwise";

is roughly how I'd see this used in a case where we need to handle colinear. For the case where we're not worried about it:

if( p.isCounterclockwise() )
    cout << "Counterclockwise";
else
    cout << "Clockwise"; // or colinear, but we're not concerned about that

Open to other opinions here, but I think the main advantage to orientation() is that it avoids the need for a local colinear var, which we basically can't avoid when we drop the Orientation enum.

@drewish
Contributor
drewish commented Oct 19, 2016

@andrewfb I took a pass at this and I'm pretty happy with how it turned out. I made the changes as a separate commit so you could compare the two approaches. I can squash that down when you're happy or you can do that when you merge.

@drewish
Contributor
drewish commented Oct 21, 2016

I went ahead and rebased this to pick up the other polyline changes and squashed it down.

@andrewfb andrewfb merged commit c1a91f3 into cinder:master Oct 21, 2016
@andrewfb
Collaborator

Thanks very much for this, particularly for including the unit test cases. I made a minor tweak - a case change to make isCounterclockwise(), in keeping with what I believe is the most common spelling convention.

@drewish
Contributor
drewish commented Oct 22, 2016

Ah nice data point for the naming. Glad to finally see this merged.

@drewish drewish deleted the drewish:polyline-orientation branch Oct 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment