Shell and Solid validation #1695

A-Walrus · 2023-03-19T10:39:35Z

Attempt to validate Shell's by making sure that each half-edge in a shell refers to a global-edge that is referred to by another half-edge in that same shell. My logic says that each edge in a shell should be a part of exactly two faces but I may be wrong.

This validation check currently fails with cargo run --package export- validator, but I'm not sure if this is a bug in my validation checks, or whether I exposed a bug in existing code.

For example looking at the cuboid example, many global edges are referred to by only one half_edge, which seems wrong to me...

edit by @hannobraun:
Close #1594

hannobraun · 2023-03-19T11:57:06Z

Thank you, @A-Walrus! I'll take a closer look on Monday. For now, some comments.

My logic says that each edge in a shell should be a part of exactly two faces but I may be wrong.

No, I think that should be correct! At least it matches what I've been thinking.

For example looking at the cuboid example, many global edges are referred to by only one half_edge, which seems wrong to me...

Yes, that is definitely wrong. Unless there's a problem with the new validation code, this is likely a bug in the sweep code. I'm pretty sure that the sweep code creates the correct vertices (I've had problems with exactly that recently, and I'm pretty sure that's dealt with), but it might be creating duplicate GlobalEdges. As long as those refer to the correct vertices, it's plausible that all the code we currently have still works correctly.

So yeah, totally plausible you exposed a bug!

hannobraun · 2023-03-20T14:39:47Z

I've taken a look at the sweep code, and I think I've found a bug:
https://github.com/hannobraun/Fornjot/blob/ccebda5e0adb248deb52aa51a1c63901540da27e/crates/fj-kernel/src/builder/cycle.rs#L48-L49

This is missing a call to HalfEdgeBuilder::with_global_form. That this call is missing definitely explains the validation failure.

Problem is though, if I add the call, validation still fails. I don't know what that means. Either there are more cases like this (I couldn't find any), or something weird is going on.

hannobraun · 2023-03-20T14:51:47Z

Here's the change I made to the code I quoted in my previous comment:

