KittyCAD / modeling-app

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

Allow tags to be referenced on geometry with dot notation #2929

Closed franknoirot closed 4 months ago

franknoirot commented 4 months ago

There is currently no way to refer to tags defined within functions, which means that you can't—for example—fillet an edge of a part defined in a function. We should allow the tags defined in a pipe expression be referenced later in the program using dot notation on the named constant that references the pipe expression. This pairs well with #2928 to give us big strides towards "Components as KCL functions".

This KCL code doesn't work today, but should eventually:

fn triangle = (origin) => {
  return startSketchOn('XZ')
  |> startProfileAt(origin, %)
  |> line([60.53, 96.64], %, $A)
  |> line([45.66, -93.45], %)
  |> lineTo([profileStartX(%), profileStartY(%)], %)
  |> close(%)
}

const myTriangle = triangle([3.19, 0.53])

// This doesn't work today
const myTaggedEdge = myTriangle.A

Here is a longer example comparing the direct definition and function definition approaches. I think both should work.

// Function version
fn rect = (origin) => {
  return startSketchOn('XZ')
  |> startProfileAt(origin, %)
  |> angledLine([0, 191.26], %, $rectangleSegmentA001)
  |> angledLine([
       segAng(rectangleSegmentA001, %) - 90,
       196.99
     ], %, $rectangleSegmentB001)
  |> angledLine([
       segAng(rectangleSegmentA001, %),
       -segLen(rectangleSegmentA001, %)
     ], %, $rectangleSegmentC001)
  |> lineTo([profileStartX(%), profileStartY(%)], %)
  |> close(%)
}
const rectangleFromFunction = rect([0, 0])

// Direct version
const rectangleDirect= startSketchOn('XZ')
  |> startProfileAt([200, 0], %)
  |> angledLine([0, 191.26], %, $rectangleSegmentA002)
  |> angledLine([
       segAng(rectangleSegmentA002, %) - 90,
       196.99
     ], %, $rectangleSegmentB002)
  |> angledLine([
       segAng(rectangleSegmentA002, %),
       -segLen(rectangleSegmentA002, %)
     ], %, $rectangleSegmentC002)
  |> lineTo([profileStartX(%), profileStartY(%)], %)
  |> close(%)

// Extrusions
const extrusionFromFunction = extrude(40, rectangleFromFunction)
const extrusionDirect = extrude(40, rectangleDirect)

// Fillets
fillet({
  radius: 40,
  tags: [getNextAdjacentEdge(rectangleSegmentA002, extrusionDirect)]
}, extrusionDirect)

// This code won't work today: referencing the tags within a sketch using dot notation
fillet({
  radius: 40,
  tags: [
    getNextAdjacentEdge(
      rectangleFromFunction.rectangleSegmentA001,
      extrusionFromFunction
    )
  ]
}, extrusionFromFunction)
lf94 commented 4 months ago

Please read https://kittycadworkspace.slack.com/archives/C04KFV6NKL0/p1720192343486969 before executing on this one :)

jtran commented 4 months ago

In the Slack thread, the conversation went to defining reusable components. But I think this feature is in some ways counter to making reusable components because without additional knobs, allowing referencing geometry using dot notation exposes the internal implementation. This is problematic because later, when you change the implementation, you need to worry about not breaking callers who may reference all the tags that you exposed. For backward compatibility, you have to assume Hyrum's Law, that anything exposed will be depended upon.

For this reason, I think that there needs to be a way to explicitly control the output of a tag. The tagging feature, as it currently is, is convenient. But it's essentially an additional return value. Anything that will be used as a clear interface between two things -- whether those things are functions, modules, or whatever -- the author needs to be able to control it to not expose internals that were never meant to be a promised part of the interface or contract.

I, personally, would like if the proposed dot notation worked internally only.

fn triangle = (origin) => {
  const t = startSketchOn('XZ')
  |> startProfileAt(origin, %)
  |> line([60.53, 96.64], %, $A)
  |> line([45.66, -93.45], %)
  |> lineTo([profileStartX(%), profileStartY(%)], %)
  |> close(%)

  // I can use t.A here like any other variable.

  return t
}

const myTriangle = triangle([3.19, 0.53])

// This shouldn't work since $A is local to the function.
// const myTaggedEdge = myTriangle.A

But maybe there's a way to specify in the signature what's exposed outside the function.

I don't particularly like the syntax below, and it's definitely half-baked, but the point is that it's explicit.

Additionally, we can determine whether the use is valid by examining only the function signature. Contrast this with the OP's proposal where we would need to inspect the implementation of triangle() to determine whether myTriangle.A is defined, what its type is, etc.

I could also conceive renaming the tags before output so that you can use concise names internally and explicit, unambiguous names externally.

// The function specifies that it returns a Sketch with a tag called top.
// It can be a comma-separated list to include others, like Sketch{top, left}
fn triangle = (origin): Sketch{top} => {
  const t = startSketchOn('XZ')
  |> startProfileAt(origin, %)
  |> line([60.53, 96.64], %, $top)
  |> line([45.66, -93.45], %, $left)
  |> lineTo([profileStartX(%), profileStartY(%)], %)
  |> close(%)

  // I can use t.top and t.left here like any other variable.

  return t
}

const myTriangle = triangle([3.19, 0.53])

// Now this works since `top` is exposed in the signature.
const myTaggedEdge = myTriangle.top

// This doesn't work since `left` is not exposed in the signature.
// const myTaggedEdge2 = myTriangle.left
franknoirot commented 4 months ago

This is a very cool idea @jtran, I like it. There's a lot of authoring intent going into that interface, so I guess I'm wondering what a point-and-click only user flow for that experience could look like. I worry that if a user wants to make a function for some repeatable piece of geometry, it will not be obvious for a long while why they can't perform some later action that relies on having access to a tag within the component's geometry.

Sketching on face, for example, relies on having a tag of a sketch's segment. So if someone has made some geometry with a component, then goes to sketch on a face only to learn they don't have access to that face because the component author didn't make a public tag, that seems frustrating. Or, like you see in my demo, they actually have to alter the implementation to unlock sketching on a face generated by that segment.

https://github.com/KittyCAD/modeling-app/assets/23481541/af74757e-f408-4b12-a932-9c98ca070a97

jtran commented 4 months ago

Yes, I see what you mean. In the point and click interface, it would likely be confusing if it didn't let them choose a face and start sketching on it. After chatting with Jess about it, maybe we'll need to deal with hiding internal implementation details for assemblies or packages. But for everyday scripts that we're working with now, it's not really a problem.

Irev-Dev commented 4 months ago

I was about to raise the same point as Frank, I think everything does need to be exposed.

And in the case of us we having robust support for installing 3rd party packages, and we have a crates.io equivalent, I think we should have a lint rule that forces all segments to have a tag before you can publish. That way user's are still able to select any and all features on what they've imported (we can add a ast-mod fix as well for making the tags 6 character hashes or similar).

Controling what's exposed makes sense for what functions or geometry is exported from a module, but I don't think it would really work for tags.

Sure the scripts we currently support now don't have this usecase, but this seems like a future problem we will hit sooner or later, and makes sense to avoid.

lf94 commented 4 months ago

I think we should have a lint rule that forces all segments to have a tag before you can publish

I think it'd be better if all segments get auto tagged (could be a hash of the line and in cases of duplicates an incremented counter, all which would be more robust than just 0, 1, 2 etc which would change as maybe more segments are added), and then maybe always returned from a function

jessfraz commented 4 months ago

I solved this when I solved the other