search

GeoLatte / geolatte-geom

A geometry model that conforms to the OGC Simple Features for SQL specification.
Other
132 stars 63 forks source link

Fix isCounterClockwise implementation #77

Closed jvdvegt closed 5 years ago

jvdvegt commented 5 years ago

This fixes the algorithm to work as documented.

It seems that with commit 7773d90 the logic broke. Note that the commit-comment doesn't match javadoc. Due to 'det == 0' in while-condition, the loop was only executed once.

Added a test to demonstrate the fix.

jvdvegt commented 5 years ago

I found another issue: if the first two points have the exact same location, my updated version still won't work correctly. You'd need to actually compute the determinant for points at the locations (0,1,2), then (1,2,3), then (3,4,5), etc. This patch will compute the determinant for (0,1,2), (0,1,3), (0,1,4) etc, so it still needs some (minor) work. If you'd like me to do this, let me know.

maesenka commented 5 years ago

I looked into the code and learned that the proper way to do this is to use the Shoelace formula, so I implemented that. It passes your test case. Btw, I translated the test case to a Cartesian coordinate space because I find that easier to think in.

jvdvegt commented 5 years ago

Thanks, your code looks great. Though you don't need p0 and p1 anymore...

Regards,

Jeroen.

Op wo 20 mrt. 2019 22:08 schreef Karel Maesen notifications@github.com:

I looked into the code and learned that the proper way to do this is to use the Shoelace formula, so I implemented that. It passes your test case. Btw, I translated the test case to a Cartesian coordinate space because I find that easier to think in.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/GeoLatte/geolatte-geom/pull/77#issuecomment-475028936, or mute the thread https://github.com/notifications/unsubscribe-auth/ACevIzLzt3DJDfzGUJQxODgp-NJrAMb8ks5vYqM-gaJpZM4bqla5 .