Skip to content
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

The mollerTrumboreIntersect.py implements three intersection algorithm and checks for class: why not method in each class? #120

Open
dccote opened this issue Aug 6, 2024 · 1 comment
Assignees
Labels
enhancement New feature or request

Comments

@dccote
Copy link
Contributor

dccote commented Aug 6, 2024

Describe the bug
The MollerTrumboreIntersect class implemented triangle, quad and polygon intersect. The main call is getIntersection, which checks for the class requesting the intersection. This really looks like the getIntersection function should exist in each class (Triangle, Quad and Polygon) and should simply implement the specifics of the intersection.

Why is this not the case?

To Reproduce
See MollerTrumboreIntersect.getIntersection()

Expected Design
Looks like a simple chip method for each case would work

@dccote dccote assigned dccote and JLBegin and unassigned dccote Aug 6, 2024
@dccote dccote added the enhancement New feature or request label Aug 6, 2024
@JLBegin
Copy link
Contributor

JLBegin commented Aug 12, 2024

While the current implementation can feel a bit misaligned with OO practices (depending on your interpretation of the domain), it has benefits:

  • Flexible intersection strategies (OCP) with easy injection through a single IntersectionFinder endpoint, rather than managing separate objects (polygon, triangle, quad, bounding box). This was used extensively in the past to compare the performance of different intersectors and while we have settled on a preferred solution, I would be hesitant to remove this flexibility.
  • From my point of view, the intersection module is at a higher-level than the geometry module and is also quite complex, so the current implementation clearly seperates concerns (SRP).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants