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

fixed removing non identical ruling from the tree #488

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

zanninso
Copy link

@zanninso zanninso commented Aug 9, 2022

i found that the ruling find intersection method doesn't give me all the intersections, so after investigating the problem in found that is because of the comparator in the tree on the ruling, it compare just the top/y so that lead to remove any ruling have the same top/y.

so it's better to use the hashcode of the object as the key an the object it self as the value, this fixed the problem for me.

@jeremybmerrill
Copy link
Member

I spoke with @zanninso via email. We discussed that we didn't originally intend for findIntersections() to be called without first calling collapseOrientedRulings, which should fix this problem (by guaranteeing that there are no overlapping lines with the same orientation).

I don't, however, recall why the TreeMap using Ruling.top() as the comparator, rather than the hashcode (as proposed in this PR). It seems either might work and the suggestion might be simpler. @jazzido do you recall?

@azeddine-leet
Copy link

bump

@zanninso
Copy link
Author

hello
The problem that we discussed before it just happened in the unexcepted way, even following the process and call "collapseOrientedRulings()" before calling "findIntersections()" , it's harder to get the case where it happens, but i will try to investigate when i get some free time and i will send it to you.

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

Successfully merging this pull request may close these issues.

3 participants