Rework Sketches #1801

A-Walrus · 2023-04-27T13:12:37Z

Closes #1691.
Currently it is in a semi-working state. Tests are passing, and example models work. There are a couple things I'm not sure about so would love some feedback, see comments below

A-Walrus · 2023-04-27T13:13:52Z

crates/fj-kernel/src/algorithms/approx/sketch.rs

-        tolerance: impl Into<Tolerance>,
-        cache: &mut Self::Cache,
+        _tolerance: impl Into<Tolerance>,
+        _cache: &mut Self::Cache,
    ) -> Self::Approximation {
-        self.faces().approx_with_cache(tolerance, cache)
+        todo!()


I don't really know what this code is supposed to do...

Within the kernel, objects are represented using boundary representation (b-rep). That's what all the object types (Solid, Shell, Face, ...) are. The purpose of boundary representation is to provide an accurate representation (in which, for example, a circle is actually a circle) that is also suitable for various operations to be performed on it (eventually, we want the user to be able to select an edge and put a chamfer there, for example).

However, boundary representation is not suitable for all purposes. Namely, our rendering code is based on triangle meshes, and there are also external file formats (3MF and STL), which are also based on triangle meshes.

To turn boundary representation into a triangle mesh, we use a two-step process:

Approximation, which turns b-rep into a simpler approximation, which is still accurate to a given tolerance. For example, a circle would become a polygon (a very high tolerance value would result in a triangle; the lower the tolerance value, the more edges the polygon will have).

Triangulation, which takes this approximation and turns it into a triangle mesh.

If you know a good place (or multiple good places) in the code where this explanation could be put as a comment (or multiple), feel free to do so 😄

So, what is that code supposed to do? It's supposed to approximate the sketch in a way that is useful to the triangulation. That means we're going to need a suitable implementation of Approx for Region (which I suspect is going to be very similar to that of Face), and then Sketch's Approx implementation can be adapted accordingly.

There's an open question though: Make the approximation 2D or 3D? Maybe it makes most sense to keep the approximation 2D (that's the most correct implementation for the new Sketch/Region, I think), then triangulation can convert that into 3D, using a default plane (x-y plane is going to be best, probably).

