SketchGroup should allow multiple profiles

[Irev-Dev]

We want to support having multiple profiles in the one sketch, (outer and inner loop).
The kcl code would be as simple as having more than one startProfileAt.

const part001 = startSketchOn('YZ')
  |> startProfileAt([32.21, 30.83], %)
  |> line([-3.16, 83], %)
  |> line([-3.16, 83], %)
  |> close(%)
  |> startProfileAt([32.21, 30.83], %)
  |> line([-3.16, 83], %)
  |> line([-3.16, 83], %)
  |> close(%)

But on top of that we also want to support "construction geometry", the concept here is that it's 2d geometry that you can reference in your sketch, but is not necessarily part of your profile for example.

const part001 = startSketchOn('YZ')
  |> constructionCircle([0,5], 3, %, 'myCirc')
  |> angledLineThatIntersects({
       angle: 201,
       offset: -1.35,
       intersectTag: 'myCirc'
     }, %)
// ...

Example code aside we'll need to change the sketch group struct, something like this would work.

// a sketch groups values, multiple profiles or construction geometry
#[derive(Debug, Clone, Deserialize, Serialize, PartialEq, ts_rs::TS, JsonSchema)]
#[ts(export)]
#[serde(tag = "type", rename_all = "camelCase")]
pub enum SketchGroupValues {
    /// a single profile Vec<Path>
    Profile(Vec<Path>),
    /// Construction geometry
    Construction(Path),
}

/// A sketch group is a collection of paths.
#[derive(Debug, Clone, Deserialize, Serialize, PartialEq, ts_rs::TS, JsonSchema)]
#[ts(export)]
#[serde(tag = "type", rename_all = "camelCase")]
pub struct SketchGroup {
    /// The id of the sketch group.
    pub id: uuid::Uuid,
    /// The paths in the sketch group.
    pub value: Vec<SketchGroupValues>,
    /// What the sketch is on (can be a plane or a face).
    pub on: SketchSurface,
    /// The starting path.
    pub start: BasePath,
    /// The position of the sketch group.
    pub position: Position,
    /// The rotation of the sketch group base plane.
    pub rotation: Rotation,
    /// The x-axis of the sketch group base plane in the 3D space
    pub x_axis: Point3d,
    /// The y-axis of the sketch group base plane in the 3D space
    pub y_axis: Point3d,
    /// The z-axis of the sketch group base plane in the 3D space
    pub z_axis: Point3d,
    /// The plane id or face id of the sketch group.
    pub entity_id: Option<uuid::Uuid>,
    /// Metadata.
    #[serde(rename = "__meta")]
    pub meta: Vec<Metadata>,
}

[franknoirot]

Since a user might want any sketch/2D geometry to be construction geometry, would it make sense to have it as a construction() function that can take any sketch geometry, whether a circle() or a startProfile()...close()? So your example would be:

const sketch001 = startSketchOn('YZ')
  |> construction(circle([0,5], 3, %, 'myCirc'), %)
  |> angledLineThatIntersects({
       angle: 201,Ï
       offset: -1.35,
       intersectTag: 'myCirc'
     }, %)
// ...

Or would that cause other problems? I'm seeing if there's a way to avoid making construction... versions of every stdlib function

[franknoirot]

Another thought: if we're going to support multiple profiles it might help readability to rename close to something that mirrors startProfileAt, like closeProfile or endProfile

[franknoirot]

@jessfraz I didn't realize @Irev-Dev already had this all written up; this is the issue I mentioned to you on Tuesday

[jessfraz]

related to #1553 #1554

[adamchalmers]

So the ask is:

• SketchGroups currently only have one profile. A profile is a path, which is a collection of contiguous path segments.
• We want SketchGroups to support multiple profiles.
• After a SketchGroup is closed via |> close(%), users can add another profile via |> startProfileAt() or similar

I don't really know what you want for construction geometry yet.

• Is each profile entirely real XOR construction? Or can a single profile have some segments that are real and some that are construction?
• Is construction geometry actually sent to the engine, or is it purely for doing calculations client-side?

So I think construction geometry should be out of scope for this, let's do it in #1553 instead.

