hypar-io / Elements

The smallest useful BIM.
https://www.hypar.io
MIT License
354 stars 74 forks source link

Add edge intersection test to Contains3D #767

Open ikeough opened 2 years ago

ikeough commented 2 years ago

It might not be enough to only check vertices in non-convex cases. Consider to check edge intersections, see image.

afbeelding

_Originally posted by @aothms in https://github.com/hypar-io/Elements/pull/638#discussion_r792408055_

DmytroMuravskyi commented 1 year ago

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.

wynged commented 1 year ago

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.

DmytroMuravskyi commented 1 year ago

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.