Add winding validation for sketches #2256
Conversation
emka
changed the title
|
Test cases |
There was a problem hiding this comment.
Thank you, @emka, great work!
I left some comments with suggestions. But to be clear, none of those need to be addressed. Feel free to look into them, if you're up for it. But I'm happy to leave this as-is, as the code can always be cleaned up later, when making changes to it.
What needs to be addressed is the CI failure.
Test cases
should_find_cycle_multiple_referencesandshould_find_half_edge_multiple_referencesare currently failing because they contain cycles without valid winding. Do you have a preference how to handle this case?
My preferred approach to these tests is to build a valid object using high-level builder APIs, then create an invalid variant by doing a minimal update of the valid object. Just like I've suggested for the tests you added.
I'm not too picky, however. Looks like the failing tests can be fixed by just replacing Cycle::new with Cycle::circle, so feel free to just do that!
| let valid_outer_circle = | ||
| HalfEdge::circle([0., 0.], 1., &mut core).insert(&mut core); | ||
| let valid_exterior = | ||
| Cycle::new(vec![valid_outer_circle.clone()]).insert(&mut core); | ||
| let valid_sketch = | ||
| Sketch::new(vec![ | ||
| Region::new(valid_exterior.clone(), vec![]).insert(&mut core) | ||
| ]); |
There was a problem hiding this comment.
This can be shortened using BuildRegion, specifically, BuildRegion::circle.
There's also BuildSketch, but that's quite rudimentary. Would be nice to have some more methods there too, for cases like this, but that's not within the scope of this pull request.
| let invalid_outer_circle = HalfEdge::from_sibling( | ||
| &valid_outer_circle, | ||
| Vertex::new().insert(&mut core), | ||
| ) | ||
| .insert(&mut core); | ||
| let invalid_exterior = | ||
| Cycle::new(vec![invalid_outer_circle.clone()]).insert(&mut core); | ||
| let invalid_sketch = | ||
| Sketch::new(vec![ | ||
| Region::new(invalid_exterior.clone(), vec![]).insert(&mut core) | ||
| ]); |
There was a problem hiding this comment.
This can probably be shortened by updating the valid sketch. Something like this:
let invalid_sketch = valid_sketch.update_region(valid_sketch.regions().first(), |region|
region.update_exterior(|cycle| cycle.reverse())
);I didn't test this, so this might not work precisely like this. But something along those lines should be possible.
| let outer_circle = | ||
| HalfEdge::circle([0., 0.], 2., &mut core).insert(&mut core); | ||
| let inner_circle = | ||
| HalfEdge::circle([0., 0.], 1., &mut core).insert(&mut core); | ||
| let cw_inner_circle = HalfEdge::from_sibling( | ||
| &inner_circle, | ||
| Vertex::new().insert(&mut core), | ||
| ) | ||
| .insert(&mut core); | ||
| let exterior = Cycle::new(vec![outer_circle.clone()]).insert(&mut core); | ||
|
|
||
| let valid_interior = | ||
| Cycle::new(vec![cw_inner_circle.clone()]).insert(&mut core); | ||
| let valid_sketch = Sketch::new(vec![Region::new( | ||
| exterior.clone(), | ||
| vec![valid_interior], | ||
| ) | ||
| .insert(&mut core)]); |
There was a problem hiding this comment.
Can probably be shortened by using available APIs. Maybe (attention, pseudo-code) Region::circle().add_interior(invalid_circle).
| let invalid_interior = | ||
| Cycle::new(vec![inner_circle.clone()]).insert(&mut core); | ||
| let invalid_sketch = Sketch::new(vec![Region::new( | ||
| exterior.clone(), | ||
| vec![invalid_interior], | ||
| ) | ||
| .insert(&mut core)]); |
There was a problem hiding this comment.
Can probably be shortened using Sketch::update_region and other operations, as suggested above in the other test.
|
Thank you for the feedback! I want to take care of the unresolved comments as well before merging. |
|
Great, thank you! Let me know when this is ready for another look. One additional note: It would be great, if the commits with changes to the existing tests came before the commit with new validation check, to make sure every single commit builds/tests correctly (this can be very useful when using |
There was a problem hiding this comment.
Hey @emka, I've decided to merge this pull request right away. I'm about to make some changes to the validation code that are not directly related to this pull request, but might touch some of the same code. Keeping this open will only lead to merge conflicts and frustration.
I'll do a rebase before merging, to get the commits in the order I requested.
I'd be happy to accept further improvements in follow-up pull requests!
Use BuildRegion to create cycles with valid windings.
Add info to help determine which cycles are wrong
hannobraun
force-pushed
the
validate_winding
branch
from
b8e3ada
to
9f6bb58
Compare
Tests use circles, arcs currently can not be tested (#2130). Adding sketch validation for #2158.