KittyCAD / modeling-app

The KittyCAD modeling app.
https://kittycad.io/modeling-app/download
MIT License
412 stars 35 forks source link

Rework sketch mode to show all profiles and sketch geometry in AST that reference the current surface #2931

Open franknoirot opened 4 months ago

franknoirot commented 4 months ago

"Surface" here meaning either a plane or a 2D solid face.

In the pursuit of #1876, we have aligned on treating the sketch plane as a standalone constant, then using it to create separate top-level constants—profiles made of segments; circles; polygons, etc—that reference that sketch constant. This issue tracks the specific rework that needs done in our sketch mode to make meaningful those new top-level constants. Here are some requirements:

  1. Rework sketch mode to not assume that everything is in one pipe expression, but rather search for all sketch geometry that uses the current sketch surface
  2. Rework sceneEntities and sketch tools to not assume one path is being edited
  3. Rework sceneEntities and sketch tools to allow the determination of what segments should be displayed as draft lines
  4. Add a listener to line/arc tools to snap their start point to the end of a previously-made profile if the user clicks on it for their first point
  5. Rework line segment tools to create their own top-level constant when point-and-clicking
Irev-Dev commented 4 months ago

I was thinking should sketch details

export interface SketchDetails {
  sketchPathToNode: PathToNode
  zAxis: [number, number, number]
  yAxis: [number, number, number]
  origin: [number, number, number]
}

Change so that sketchPathToNode is many instead of one, or maybe it's like a start-pathToNode and end-PathToNode, but maybe it should stay the way it is and the pathToNode acts as entry point (because ultimately the user is going to click somewhere specific), and we have a astQuery that figures what else should be editable.

const plane = startSketchOn('XY')

const profile01 =  startProfileAt(...)
  |> line(...)

const profile02 =  startProfileAt(...)
  |> line(...).  //       <-------- entry point, sketchPathToNode

const yo = someEngCallNotSketchRelated(...)

const profile03 =  startProfileAt(...)
  |> line(...)

In the above our query explores, using the sketchPathToNode as the hook and it collects both profile01 and profile02, but knows to stop at yo.

Kinda of implied in my example above I think we need to change the code-gen to split startSketchOn out of the pipe.

Does that sound reasonable?

jtran commented 3 months ago

Feel free to ignore this if you already have a plan, but I like to think through it as an exercise.


Consider this. Take Kurt's example above, and put it in a function taking the plane like so.

const logo = (plane) => {
  const profile01 = plane
    |> startProfileAt(...)
    |> line(...)

  const profile02 = plane
    |> startProfileAt(...)
    |> line(...)

  const yo = someEngCallNotSketchRelated(...)

  const profile03 = plane
    |> startProfileAt(...)
    |> line(...)
}

logo(startSketchOn('XY'))
logo(startSketchOn('YZ'))

Now imagine something even more complicated where the calls to logo are in another function, inside a loop, etc.

search for all sketch geometry that uses the current sketch surface

If I were doing it, I feel like searching is a bad idea as KCL gets more complicated.

I would treat this as a runtime thing. I think I would modify executor.rs so that when it executes line(), arc(), etc., -- I'll call these entities -- it tracks what surface each entity is on. It needs the surface to make the engine command, right? The executor returns this info along with ProgramMemory in something called ProgramOutput.

Then in TS, you can do something like:

for (const entity of programOutput.entities) {
  const surface = entity.surface
  const pathToNode = buildPathToNode(entity.astNodeId)
  entitiesBySurface[surface].push(pathToNode)
}

// Later... find all entities on a surface.
const pathToNodes: PathToNode[] = entitiesBySurface[surface]
lf94 commented 3 months ago

What you describe is essentially what we're already doing, just articulated in a different way. When we execute the program, we collect "entities" into artifactMap. Creating the appropriate associations is possible with the current mechanisms, such as pathToNode, already in place. The AST traversing should not really be needed at all the more I look at the problem and the data available to me.

Thank you for articulating the general idea very clearly though for future reference and to others - this is definitely how we need to be crunching state!


Extra crap I wrote that I was going to just delete but figured it's nice to see into my head anyway:

The real tough bit for me is working within our current codebase and not do a massive swathe of changes... Eating time like this is currently not a luxury, especially for me where I have to frequently switch context from TypeScript to Rust to C++ and back so we can move forward on several fronts. After I finish an MVP of multi profile sketch, I would love to hand it off to you to clean up the situation further. :D

jessfraz commented 3 months ago

just a reminder to whoever closes this one, to close all the other linked ones, this is killing like 5 issues

lf94 commented 2 months ago

Reassigned @Irev-Dev , since they did the artifactGraph rework and that was a direct blocker to this and they are extremely familiar with it. I will move to doing linear and circular patterns