[adamchalmers]

@franknoirot can profiles within a sketch group be extruded independently? Or are they all extruded/rotated at once, with the same parameters?

[franknoirot]

@franknoirot can profiles within a sketch group be extruded independently? Or are they all extruded/rotated at once, with the same parameters?

@adamchalmers they should be able to be extruded independently if we are to match behavior with existing CAD programs. Sketches are a collection of profiles, some of which are closed and therefor eligible for use in 3D geometry operations like Extrude and Sweep. Here's an example of this behavior from OnShape:

Screenshare.-.2024-05-29.2_55_08.PM-compressed.mp4

[adamchalmers]

OK, so clarifying question: in what way are the profiles within a SketchGroup related? Why not just make two separate SketchGroups?

[franknoirot]

The profiles in a SketchGroup are related by dimensions and constraints between the profiles and their constituent elements.

Doing a number of sketch operations become significantly more tedious if each closed profile is its own sketch:

1. Dimensions and other constraints between those profiles and their constituent elements would require use of projected geometry which is not as secure a relationship
2. It muddies the top-level entities with many sketches, when a sketch would conceptually group these entities as one. In this tutorial for example it would be frustrating to users to require separate sketches for the inner and outer profiles of a simple washer/donut shape.

[Irev-Dev]

I agree with Franks points.

Construction geometry doesn't need to be sent to the engine.

I'm not sure about the independently extrude thing, I think it might be a good to check with @jgomez720.

Not extruded independently

