-
Notifications
You must be signed in to change notification settings - Fork 28
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
Improve event ordering logic in segment division #11
Merged
untoldwind
merged 22 commits into
21re:master
from
bluenote10:feature/improve_event_ordering_logic_in_segment_division
Feb 24, 2020
Merged
Improve event ordering logic in segment division #11
untoldwind
merged 22 commits into
21re:master
from
bluenote10:feature/improve_event_ordering_logic_in_segment_division
Feb 24, 2020
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Looks good ... and good to know that num_traits conversion seems to work now. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I spend a while thinking about how to solve #3 / w8r/martinez#99 and came to the conclusion that this is the easiest solution. The idea has two steps:
The most extreme example is a line segment that differs in just one ULP (unit of least precision) in x, going from top to bottom:
The intersection point can either take the left x value (case 1) or the the right x value (case 2).
In terms of the event order, the normal case after subdivision is se_l < se_intersection < se_r. The problem is that perfectly vertical line segments must be consistently processed in bottom-to-top order, but the original line segment has top-to-bottom order. This means that the order of the newly produced vertical segment must be reversed.
In the second case reversing the order is not a problem, because both events are future events that will be processed later.
Case 1 is a bigger problem though, because se_l is the event currently being processed, and a theoretically correct processing order would have required to process se_intersection earlier. To avoid overly complex backtracking logic, I think it should be fine to simply increment the x-value of the intersection point by 1 ULP (i.e., always avoiding the order reversal in the first subsegment). I've measured the accuracy of the intersection point computation, and due to cancellation effects the error is anyway much larger than 1 ULP.
Minor changes:
nextafter
function, which Rust doesn't offer by default unfortunately. At first I was using thefloat_extra
crate, but since it doesn't offer thef32
variant of nextafter, I decided to follow the SO recommendation of manually wrapping the C function.cargo fmt
&cargo clippy
(clippy suggested to remove a few existing clones, I hope they weren't there an purpose).Visualization of new/modified test cases: test_cases.pdf