--- a/crates/fj-kernel/src/builder/cycle.rs
+++ b/crates/fj-kernel/src/builder/cycle.rs
@@ -44,9 +44,10 @@ impl CycleBuilder {
         let half_edges = edges
             .into_iter()
             .circular_tuple_windows()
-            .map(|((prev, _, _), (_, curve, boundary))| {
+            .map(|((prev, _, _), (half_edge, curve, boundary))| {
                 HalfEdgeBuilder::new(curve, boundary)
                     .with_start_vertex(prev.start_vertex().clone())
+                    .with_global_form(half_edge.global_form().clone())
             })
             .collect();

A-Walrus · 2023-03-20T16:50:35Z

There seems to have been a couple problems:

What you found about with_global_form
Incorrect order in global_edges
Needing to cache edges like we do vertices

A-Walrus · 2023-03-20T16:51:25Z

crates/fj-kernel/src/algorithms/sweep/mod.rs

+    /// Cache for global edges
+    pub global_edge: BTreeMap<ObjectId, Handle<GlobalEdge>>,


Maybe we should have a more general caching solution. This seems to work for now but IDK if it's ideal.

I don't know, this seems fine to me. If you come up with something better, feel free to suggest it of course, but I think we can keep it as-is.

A-Walrus · 2023-03-20T16:51:55Z

crates/fj-kernel/src/objects/full/face.rs

@@ -75,7 +75,7 @@ impl Face {
        self.interiors.iter()
    }

-    /// Access all cycles of the face
+    /// Access all cycles of the face (both exterior and interior)


Sorry for the unrelated change to the docs, just wanted to clear it up

That's fine! In general, I prefer to have unrelated changes in different commits, as I think that makes things a bit clearer overall, but that is a weak preference. We can leave it as-is.

Attempt to validate `Shell`'s by making sure that each half-edge in a shell refers to a global-edge that is referred to by another half-edge in that same shell. My logic says that each edge in a shell should be a part of exactly two faces but I may be wrong. This validation check currently fails with `cargo run --package export- validator`, but I'm not sure if this is a bug in my validation checks, or whether I exposed a bug in existing code. For example looking at the cuboid example, many global edges are referred to by only one half_edge, which seems wrong to me...

Validate whether coincident HalfEdges refer to the same GlobalEdge. This should resolve: hannobraun#1594. Checks whether they coincide by sampling.

Currently there are no tests for valid cases

This allows us to easily check whether some object raises some validation error, without requiring it to be the first. Switched to this macro whenever there was an assertion that the **first** validation error matches some pattern.

Simplify code to only sample 3 points, since that is enough for cicrcles and lines.

Fixed cycle builder like you commented, global_edge order, and added an edge cache. PS: We might want to make the `SweepCache` more general

Fix many inaccuracies and cleanup code.

Before it was in shell. This partially addresses hannobraun#1689, holding off on validating sketches until we figure out sketch-face relationship: hannobraun#1691

hannobraun

Thank you, @A-Walrus, this is great!

There seems to have been a couple problems:

What you found about with_global_form

Incorrect order in global_edges

Needing to cache edges like we do vertices

This is some nice debugging. Well done!

I left some comments, but there's nothing critical. There's one suggestion that I will apply myself after submitting this review. This is good to merge!

Some additional comments:

In general, I'd prefer if the final order of commits were such, that every single commit works (that means moving the fixes before the validations that require them). This doesn't matter most of the time, but if you're using git bisect, then a commit that has build errors or test failures can be inconvenient.
As you mention, the test cases only cover failure. This would be hard to fix right now, as we don't have good tools to build Shells and Solids. But since I'm going to focus on APIs to build shapes for the foreseeable future, those test cases are probably a good testing ground for those new APIs.

hannobraun · 2023-03-21T09:38:31Z

crates/fj-kernel/src/objects/full/face.rs

@@ -75,7 +75,7 @@ impl Face {
        self.interiors.iter()
    }

-    /// Access all cycles of the face
+    /// Access all cycles of the face (both exterior and interior)


That's fine! In general, I prefer to have unrelated changes in different commits, as I think that makes things a bit clearer overall, but that is a weak preference. We can leave it as-is.

crates/fj-kernel/src/validate/shell.rs

hannobraun · 2023-03-21T10:01:07Z

crates/fj-kernel/src/algorithms/sweep/mod.rs

+    /// Cache for global edges
+    pub global_edge: BTreeMap<ObjectId, Handle<GlobalEdge>>,


I don't know, this seems fine to me. If you come up with something better, feel free to suggest it of course, but I think we can keep it as-is.

hannobraun · 2023-03-21T10:08:53Z

crates/fj-kernel/src/algorithms/sweep/vertex.rs

-        let global_edge = GlobalEdge::new().insert(objects);
+        let global_edge = cache
+            .global_edge
+            .entry(self.id())
+            .or_insert_with(|| GlobalEdge::new().insert(objects))
+            .clone();


I swear, I was looking for exactly this code yesterday. I found the vertex caching code instead, and my brain was like "oh, so we do have the required caching". I'm pretty stupid sometimes 😄

In any case, good find!

hannobraun mentioned this pull request Mar 19, 2023

Allow validation to look at more than the object itself #1697

Closed

A-Walrus force-pushed the validate branch from 292c57c to 2977ab5 Compare March 20, 2023 13:54

A-Walrus commented Mar 20, 2023

View reviewed changes

A-Walrus changed the title ~~Attempt at Shell validation~~ Shell and Solid validation Mar 21, 2023

A-Walrus added 9 commits March 21, 2023 08:29

Add checks for coincident HalfEdges

e23a0fc

Validate whether coincident HalfEdges refer to the same GlobalEdge. This should resolve: hannobraun#1594. Checks whether they coincide by sampling.

Add basic tests for watertight and coincident

a7455ca

Currently there are no tests for valid cases

Add assert_contains_err! macro

ccee909

This allows us to easily check whether some object raises some validation error, without requiring it to be the first. Switched to this macro whenever there was an assertion that the **first** validation error matches some pattern.

Cleanup and make clippy happy

ec669d2

Only sample 3 points

44f606f

Simplify code to only sample 3 points, since that is enough for cicrcles and lines.

Fix Sweep edge duplication

ae2a3ab

Fixed cycle builder like you commented, global_edge order, and added an edge cache. PS: We might want to make the `SweepCache` more general

Fixup

edcfffb

Fix many inaccuracies and cleanup code.

Move vertex coincidence validation to solid

c88ac6c

Before it was in shell. This partially addresses hannobraun#1689, holding off on validating sketches until we figure out sketch-face relationship: hannobraun#1691

A-Walrus force-pushed the validate branch from 83bf61a to c88ac6c Compare March 21, 2023 06:29

A-Walrus marked this pull request as ready for review March 21, 2023 06:29

A-Walrus requested a review from hannobraun as a code owner March 21, 2023 06:29

hannobraun previously approved these changes Mar 21, 2023

View reviewed changes

Remove misplaced comment

f6cd58a

hannobraun dismissed their stale review via f6cd58a March 21, 2023 10:30

hannobraun enabled auto-merge March 21, 2023 10:31

hannobraun merged commit 8d4872a into hannobraun:main Mar 21, 2023

hannobraun mentioned this pull request Mar 21, 2023

Validate that coincident vertices within Sketch are identical #1689

Open

A-Walrus deleted the validate branch September 25, 2023 10:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Shell and Solid validation #1695

Shell and Solid validation #1695

A-Walrus commented Mar 19, 2023 •

edited by hannobraun

Loading

hannobraun commented Mar 19, 2023

hannobraun commented Mar 20, 2023

hannobraun commented Mar 20, 2023

A-Walrus commented Mar 20, 2023

A-Walrus Mar 20, 2023

hannobraun Mar 21, 2023

A-Walrus Mar 20, 2023

hannobraun Mar 21, 2023

hannobraun left a comment

hannobraun Mar 21, 2023

hannobraun Mar 21, 2023

hannobraun Mar 21, 2023

		/// Cache for global edges
		pub global_edge: BTreeMap<ObjectId, Handle<GlobalEdge>>,

Shell and Solid validation #1695

Shell and Solid validation #1695

Conversation

A-Walrus commented Mar 19, 2023 • edited by hannobraun Loading

hannobraun commented Mar 19, 2023

hannobraun commented Mar 20, 2023

hannobraun commented Mar 20, 2023

A-Walrus commented Mar 20, 2023

A-Walrus Mar 20, 2023

Choose a reason for hiding this comment

hannobraun Mar 21, 2023

Choose a reason for hiding this comment

A-Walrus Mar 20, 2023

Choose a reason for hiding this comment

hannobraun Mar 21, 2023

Choose a reason for hiding this comment

hannobraun left a comment

Choose a reason for hiding this comment

hannobraun Mar 21, 2023

Choose a reason for hiding this comment

hannobraun Mar 21, 2023

Choose a reason for hiding this comment

hannobraun Mar 21, 2023

Choose a reason for hiding this comment

A-Walrus commented Mar 19, 2023 •

edited by hannobraun

Loading