• Completely separate profiles are extruded together
• Cases where profiles are 100% contained within another profile this becomes negative space (and then positive again if there's more nesting and so on)
• Cases where there's an intersection between two profiles, it's considered an error and the extrusion fails (we give the error to the user). If you have common things between two profiles where a user might be tempted to have intersection profiles, that's what the construction geometry is for.

Extruded independently

• Each profile needs to be selected and extruded separately. in which case we'll need to add tags to the startProfileAt
• What does the UI look like for wanting an inner profile to be subtracted from an outer one when extruded? Does that automatically happen when you select them both?
• What do we do with intersecting profiles, maybe we should still error because users probably think they'll be unioned, but the engine doesn't support that atm?

[jgomez720]

Profiles should be able to be extruded independently. When that happens, typically the designer created a profile as a reference (really similar to the purpose of construction geometry).

This is a complex example, but gears are really hard without construction geometry.
https://www.youtube.com/watch?v=dxtq_sy7Gw0

Even if you skim through the video, you'll see how often he creates a profile to reference off of, and then deletes it afterward (deleting a construction geometry after using it is a bad practice)

[adamchalmers]

I feel like if you draw two profiles A and B, and A is inside B, and you want them subtracted from each other, then the frontend code generation should generate KCL that uses the hole function to make B a hole of A.

This way you wouldn't need a rule like "generally, extrude profiles independently, but if one encloses the other then treat it as a subtraction". You could have the simpler "always extrude profiles independently" (though some profiles will have holes).

[franknoirot]

Just one small point @Irev-Dev:

Cases where profiles are 100% contained within another profile this becomes negative space (and then positive again if there's more nesting and so on)

Onshape doesn't alternate nested profiles as extrude/void; only the outer-most is extruded and the user opts into any nested profiles by clicking.

Screenshare.-.2024-06-13.8_26_02.PM.mp4

[franknoirot]

I really like that idea @adamchalmers. I think using those profiles in that generated code with hole would still require us to tag profiles, right? Then a user could create a sketch with multiple profiles, some nested within each other, and use different parts of the sketch across multiple extrusions, some with holes and some not. Like this example where I extrude in two directions using the same sketch. The first extrude takes the innermost profile and outmost two profiles, and the second extrusion goes in the opposite direct and just extrudes the space between the second and third outermost profiles:

[jessfraz]

Frank and I talked about this a bit in length and honestly I think the multi-profile code the way it is is extrememly hard to read and not user friendly.

Instead if you look in on shape when you edit a "sketch" the shared commonality for that sketch is the "plane" so we should allow anyone to edit any sketchgroup on that same plane versus our one sketchgroup per edit we have now. I feel like this is a problem with the constraint we put on ourselves for one sketch per edit/create mode versus being a code issue.

ill show what im talking about.

Screenshare.-.2024-06-21.1_52_59.PM.mp4

I will try and take a look at where that forced requirement of one sketchgroup per edit/create mode is but if you've got ideas @franknoirot or @Irev-Dev let me know but i def think this is a better approach

[jgomez720]

@Irev-Dev's original idea is still what makes the most sense to me. Even if two sketches share the same plane, that does not mean they are used for similar processes

Screenshare.-.2024-06-21.3_15_55.PM.mp4

This really only touches on the multiple profiles part. Nothing about the construction geometry stuff.

[jessfraz]

I promise i think we want the same things here just I feel strongly the code as an artifact of the first idea is completely unreadable and maintainable and will get out of hand quickly so we need some sort of middle ground here, but like if we do this code it will deflect MEs from learning to code.

But i think we are saying the same thing, let me show you, the semantic is just can I edit it in here, and can I not.

They would not be combined sorry let me make this more clear. But i think we are saying the same thing.

Screenshare.-.2024-06-21.3_35_14.PM.mp4

[jessfraz]

the main difference between what you are doing and what i am doing is youd be able to edit all three of those sketches on the same plane at the same time versus being constrained to the ones you explicitly put in a sketch which I dont think should matter it kinda seems like a feature but maybe im wrong

[jgomez720]

Okay, I see what you are saying. I am fine with the idea, but I do have one more artifact in my devils advocate toy box. I'll show in video one sec

[jessfraz]

and this doesnt block the extruding at the same time it would just be like

const plane001 = startSketchOn('XZ')

const profile001 = startSketchOn('XZ')
  |> startProfileAt([40.82, 240.82], %)
  |> line([235.72, -8.16], %)
  |> line([13.27, -253.07], %)
  |> line([-247.97, -19.39], %)
  |> lineTo([profileStartX(%), profileStartY(%)], %)
  |> close(%)

const profile002 = plane001
  |> startProfileAt([47.17, -71.91], %)
  |> line([247.96, -4.03], %)
  |> line([-17.26, -116.79], %)
  |> line([-235.87, 12.66], %)
  |> lineTo([profileStartX(%), profileStartY(%)], %)
  |> close(%)

extrude(10, [profile001,profile002])

[jessfraz]

i am glad! sorry im also only getting heated not because im like mad just becasuse im into finding a better solution here haha

and im hoping the you can edit them all at the same time is a win versus a blocker

[jessfraz]

also now i really like the idea of this

const plane001 = startSketchOn('XZ')

const profile001 = startSketchOn('XZ')
  |> startProfileAt([40.82, 240.82], %)
  |> line([235.72, -8.16], %)
  |> line([13.27, -253.07], %)
  |> line([-247.97, -19.39], %)
  |> lineTo([profileStartX(%), profileStartY(%)], %)
  |> close(%)

const profile002 = plane001
  |> startProfileAt([47.17, -71.91], %)
  |> line([247.96, -4.03], %)
  |> line([-17.26, -116.79], %)
  |> line([-235.87, 12.66], %)
  |> lineTo([profileStartX(%), profileStartY(%)], %)
  |> close(%)

const sketch=[profile001,profile002]

extrude(10, sketch)

where then the sketch is a vec of profiles, i like that!

for some reason this code doesn't work today but I can fix it , in thoery it should very weird

[jgomez720]

Screenshare.-.2024-06-21.3_51_54.PM.mp4

[jessfraz]

Ok wait i gotchu i gotchu 1 sec

[jessfraz]

also this is fun you likely think im insane but i like this hahah in general the discussion ahaha

Screenshare.-.2024-06-21.3_59_23.PM.mp4

[jessfraz]

also we could call it a surface for sketch on face too:

const surface001 = startSketchOn('XZ')

const profile001 = surface001
  |> startProfileAt([40.82, 240.82], %)
  |> line([235.72, -8.16], %)
  |> line([13.27, -253.07], %)
  |> line([-247.97, -19.39], %)
  |> lineTo([profileStartX(%), profileStartY(%)], %)
  |> close(%)

const profile002 = surface001
  |> startProfileAt([47.17, -71.91], %)
  |> line([247.96, -4.03], %)
  |> line([-17.26, -116.79], %)
  |> line([-235.87, 12.66], %)
  |> lineTo([profileStartX(%), profileStartY(%)], %)
  |> close(%)

const sketch=[profile001,profile002]

extrude(10, sketch)

so then its clear shared surfaces can be added or edited together, so if you sketch on a face the same goes

[jessfraz]

we could also do something cool and fade out all the code from other "surfaces" so you can see it but its lighter when in edit mode so its clear you just changing code for this surface

[jessfraz]

so then if i wanted to do a cut out it would generate like:

const surface001 = startSketchOn('XZ')

const profile001 = surface001
  |> startProfileAt([40.82, 240.82], %)
  |> line([235.72, -8.16], %)
  |> line([13.27, -253.07], %)
  |> line([-247.97, -19.39], %)
  |> lineTo([profileStartX(%), profileStartY(%)], %)
  |> close(%)

const profile002 = surface001
  |> startProfileAt([47.17, -71.91], %)
  |> line([247.96, -4.03], %)
  |> line([-17.26, -116.79], %)
  |> line([-235.87, 12.66], %)
  |> lineTo([profileStartX(%), profileStartY(%)], %)
  |> close(%)

const sketch=profile001 
 |> hole(profile002)

[Irev-Dev]

I'm on board,

From the get-go, I preferred these proposal's API, and in fact the only reason I wanted to pipe forever with sketches is because of what I thought were pragmatic reasons around re-executing to keep code as the source of truth and the fact that in sketch mode we also make a new mini-ast for perf reasons so that when we re-execute it's not having to do all the geometry again. I'm brushing over it, but I had a quick chat with Jess to sanity check my thoughts.

The main case I was worried about was something like

const plane = startSketchOn('XZ')

const profile = startProfileAt([1.01, 0.93], plane)
  |> line([1.63, 1.25], %)
  |> line([2.69, -0.6], %)
  |> line([-0.01, -2.85], %)
  |> lineTo([profileStartX(%), profileStartY(%)], %)
  |> close(%)
const profile2 = startProfileAt([6, 0.93], plane)
  |> line([2.14, 1.77], %, 'seg01')
  |> line([2.69, -0.6], %)
  |> angledLine([
       -90,
       segLen('seg01', %) * segLen('seg01', %) / 2
     ], %)
  |> lineTo([profileStartX(%), profileStartY(%)], %)
  |> close(%)

const extrude001 = extrude(5, profile2)

const profile3 = startProfileAt([2, 3], plane)
  |> line([1.63, 5], %)
  |> line([4.85, -3.14], %)
  |> line([4.85, getExtrudeVolume(extrude001)], %)

emphasis on getExtrudeVolume. In this case we can not allow editing profile1, profile2 and profile3 at the same time because changing profile2 effects something in 3d which profile3 relies on.
This is an edge case so not to be too concerned about but the compromise Jess and I came up with is that
a) Our AST modes will never put something 3d between two 2d profiles using the same plane
b) When a user does it, we can have a lint warning for it
c) In the case of the example where there's a legitimate reason to have 3d between 2 profile definitions, then we simply block editing both at the same time, user has to edit profile1 and profile2 together, and profile3 separately.

