Sweep code is generating duplicate GlobalCurves

[hannobraun]

Problem

Sweeping a Face into a Shell includes sweeping all HalfEdges that bound the Face, to create the side faces:
https://github.com/hannobraun/Fornjot/blob/2b32926a3723bdf83745a58fdd84ca82a3e61559/crates/fj-kernel/src/algorithms/sweep/face.rs#L55-L65

Sweeping a HalfEdge in turn includes sweeping the Vertex instances that bound it, to create the side edges:
https://github.com/hannobraun/Fornjot/blob/2b32926a3723bdf83745a58fdd84ca82a3e61559/crates/fj-kernel/src/algorithms/sweep/edge.rs#L76-L79

Sweeping those Vertex instances includes sweeping the GlobalVertex that each Vertex references:
https://github.com/hannobraun/Fornjot/blob/2b32926a3723bdf83745a58fdd84ca82a3e61559/crates/fj-kernel/src/algorithms/sweep/vertex.rs#L56-L57

And sweeping a GlobalVertex creates a new GlobalCurve:
https://github.com/hannobraun/Fornjot/blob/2b32926a3723bdf83745a58fdd84ca82a3e61559/crates/fj-kernel/src/algorithms/sweep/vertex.rs#L132

This is all well, except that each side faces shares those GlobalEdges with its neighbors, but the GlobalCurves aren't shared. They are duplicated. As far as I know, this isn't causing any issues right now, but I'm currently working on code (part of #42) that can't work reliably, because those duplicated GlobalCurves lead to GlobalEdges that should be the same but aren't recognized as such.

In addition, I'm not sure if other parts of the sweep code exhibit the same problem (for example, sharing GlobalCurves properly between the side faces and the top/bottom faces).

Solution

At least the specific issue I've presented here should be relatively straight-forward to fix, but I'd rather make sure I'll fix all instances of this issue properly. This requires adding validation code to detect this. Specifically, I think we need the following:

• Detect any Curves that are coincident, but don't reference the same GlobalCurve.
• Detect any HalfEdges that share their Vertex instances and Curve, but don't reference the same GlobalEdge.

I think that should expose the problem I describe here.

Context

The additional validations would benefit from the scope of the centralized object storage being expanded (#1021). Specifically, if HalfEdges and all objects they reference (directly or indirectly) were managed in the centralized object storage, that would allow those problems to be found directly when the HalfEdges are created, and not on a later validation step. This would make it more likely that the problem is triggered in unit tests.

For that reason, this issue is not really but kinda soft-blocked on #1021.

I'm going to assign myself to this issue, as it is blocking further progress on #42, which is my current priority.

[hannobraun]

#1021 is now complete, and I'm now working on this issue. I've started with a cleanup of the validation code. It's not in the shape I'd like it to be, and I figured I'll fix that before piling more validation code on top of it (I think I've made enough of a mess recently).

I don't expect this to take very long, so unless the new validation code I'm going to write for this issue turns up more currently unknown problems, I have hope to wrap all of this up relatively quickly.

[hannobraun]

Short update: Cleaning up the validation code is taking longer than expected. There were design decisions that I had some trouble with, but those have been made now. I don't expect much more delay before I can start working on this in earnest, but we all know how much that is worth 😄

[hannobraun]

It's been a while since I wrote an update here! The validation code is all sorted now. It still caused more trouble than expected, mostly related to its test code and the partial object API, but that's done now. I'm now moving on to implementing the new validation checks that are required to expose this bug.

[hannobraun]

So, I forgot to update this issue in a long while. Sorry! Last time I posted here, I was starting to write the new validation checks required to expose this bug. Unfortunately this turned out more difficult expected, due to the sorry state the object construction code was in (#1249). This made writing unit tests for the new validation code very difficult.

I guess I could have circumvented the whole thing by just not writing any unit tests, but given that #1249 was such a pervasive problem that I would have kept encountering over and over, I decided that cutting corners wasn't worth it. I just needed to bite the bullet and fix the problem. Happy to report that is done now!

That doesn't mean all work on the object construction code is over. What we currently have is well under control now, but more object construction code needs to be written (for example said unit tests!) that will require more and better APIs to become practical. This is what I'm working on right now. Attempt writing more code that is required to address this issue, note the holes in the builder APIs that prevent me from actually writing that code, fill those holes, repeat.

[hannobraun]

As I alluded to in my previous update here, work on this issue currently consists mostly of expanding the builder API. The purpose of this expanded builder API is to construct geometry in the unit tests for the new validation checks (and it will be useful in many other places later). And the purpose of the new validation checks is to expose all instances of this bug.

I've been making progress on the builder API (see the various pull request above, which reference this issue). I'm not quite there yet, but it feels like that unit test is getting closer to being done. I don't expect this to be a fast process though, as this requires thinking and design work at every turn. There's no single big problem, and therefore no single big solution. Just more steps to be taken.

[hannobraun]

The actual bug has been addressed in #1593, but the validation code has not yet been written. I've opened #1594 as a follow-up to this issue, to track that.