Simplify creation of boundary representation #254
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
It fits better there. The modules in there is where it's implemented, after all.
I'm going to need the name `Shape` for another type I'm going to add.
The struct it returns is just a placeholder right now, but the method is intended to replace `ToShape::faces`, `ToShape::edges`, and `ToShape::vertices`.
This is not a pure refactoring, as there's one shape that would have previously panicked, if faces were requested from it, which now just returns no faces.
This is going to help with merging `edges` into `to_shape`.
This is not a pure refactoring, as there are shapes that would have previously panicked, if edges were requested from them, which now just returns no edges.
This will make merging `vertices` into `to_shape` easier.
No vertices are required to compute the bounding volume of the sketch, so none should be created.
This is not a pure refactoring, as there are shapes that would have previously panicked, if vertices were requested from them, which now just returns no vertices.
The issue it describes has been resolved, thanks to the redesign of the `ToShape` trait.
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.
Merges most methods of the
ToShapetrait (renamed fromShape) into a singleto_shapemethod, where the boundary representation of the shape is created. This simplifies theToShapeimplementations somewhat (although it also created some methods that are now way too long).This is not a pure refactoring. Some shapes that would have previously panicked, if vertices, edges, or faces were requested from them (because that functionality had not yet been implemented), will now just return no vertices/edges/faces. In the short term, that might replace what would have been obvious errors with silent failures, leading to more bugs.
I don't like this, but I think it's a worthwhile trade-off. The new approach, with the single
Shapestruct that holds all vertices/edges/faces of a shape, provides a much better basis for future cleanups, as well as for adding more validation. Specifically, this enables #242, and the sweep clean-ups required to continue work on #97.Close #176