One other final thing I'll note is that this is our language and we can do whatever we want with it, and infact originally in the language before we swapped it out for plain old pipes we had something like something like

const mySketch = sketch {
  const profile001 = surface001
    |> startProfileAt([40.82, 240.82], %)
    |> line([235.72, -8.16], %)
    |> line([13.27, -253.07], %)
    |> line([-247.97, -19.39], %)
    |> lineTo([profileStartX(%), profileStartY(%)], %)
    |> close(%)
  
  const profile002 = surface001
    |> startProfileAt([47.17, -71.91], %)
    |> line([247.96, -4.03], %)
    |> line([-17.26, -116.79], %)
    |> line([-235.87, 12.66], %)
    |> lineTo([profileStartX(%), profileStartY(%)], %)
    |> close(%)
}

That is a keyword sketch with curlies {}. Sketches having a language level feature like this might be pragmatic because

• We can block 3d stuff between profiles at a language level instead of a linting/best-practise, i.e. stdLib functions like extrude, loft etc would be blocked inside of a sketch block.
• The mental model for what's editable in sketch mode at the same time is very easy to grok and very easy from a AST level, because it's just everything within the sketch block, the same way as it's now "everything with in the pipe"

That would be a bigger change, but something to consider.

[jgomez720]

There's going to be some confusion with the three-level approach from those that come from traditional CAD. Users are used to sketch -> profiles. Not sketch -> surface -> profile.

