Add Cycle validation #1686
Add Cycle validation #1686
Conversation
There was a problem hiding this comment.
Thank you for this pull request, @A-Walrus! We used to have a validation check for that, but it seems like it got lost during my recent clean-up, and I missed that. Thank you for adding it back!
I've left some comments. Only one of them is critical (the one about HalfEdge::end_position). The other ones would be nice-to-have, but I'd be perfectly happy to merge this PR without them being addressed.
| // Computing the surface position from the curve position is fine. | ||
| // `HalfEdge` "owns" its end position. There is no competing code that | ||
| // could compute the surface position from slightly different data. |
There was a problem hiding this comment.
This comment is completely false, and dangerously so. It used to be true for the start position (from where it was adapted), but by adding this method, you add the competing code that the comments in both methods claim doesn't exist.
This is a problem, because sooner or later someone is going to call this method to compare it to the start position of the next half-edge in the cycle, or use it in some scenario where it's assumed that both positions are going to be equal.
I think this method shouldn't exist. It's still possible to make the same mistake doing exactly what the method does internally, but at the very least we shouldn't make that easy. (And there should be better documentation around this, but that's a separate issue.)
However, what the new validation check does is perfectly fine, as it uses an epsilon value (config.identical_max_distance) for the comparison. I think it would be best to inline the body of this method into the validation check.
| // If there are no half edges | ||
| if cycle.half_edges().next().is_none() { | ||
| errors.push(Self::NotEnoughHalfEdges.into()); | ||
| return; | ||
| } |
There was a problem hiding this comment.
This is fine as-is, but I think it would be even better, if this check had its own method. Then there would be an easy 1:1 relationship between error conditions and methods. I think this would be slightly clearer.
| // Chain the first half_edge so that we make sure that the last connects to the first. | ||
| // This unwrap will never fail because we checked before that there are enough half_edges. | ||
| .chain(std::iter::once(cycle.half_edges().next().unwrap())) | ||
| .tuple_windows() |
There was a problem hiding this comment.
Also fine as-is, but there's an easier (and less panicky) way to do the same thing:
| // Chain the first half_edge so that we make sure that the last connects to the first. | |
| // This unwrap will never fail because we checked before that there are enough half_edges. | |
| .chain(std::iter::once(cycle.half_edges().next().unwrap())) | |
| .tuple_windows() | |
| .circular_tuple_windows() |
| )) | ||
| )); | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Not strictly necessary, but it would be nicer to also have a test that checks for the second validation error (NotEnoughHalfEdges).
A-Walrus
force-pushed
the
cycle_validation
branch
from
8927cbd
to
deade38
Compare
There was a problem hiding this comment.
Thank you for the changes, @A-Walrus!
Looks like HalfEdge::end_position still exists in the final version. If you can remove it, then this is ready to merge.
A-Walrus
force-pushed
the
cycle_validation
branch
from
f3728c0
to
eb95ffb
Compare
Forgot about the fact that a single circle counts as a cycle
- Inlined the `end_position` function to not expose potentially error- prone behaviour. - Seperate empty check to new method. - Switch to `circular_tuple_windows` - Add test for empty cycle
hannobraun
force-pushed
the
cycle_validation
branch
from
eb95ffb
to
f0423c6
Compare
Add basic checks to ensure that
Cycles contain at least 2 half-edges, and that all half-edges are connected to each-other.