Redefine relationship between Sketch and Face

[hannobraun]

Shapes in the Fornjot kernel are made up of objects (faces, edges, vertices, ...). Those objects reference each other, and the resulting object graph defines the shapes. This object graph has been simplified a lot recently (#1589), to reduce redundancy and represent shapes in a way that is as correct-by-design as possible.

The topic of this issue is one object relationship that isn't satisfactory yet, the relationship between Sketch and Face. Sketch is a top-level object, meaning it isn't itself referenced and more-or-less represents a whole shape. Sketch references an arbitrary number of Faces which make up the Sketch. Face is also referenced by Shell, which itself is referenced by Solid, another top-level object.

Face works well in its role within the object graph of Solid/Shell. Each face is defined by the surface it's on (which could be different for any Face in the Shell), its exterior cycle, possibly interior cycles, and a color. But the same representation does not work well within a Sketch. The Faces of a Sketch being in different surfaces would be wrong, and actually, the surface doesn't need to be defined at all within the context of a sketch.

I've come up with the following possible solutions to this problem:

1. Add validation checks to make sure that all Faces in a Sketch are defined in the same surface. This would be problematic for the following reasons:
   • It uses validation to paper over the redundancy within the object graph. One theme of the recent cleanup has been to rather remove such redundancy, and I think the object graph has benefited a lot from that.
   • As noted above, Sketch doesn't need to be defined in a surface at all, and in fact I think it would be cleaner if it weren't. We only need a surface once we somehow apply this sketch to a 3D model (by sweeping it, for example).
2. Invert the relationship. Move all of Face's fields, except for surface, to Sketch, and reference Sketch from Face. This solution has problem too:
   • A Face needs exactly one exterior cycle, while a sketch could be made up of any number of cycles that are nested in arbitrary ways.
   • Such a 1:1 relationship between Face and Sketch doesn't meet the requirements we are going to face. At some point, we'll need the capability to draw a sketch, then apply it to a face, to extrude it for example. Applying a sketch to a face in this way would would be different from referencing the sketch from the face. This mismatch would at the very least be very awkward.
3. Break the relationship. Don't reference Face from Sketch, and instead reference Cycles there directly. How exactly those cycles are referenced, all in a single collection, for example, or using some structure that reflects their nesting, is an open question. In principle, I don't think you need much structure for the cycles within a Sketch, but how you do it influences how to later apply this sketch (or convert it) to a face.

I currently favor option 3., as it seems to have the fewest drawbacks. The details of the implementation would have to be figured out, however.

[A-Walrus]

As noted above, Sketch doesn't need to be defined in a surface at all, and in fact I think it would be cleaner if it weren't. We only need a surface once we somehow apply this sketch to a 3D model (by sweeping it, for example).

Would this mean adding a new type, which contains a Sketch + a Surface which we would use instead of Sketch in places like

impl Sweep for Handle<Sketch> {

[hannobraun]

A new type is probably overkill. This is possible:

impl Sweep for (Handle<Sketch>, &Surface) {

Other Sweep implementations are already doing this.

[A-Walrus]

I was thinking that maybe we could introduce another object type, Region which is like Face except without the surface. That way sketches would be a collection of Regions and a Face would just be a Region with a Surface. @hannobraun what do you think?
Obviously name is open to bike-shedding...

[hannobraun]

Yeah, that sounds workable! It is similar to solution 3 from the original issue description, using Cycles directly in Sketch.

The advantage of adding a new type would be less redundant code. The disadvantage would be, that we'd have another object type (which adds additional code in quite a few places). Which would be better, depends on how we eventually end up using Shape. Maybe the additional structure of having multiple Regions within Sketch would be advantageous, maybe the simplicity of having just a bunch of Cycles would be better.

I really can't say what would be better. But I'd happily merge either solution. We can always adapt, as the use cases become more clear.

Obviously name is open to bike-shedding...

I think "region" is pretty decent. Some alternative ideas:

• I was thinking maybe "figure", but it seems that can be used pretty interchangeably with "shape" (as in, there can be 2D and 3D figures). It would be a unique name within Fornjot, but probably not as clear for a newcomer.
• Another idea I had was "flat", in the sense of "an area of level ground".

After thinking about it for a bit, I like "region" best, out of those three. (But again, I'd merge whatever. Renaming things is easy enough.)

[A-Walrus]

I've been working on this and got it pretty much working aside from a little issue:
https://github.com/hannobraun/Fornjot/blob/a2579f6ee99793a9bdc12b499454dd8d9667fec1/crates/fj-operations/src/lib.rs#L53-L64
With the change of having sketch only contain the cycles (indirectly through Region), and not the surface, the Brep of a Shape2D can no longer be a FaceSet...
If we want the output to be a face set we can "apply" the sketch to some default surface (xy_plane), but that seems like the wrong solution?
Thoughts?

[hannobraun]

Thank you for looking into this, @A-Walrus!

I don't know what the answer is right now. What I can say, is that fj-operations is not exactly my masterpiece, it just grew as it needed to. So feel free to make any change you deem necessary. (I'll most likely have time to look at your pull request tomorrow, and maybe I'll come up with something then.)

If we want the output to be a face set we can "apply" the sketch to some default surface (xy_plane), but that seems like the wrong solution?

It would be a hack, but I think it would be fine as an initial solution. That would give us some breathing room while we figure out something better.

[hannobraun]

Addressed by @A-Walrus in #1828. Thank you!