However, none of those programs are code as source of truth, so I'm on board if it means make the code easier to understand. I've been trying to think of ways to break it, but either it's too early for my brain or I can't think of anything.

This is really clear to me @Irev-Dev. It won't be clear for users until they start using the sketch tool more. As the code generates while they create multiple profiles, they'll get it, but expect a learning curve at first if we go this route

const mySketch = sketch {
  const profile001 = surface001
    |> startProfileAt([40.82, 240.82], %)
    |> line([235.72, -8.16], %)
    |> line([13.27, -253.07], %)
    |> line([-247.97, -19.39], %)
    |> lineTo([profileStartX(%), profileStartY(%)], %)
    |> close(%)
  
  const profile002 = surface001
    |> startProfileAt([47.17, -71.91], %)
    |> line([247.96, -4.03], %)
    |> line([-17.26, -116.79], %)
    |> line([-235.87, 12.66], %)
    |> lineTo([profileStartX(%), profileStartY(%)], %)
    |> close(%)
}

[franknoirot]

we could also do something cool and fade out all the code from other "surfaces" so you can see it but its lighter when in edit mode so its clear you just changing code for this surface

Hey hey I have a little draft mockup of something related to this here in Figma (obviously not including the code tweaks discussed here which I like):

[franknoirot]

const sketch=profile001 
|> hole(profile002)

I think a sketch would be a vec of profiles, just a thin group, but where the piping with hole() becomes useful is when extruding a region that is made up of the space between two profiles, like in this Figma flow I just made So if I had it this sketch with two profiles:

And the user selects the region between them...

The resulting extrusion would look something like this:

const surface001 = startSketchOn(‘XY’)

const profile001 = surface001
  |> rectangleFromCenter({
      center: [0, 0],
      width: 20,
      height: 10,
    })
const profile002 = surface001
  |> circle([0, 0], 5)

const sketch001 = [
  profile001,
  profile002
]

const extrude001 = extrude(
  22.5,
  profile001 |> hole(profile002)
)

or if we wanted to show the user that we were making a region more clearly...

const surface001 = startSketchOn(‘XY’)

const profile001 = surface001
  |> rectangleFromCenter({
      center: [0, 0],
      width: 20,
      height: 10,
    })
const profile002 = surface001
  |> circle([0, 0], 5)

const sketch001 = [
  profile001,
  profile002
]

const sketchRegion001 = profile001 |> hole(profile002)
const extrude001 = extrude(
  22.5,
  sketchRegion001
)

[franknoirot]

A video walking through this flow:

1. I think we shouldn't convert closed sketch profiles into Solid2Ds
2. The way this discussion plugs into a concept of a "sketch region" for use in extrusions
3. Showing the user the code mod-in-progress while extruding
4. A question about how to discern when the user is trying to extrude the region between two sketch profiles
5. I like this idea of the surface being the thing you go back and edit in sketch mode

Screenshare.-.2024-06-24.6_17_37.PM-compressed.mp4

[franknoirot]

I should also note that I found a CodeMirror extension to make certain ranges read-only, so once we make a sketch surface its own top-level constant we can just make that line read-only with it.

[jessfraz]

Throw that on the thread with Marijn so we can make sure it's good :) he might suggest implementing our own

[franknoirot]

Marijn recommended against that extension in favor of making our own change filter, which is a direct CodeMirror utility. But that'd be better as its own issue.

[jessfraz]

relevant to #2931