-
Notifications
You must be signed in to change notification settings - Fork 73
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add edge intersection test to Contains3D #767
Comments
There is Cover function that also checks Polygon and it checks for edge intersections but it's 2d oriented (uses Intersects2d). Its logic can be reused in Contains with adjustment that Contains only wants Inside result and not CoincidesAtEdge/Vertex. It will fix this problem, but, in my opinion, there is some confusion between Cover/Contains/Contains3d functions.
In my opinion, ideally, there should be one private implementation (based on Clipper or custom implementation) for Point-Polygon containment and one for Polygon-Polygon containment that works efficiently on XY plane and different public function can be built on top of it depending on how they treat CoincidesAtEdge/Vertex case, do they expect 2d or 3d input. And if 3d input is expected then shared plane is first converted to XY plane. |
how long do you think the larger effort would take? I think if we can time box it to 1/2 days, fix this bug, and preserve existing tests then it sounds like a good idea. If it's a longer effort let's do the quick fix for now and maybe make another issue to track these findings, but it doesn't have to be part of 2.1 milestone. |
The changes will fit in two days as all code is there it's just about interface being clear and code is reused. It's good idea to create another issue for this as it would require same amount of discussion. I'll focus on adding edge intersection test to Contains3D as well as make sure that all functions reusing the same internal algorithms. |
It might not be enough to only check vertices in non-convex cases. Consider to check edge intersections, see image.
Originally posted by @aothms in #638 (comment)
The text was updated successfully, but these errors were encountered: