Rewrite parts of sweep code #1593
Merged
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
This is potentially panicky, while the destructuring will fail at compile-time.
This implementation will be removed as part of the ongoing `HalfEdge` cleanup.
This is going to be required in the face sweep code, to make sure the top of the solid is properly stitched up with the sides.
The rewrite of the edge sweeping code uses the builder API, making the code more compact, and making the sweep code for `Vertex` redundant (it can be removed in a follow-up commit. The rewrite of the face sweeping code is less of a slam dunk. It adds a fair bit of complexity over the previous version, but I think this is okay, due to two factors: 1. I believe that this complexity can be reigned in later, once the core data structures have been simplified. This is an ongoing effort. 2. It fixes existing bugs. See below. The initial reason for the rewrite was the ongoing `HalfEdge` cleanup: As part of the celanup, the `Reverse` implementation for `HalfEdge` needs to be removed, and this new code no longer uses it. However, as it turned out, there is a significant side effect: The new code generates different (but still correct) curves, which are less uniform than the ones generated by the old code, which happens to trigger approximation failures that exposed existing bugs. Those bugs basically boil down to the previous sweep code generating coincident edges that don't refer to the same global edge. This was already known (#1162), but so far a proper fix was blocked on missing validation code, to ensure all instances of this bug are fixed. That validation code still needs to be written, but the approximation failures already did a pretty good job of guiding me towards the various sources of the bug.
2 tasks
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.
This is part of the ongoing
HalfEdgecleanup (#1525).From the message of the main commit:
This rewrite caused me so many problems. I literally spent weeks on getting this right! Not sure if it was worth it in the end, but it is very nice to be able to close those bugs and unblock further work on #1525.
Close #494
Close #1162 (will open follow-up issue about validation checks)