Fix point-in-polygon test and use a better test in polyhedron.

[kduske]

Issue: #1573

[ericwa]

Nice how you could remove the polygonContainsPoint() call from Polyhedron::addPointToPolygon!

The only point-in-polygon test I know is the one HalfEdge::pointStatus uses; making a plane for each edge by taking the cross product of the face normal and the edge's direction vector, and reporting a point as "in the polygon" if it's behind all of those edge planes, and "outside" if it's not behind all.

I'm not sure how polygonContainsPoint() works at first glance. If more bugs turn up, I'd be tempted to replace it with the above algorithm.. but that's just the one I am familiar with.

Some randomized tests would be good, for both polygonContainsPoint and Polyhedron::addPoint, but that can be done later, and it's some work to set up.

[kduske]

Nice how you could remove the polygonContainsPoint() call from Polyhedron::addPointToPolygon!

Yeah, I had this idea when I introduced that function call in the first place, but dropped it.

I'm not sure how polygonContainsPoint() works at first glance. If more bugs turn up, I'd be tempted to replace it with the above algorithm.. but that's just the one I am familiar with.

It's very simple, it projects the polygon and point onto the XY, XZ, or YZ planes and then essentially counts how often a line through the point crosses the polygons boundary. It's standard, I remember that I took it from somewhere. It's used mostly for ray / polygon intersection tests. Please don't replace it. It's fine for those cases. The problem here was twofold:

• First, I was being inconsistent regarding how points on polygon edges were treated. They are now always considered to be inside the polygon.
• Second, polygonContainsPoint is less precise than what we need for Polyhedron, so it was not consistent with how the algorithms in that template work. This introduced a dependency on the order in which points are inserted.

Some randomized tests would be good, for both polygonContainsPoint and Polyhedron::addPoint, but that can be done later, and it's some work to set up.

If by randomized tests you mean "random for every test run", then I'm against it because tests must always be repeatable. If you mean "generated randomly and checked into source control for all eternity", then that may be doable.

[ericwa]

If you mean "generated randomly and checked into source control for all eternity", then that may be doable.

Agree, the tests must be repeatable. I was thinking of one of c++11's generators like std::mt19937, which will always produce the same sequence on any c++11 implementation.

[kduske]

Alright, yeah, that might work. But you'll need a test oracle.