Start Sketch frozen if selecting face from a revolve

[jgomez720]

Description

When I select the top face of this lug and use start sketch, the start sketch gets frozen. I think it's something along the lines of it understands that I want to sketch on the face, but doesn't know where to put the KCL since this geometry was created through a function call.

Recording

https://github.com/user-attachments/assets/56a67d64-3449-4c23-8fae-1407a58fc762

Version

v0.24.12

KCL

// Lug
// lugs are essential components used to create secure connections, whether for electrical purposes, like terminating wires or grounding, or for mechanical purposes, such as providing mounting points or reinforcing structural joints. 

// Define the plane the lug lives on
const customPlane = {

}

// Define constants
const lugHeadLength = 0.25
const lugDiameter = 0.5
const lugLength = 2

fn lug = (origin, length, diameter, plane) => {
  const lugSketch = startSketchOn(plane)
    |> startProfileAt([origin[0] + lugDiameter/2, origin[1]], %)
    |> angledLineOfYLength({
      angle: 60,
      length: lugHeadLength,
    }, %)
    |> xLineTo(0+.001, %)
    |> yLineTo(0, %)
    |> close(%)
    |> revolve({
      axis: "Y",
    }, %)

  return lugSketch
}

lug([0, 0], 10, .5, "XY")

[jgomez720]

I realized that this isn't with functions, it's the revolve that it doesn't understand.

[Irev-Dev]

Thanks Josh!

I don't think I want to throw you in the deep end with this one, but maybe we pair on it @nadr0.

[nadr0]

I provided a fix here nadro/3492/start-sketch-freezing-from-revolve which is a one line change to support revolve as extrude within the ArtifactGraph. This is a hack for now.

The line of code: https://github.com/KittyCAD/modeling-app/commit/ef1b575f6f00098e8c5f2c0dbb85cbfbfb4fb702

@Irev-Dev Should we be supporting Revolve's as RevolveArtifact and RevolveArtifactRich? If we do not want to separate them we may want a more generalize term to identify these operations. Ben said extrude, revolve, and sweep are all similar and will use the response of solid3d_get_extrusion_face_info to build the artifact graph.

I found a bug with Josh's function code with revolve + sketch on face. I'll be making a new github issue to document the problem.

If you use my branch to test the fix and use Josh's code you will realize that the sketch on face workflow he wanted to do is not supported, here is an issue describing it https://github.com/KittyCAD/modeling-app/issues/3771

Here is the engine issue I reported as well after testing Josh's workflow.

[Irev-Dev]

Generalising this sounds good, i.e. having one artifact type that covers both. I've just asked about naming in the engine, it's best to match up with naming where possible. I think we should try generalise and if later have to break it up, so be it, but doubt that will be the case. Lets say the name ends up being "bar", we can just add a subtype which describes the specific type, but I think all of the rest of the properties hold up, hence why the hack works, which means it's not a hack it's the fix just with some renaming yeah?

interface BarArtifact {
  type: 'bar'
  subType: 'extrusion' | 'revolve'
  pathId: string
  surfaceIds: Array<string>
  edgeIds: Array<string>
  codeRef: CommonCommandProperties
}

[Irev-Dev]

Had that comment ☝️ sitting there for hours without hitting the button 🤦

The response I got back from Mike was that sweep is the most general term so we could go with SweepArtifact but I don't like the idea that sweep is the generalisation and a specific operation.

Two alternatives that I think are okay are prism and contour (profile might work but we already use that in relation to sketches)

I think contour is probably the best, what do you think?

/** Countour is our general term that covers Extrudes, Revolves and Sweeps */
interface ContourArtifact {
  type: 'contour'
  subType: 'extrusion' | 'revolve' | 'sweep'
  // ...

[nadr0]

Thanks for the clarification. I agree that it is a naming problem.

Do you think that we will ever have contour lines in the software?

I like SweepArtifact, I know it is a general and specific term but I think it can be reasoned about with the curve it makes. Extrude is a shorthand for a linear sweep? Revolve is a shorthand for arc sweep?

Out of prism, contour, profile, and sweep, I'd rather name it to the closet thing and then change my mental model of a sweep. I think the collision of sweep being the general and specific term is the specific's term fault. With this mental model an extrusion isn't a real thing, it is a sweep that happens to be on a liner path. We shorthand this operation instead of making them pick sweep, pick the curve (a linear curve) then do the operation.

# My suggestion
interface SweepArtifact {
  type: 'sweep',
  subType: 'extrusion' | 'revolve' | 'sweep' 
}

# Ehh??? 
interface SweepArtifact {
  type: 'sweep',
  subType: 'linear' | 'arc' | 'any'  // path? custom path? any doesn't seem good but you get the idea. 
}

[Irev-Dev]

Cool, happy with sweep too :) 🧹