golang / geo

S2 geometry library in Go
Apache License 2.0
1.69k stars 182 forks source link

SimpleCrossing: differences from C++ tests #3

Closed rlmcpherson closed 8 years ago

rlmcpherson commented 8 years ago

I noticed a difference in the expected behavior of SimpleCrossing between the C++ and Go version of this library.

C++ test is here: https://code.google.com/p/s2-geometry-library/source/browse/geometry/s2edgeutil_test.cc#85 Go test: https://github.com/golang/geo/blob/master/s2/edgeutil_test.go#L48

The C++ test expects true while the go code expects false. I noticed similar behavior with the same test while trying to implement and test RobustCrossing.

Is this a bug in the go library or a change in behavior at edge conditions in the RobustSign function?

The go test for RobustCrossing here: https://github.com/rlmcpherson/geo/blob/master/s2/edgeutil_test.go#L233

dsymonds commented 8 years ago

@rsned

rsned commented 8 years ago

The C++ test is somewhat confusing in that the bool passed into TestCrossings is to run the SimpleCrossing, not what it's value should be. So this case you point to passes in true for simple, and -1 for robust in that test case, and the actual comparison is then:

https://code.google.com/p/s2-geometry-library/source/browse/geometry/s2edgeutil_test.cc#50

void TestCrossing(S2Point a, S2Point b, S2Point c, S2Point d, int robust, bool edge_or_vertex, bool simple) { ... if (simple) { EXPECT_EQ(robust > 0, S2EdgeUtil::SimpleCrossing(a, b, c, d)); }

So with robust = -1, the result should be false.

On Thu, Jan 21, 2016 at 5:03 PM, David Symonds notifications@github.com wrote:

@rsned https://github.com/rsned

— Reply to this email directly or view it on GitHub https://github.com/golang/geo/issues/3#issuecomment-173766850.

rlmcpherson commented 8 years ago

Thanks @rsned. You're correct, I misread the C++ testing code as the expected value of SimpleCrossing.

Looking closer at RobustSign, my guess is that the test case for RobustCrossing linked above is returning Indeterminate instead of Clockwise for two edges on the same great circle because ExactCCW is not yet implemented but I'll try to confirm that.

Closing this issue.