We might need something different here at some point, once we start supporting export to 2D-specific file format (see #798, for example). I'm not sure how that's going to work though, and for all I know, that might bypass approximation/triangulation. So let's not worry too much, and just do something that works for now.

A-Walrus · 2023-04-27T13:14:52Z

crates/fj-kernel/src/algorithms/transform/sketch.rs

 impl TransformObject for Sketch {
    fn transform_with_cache(
        self,
-        transform: &Transform,
-        services: &mut Services,
-        cache: &mut TransformCache,
+        _transform: &Transform,
+        _services: &mut Services,
+        _cache: &mut TransformCache,
    ) -> Self {
-        let faces =
-            self.faces().into_iter().cloned().map(|face| {
-                face.transform_with_cache(transform, services, cache)
-            });
-
-        Self::new(faces)
+        todo!()


Is this even possible now that sketch is entirely 2d? We can't apply a 3D transform to a 2D sketch?

Very good point!

For now, do we need the TransformObject operation for Sketch? I'd be fine with just removing that implementation, and also removing anything from fj-operations/fj that relies on it.

Eventually, we need real 2D transformations (#944), but I'm fine with removing the capability for transform sketches in the meantime, if it means we end up with cleaner code that is going to serve us better going forward.

I removed TransformObject for Sketch, without touching fj-operations/fj and everything seems to work fine...
Is there anything specific I should check/change?

Oh yeah, that's because none of the Shape::Brep associated type are actually set to Sketch. Since you decided to convert it to 3D using a default plane, no changes will be required.

A-Walrus · 2023-04-27T13:16:39Z

crates/fj-operations/src/lib.rs

 impl Shape for fj::Shape {
-    type Brep = FaceSet;
+    type Brep = Set<Face>;

    fn compute_brep(
        &self,
        services: &mut Services,
        debug_info: &mut DebugInfo,
    ) -> Self::Brep {
        match self {
-            Self::Shape2d(shape) => {
-                shape.compute_brep(services, debug_info).faces().clone()
+            Self::Shape2d(_shape) => {
+                todo!()


Now that sketch is completely 2D, the Brep of a Shape2d can't be a FaceSet (now Set<Face>)

Yeah, we need to be smarter here. Maybe have a type Brep that's something like this:

pub enum Brep { 2d(Sketch), 3d(Solid), }

But I'm not sure. It's probably worth taking a step back, look at how Shape::Brep is actually used, and then decide what's appropriate.

In the meantime, I'm fine with converting to Set<Face> using the x-y plane. That should work, until we come up with something better.

Allowing the Brep to be a sketch, we would run into a problem where in:
https://github.com/hannobraun/Fornjot/blob/a2579f6ee99793a9bdc12b499454dd8d9667fec1/crates/fj-operations/src/transform.rs#L10-L20
We need to be able to transform the Brep which we can't do to a Sketch.
I think for now I'll implement it using the xy-plane 👍

A-Walrus · 2023-04-27T13:18:07Z

Also I turned FaceSet into a generic Set<Face>, so that Sketch can contain Set<Region>

hannobraun · 2023-04-27T15:20:26Z

Thank you, @A-Walrus! I'll try to take a look tomorrow.

hannobraun · 2023-04-28T14:14:20Z

I haven't had a chance to do a full review yet, but I've answered the questions. Please let me know, if there's anything else that's not clear!

A-Walrus · 2023-04-28T19:16:48Z

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

-    exterior: Handle<Cycle>,
-    interiors: Vec<Handle<Cycle>>,
-    color: Option<Color>,
+    region: Region,


Should this be a Region or Handle<Region>? Same question about how Sketch references Region through a Handle or not?

That question is basically the same as asking, is Region an object (i.e. stored in centralized storage and referred to via Handle) or is it just a normal struct. It should be an object, if it needs an identity (documented here), which can be relevant for validation (see #1689, for example).

The the answer will probably become obvious at some point. For now, I don't know, but I can't think of a reason why it needs to be an object. I think that means it would be best to make it a normal struct, as that's less complex. If it needs to become an object at some point, we can make that happen then.

If it's not an object, that means it should probably live in geometry, rather than within the objects module.

Good catch, by the way! I hadn't thought about this before, and had just assumed that it needs to be an object.

hannobraun · 2023-05-15T10:41:59Z

Hey, @A-Walrus! I've recently decided to reduce the scope of this project (see A New Direction for context). As a result, parts of the code changed here have been removed. Namely, the fj-operations crate is no longer part of this repository.

As far as I'm aware, all open questions have been addressed, so I guess aside from a rebase to fix the merge conflixts, this should be ready for review?

Currently Set itself is still defined in `objects/full/face.rs` didn't know where to move it...

Sketch no longer implements `TransformObject` since it is a 2D object which cannot be moved in 3D.

hannobraun · 2023-05-16T08:29:33Z

Superseded by #1828. Closing.

A-Walrus commented Apr 27, 2023

View reviewed changes

A-Walrus commented Apr 28, 2023

View reviewed changes

A-Walrus added 5 commits May 15, 2023 18:43

Turn FaceSet into generic Set<Face>

fd2f1c6

Refactor transform Set out of Face

95ab5b2

Currently Set itself is still defined in `objects/full/face.rs` didn't know where to move it...

Move Set into objects/full/set.rs

2e5dfed

Switch to Region based approach

9fdabd0

Remove transform/sketch.rs

d93249e

Sketch no longer implements `TransformObject` since it is a 2D object which cannot be moved in 3D.

A-Walrus force-pushed the sketch_face branch from 85daf0f to d93249e Compare May 15, 2023 15:45

A-Walrus mentioned this pull request May 15, 2023

Rework Sketches v2 #1828

Merged

hannobraun closed this May 16, 2023

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework Sketches #1801

Rework Sketches #1801

A-Walrus commented Apr 27, 2023

A-Walrus Apr 27, 2023

hannobraun Apr 28, 2023

A-Walrus Apr 27, 2023

hannobraun Apr 28, 2023

A-Walrus Apr 28, 2023

hannobraun Apr 29, 2023

A-Walrus Apr 27, 2023 •

edited

Loading

hannobraun Apr 28, 2023

A-Walrus Apr 28, 2023

A-Walrus commented Apr 27, 2023

hannobraun commented Apr 27, 2023

hannobraun commented Apr 28, 2023

A-Walrus Apr 28, 2023 •

edited

Loading

hannobraun Apr 29, 2023

hannobraun commented May 15, 2023

hannobraun commented May 16, 2023

Rework Sketches #1801

Rework Sketches #1801

Conversation

A-Walrus commented Apr 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

A-Walrus Apr 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

A-Walrus commented Apr 27, 2023

hannobraun commented Apr 27, 2023

hannobraun commented Apr 28, 2023

A-Walrus Apr 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hannobraun commented May 15, 2023

hannobraun commented May 16, 2023

A-Walrus Apr 27, 2023 •

edited

Loading

A-Walrus Apr 28, 2023 •

edited

Loading