Fillet UI #2718

Irev-Dev · 2024-06-20T05:08:46Z

Related to #2606

qa-wolf · 2024-06-20T05:08:48Z

QA Wolf here! As you write new code it's important that your test coverage is keeping up.
Click here to request test coverage for this PR!

vercel · 2024-06-20T05:08:50Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
modeling-app	✅ Ready (Inspect)	Visit Preview	Jul 15, 2024 7:24am

Irev-Dev · 2024-06-20T05:14:17Z

src/lang/modifyAst/addFillet.ts

+  pathToSegmentNode: PathToNode,
+  pathToExtrudeNode: PathToNode,
+  filletRadius: number = 5
+): { modifiedAst: Program; pathToSegmentNode: PathToNode } {


We're decided on a new convention of not throwing, because they can get a little out of hand, and @lf94 has gone to a massive effort to refactor existing throws in #2654, plenty of examples in that PR but the main thing to change is the return type should be { modifiedAst: Program; pathToSegmentNode: PathToNode } | Error and replace throws with return Promise.reject(...)

if I would just add an Error as a possible return type:
{ modifiedAst: Program; pathToSegmentNode: PathToNode } | Error
then I would have to return an Error instead of promise:
return new Error(...)

if we want to return the Promise, like this:
return Promise.reject(...)
then I would need to wrap the whole function in the promise:

Promise<{ modifiedAst: Program; pathToSegmentNode: PathToNode } | Error> { return new Promise((resolve, reject) => {

is this the way ?

You may return an Error directly without the promise if that's what makes sense. Promise.reject(new Error(...)) is just for functions which are async. So in other words yes just do T | Error :)

Irev-Dev · 2024-06-20T06:28:48Z

src/lang/modifyAst/addFillet.ts

+  let tag = null
+  if (lineSegment.callee.name === 'line') {
+    if (lineSegment.arguments.length === 2) {
+      tag = findUniqueName(node, KCL_DEFAULT_CONSTANT_PREFIXES.SEGMENT, 2)
+      lineSegment.arguments.push(createLiteral(tag))
+    } else if (lineSegment.arguments.length === 3) {
+      if (isLiteral(lineSegment.arguments[2])) {
+        tag = lineSegment.arguments[2].value as string
+      } else {
+        throw new Error('Expected a Literal node')
+      }
+    } else {
+      throw new Error('Line segment not found.')
+    }
+  } else {
+    throw new Error('Line segment not found.')
+  }


This makes a couple of assumtions, on that it's a line stdlib function call, and that the tag is in the 3rd positon (which ofc makes sense for line) but really we want this to be flexible for all of the stdlib segment functions. The good news is that all of the sketchLineHelpers in src/lang/std/sketch.ts have a util function addTag that knows how to add a tag for each stdlib segment function. (It's always the 3rd argument, with the exception of close which it's the second, but there could be more in future where it's different.)

All of these helper function can be combine by using the sketchLineHelpersMap, you should be able to use addTagForSketchOnFace or at least take inspiration from it.

There was a typing tweak I wanted to make debf270

thanks, good point, I've swapped the whole chunk with a:

const { tag } = addTagForSketchOnFace( { pathToNode: pathToSegmentNode, node: node, }, sketchSegment.callee.name )

and it works just fine :)

Okay sweet, we should probably rename that function then haha 😝

Irev-Dev · 2024-06-20T06:29:09Z

src/lang/modifyAst/addFillet.ts

+  const filletCall = createCallExpressionStdLib('fillet', [
+    createObjectExpression({
+      radius: createLiteral(filletRadius),
+      tags: createArrayExpression([createLiteral(tag)]),
+    }),
+    createPipeSubstitution(),
+  ])


max-mrgrsk · 2024-06-23T16:22:31Z

I was eager to see the fillet button working for the simplest case of one sketch and one extrude in the code editor.
And it is working!

You can select the edge, set the radius and that's it.
Selection of extrude node to fillet is just hard-coded for now.
The next step would be to take care of the UI menu in CommandBarSelectionInput.tsx and add the extra step to select the extrude object.

Until now it was crafted for the extrude function, so the text and number of steps are just hard-coded.
I'll have to modify it into a more flexible system that can take any input.

Modified files:
src/Toolbar.tsx - Fillet button and hotkey F
src/machines/modelingMachine.ts - Fillet condition and action
src/lib/selections.ts - canFilletSelection placeholder
src/lib/commandBarConfigs/modelingCommandConfig.ts - Fillet selection types
src/components/ModelingMachineProvider.tsx - ‘fillet selection’ guard
src/components/CommandBarSelectionInput.tsx - Currently works only for the extrude case src/components/CustomIcon.tsx - Fillet icon placeholder @franknoirot could you please make a nice icon ? ^^
src/lang/modifyAst/addFillet.ts
src/lang/modifyAst/addFillet.test.ts

Next steps
Currently works only with a simplest case of one sketch and one extrude in the kcl

Irev-Dev · 2024-06-24T01:44:45Z

Dope, you're doing really well.

Let us know if you get stuck with the cmdbar stuff, feel free to reach out to Frank too, he's done most of the cmdbar implementation.

codecov · 2024-06-29T14:03:36Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.75%. Comparing base (bb9d24f) to head (8c87188).
Report is 4 commits behind head on main.

❗ Current head 8c87188 differs from pull request most recent head ddaf36d

Please upload reports for the commit ddaf36d to get more accurate results.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2718   +/-   ##
=======================================
  Coverage   86.75%   86.75%           
=======================================
  Files          62       62           
  Lines       24565    24565           
=======================================
  Hits        21311    21311           
  Misses       3254     3254

Flag	Coverage Δ
wasm-lib	`86.75% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

max-mrgrsk · 2024-07-07T20:36:13Z

Big Update

I've finally touched all the files in the chain and uploaded the working fillet button. Here are the updates:

Button

Icon

src/components/CustomIcon.tsx

Added a fillet icon

@franknoirot, is there an existing naming convention for icons, states, and functions? We might end up with many functions having the same name but different logic for 2D and 3D applications.

States

ModelingMachineProvider.tsx

The button is active if:

Nothing is selected and there are bodies to fillet in the scene.
A sketch segment is selected, and there is a related body.

The button is inactive if:

Nothing is selected, and there are no bodies to fillet.
A sketch segment is selected, but there is no related body.
A non-sketch segment is selected.

The checker currently works only with extrudes. Ideally, we should add a body map similar to sketchLineHelperMap. Functions need refactoring to support chamfers and other features.

Command Bar

Selection Text

src/components/CommandBar/CommandBarSelectionInput.tsx

Thanks to @franknoirot, the text now adapts to selection (point/edge/face).

Selection Types

src/lib/commandBarConfigs/modelingCommandConfig.ts

The selectionTypes currently only accept extrude-wall. @Irev-Dev

Selecting the body

src/lang/modifyAst/addFillet.ts
src/machines/modelingMachine.ts

Currently, I assume one extrusion per sketch in the scene, using the sketch name as a tag. This works for now, but future lofts, sweeps, and multiple extrudes from one sketch will require precise body selection. We'll need a "body selection tab" in the command bar UI. @Irev-Dev I need some tips for having different selectionRanges for edges and bodies.

Tests

src/lang/modifyAst/addFillet.test.ts

Current tests cover single selection cases:

Single fillet after extrusion
Existing tag in another segment
Tag exists in the selected line
Fillet exists on another segment (same sketch)
Fillet exists on the selected segment (updates radius)
Fillet exists on the selected segment and others (updates radius)

Further tests to consider:

Selecting multiple edges
Selecting a single edge when it exists in a fillet call with multiple tags (the selected fillet should become a separate call)

Further Steps

Example of a separate pipe:

const extrude001 = extrude(5, sketch001)
const myBody = fillet({
  radius: 1,
  tags: [seg01 ]
}, extrude001)

Standby

waiting for the opposite and adjacent edges in the selectionRanges:

getNextAdjacentEdge("tag", myExtrude)
getOppositeEdge("tag2", myExtrude)

Irev-Dev · 2024-07-08T01:12:37Z

Sorry I might punt the Selecting the body stuff to @franknoirot as well, he knows better than me.

I think the priority should be to get this merged, I know it's not ready yet, but we're just going to be constantly fixing conflicts unless we do. We should take a note out of Jess's note book and do what's she's done for copilot and only have it enable for DEV, that we we can merge it before it's 100% done, and do the rest in follow up PRs.

You should be able to use DEV from src/env.ts

The tests are great, since they're all unit test, we'll definitely want a couple of e2e/playwright tests before the feature is finished (doesn't need to be done right now though)

max-mrgrsk · 2024-07-08T20:26:50Z

We should take a note out of Jess's note book and do what's she's done for copilot and only have it enable for DEV

@Irev-Dev Could you please leave a couple links related to it?

Irev-Dev · 2024-07-09T20:46:45Z

So the idea with only having it enable in dev mode, is all the code can be present and merged so long as it's not exposed to the user, in this case I think that mostly means the button and the hotkey so like

import { DEV } from 'env'

// ...

{state.matches('idle') && DEV && (
    <li className="contents">

Irev-Dev · 2024-07-15T10:19:59Z

I went ahead and merged this for Jess and Josh, I'll still do a normal review and leave more comments on what I did etc.

Irev-Dev · 2024-07-16T05:14:13Z

src/lang/modifyAst/addFillet.test.ts

-
-  /**
-   * 5. Case of existing fillet on selected segment
-   */
-
-  it('should modify existing fillet', async () => {
-    const code = `const sketch001 = startSketchOn('XZ')
-            |> startProfileAt([2.16, 49.67], %)
-            |> line([101.49, 139.93], %)
-            |> line([60.04, -55.72], %)
-            |> line([1.29, -115.74], %)
-            |> line([-87.24, -47.08], %, $seg03)
-            |> tangentialArcTo([56.15, -94.58], %)
-            |> tangentialArcTo([14.68, -104.52], %)
-            |> lineTo([profileStartX(%), profileStartY(%)], %)
-            |> close(%)
-          const extrude001 = extrude(50, sketch001)
-            |> fillet({ radius: 10, tags: [seg03] }, %)`
-    const segmentSnippet = `line([-87.24, -47.08], %, $seg03)`
-    const extrudeSnippet = `const extrude001 = extrude(50, sketch001)`
-    const radius = createLiteral(5) as Value
-    const expectedCode = `const sketch001 = startSketchOn('XZ')
-  |> startProfileAt([2.16, 49.67], %)
-  |> line([101.49, 139.93], %)
-  |> line([60.04, -55.72], %)
-  |> line([1.29, -115.74], %)
-  |> line([-87.24, -47.08], %, $seg03)
-  |> tangentialArcTo([56.15, -94.58], %)
-  |> tangentialArcTo([14.68, -104.52], %)
-  |> lineTo([profileStartX(%), profileStartY(%)], %)
-  |> close(%)
-const extrude001 = extrude(50, sketch001)
-  |> fillet({ radius: 5, tags: [seg03] }, %)`
-
-    await runFilletTest(
-      code,
-      segmentSnippet,
-      extrudeSnippet,
-      radius,
-      expectedCode
-    )
-  })
-
-  /**
-   * 6. Case of existing fillet on selected segment
-   */
-
-  it('should modify existing fillet, surrounded with other fillets', async () => {
-    const code = `const sketch001 = startSketchOn('XZ')
-          |> startProfileAt([2.16, 49.67], %)
-          |> line([101.49, 139.93], %)
-          |> line([60.04, -55.72], %, $seg05)
-          |> line([1.29, -115.74], %)
-          |> line([-87.24, -47.08], %, $seg03)
-          |> tangentialArcTo([56.15, -94.58], %)
-          |> tangentialArcTo([14.68, -104.52], %, $seg07)
-          |> lineTo([profileStartX(%), profileStartY(%)], %)
-          |> close(%)
-        const extrude001 = extrude(50, sketch001)
-          |> fillet({ radius: 10, tags: [seg03] }, %)
-          |> fillet({ radius: 15, tags: [seg05] }, %)
-          |> fillet({ radius: 7, tags: [seg07] }, %)`
-    const segmentSnippet = `line([-87.24, -47.08], %, $seg03)`
-    const extrudeSnippet = `const extrude001 = extrude(50, sketch001)`
-    const radius = createLiteral(5) as Value
-    const expectedCode = `const sketch001 = startSketchOn('XZ')
-  |> startProfileAt([2.16, 49.67], %)
-  |> line([101.49, 139.93], %)
-  |> line([60.04, -55.72], %, $seg05)
-  |> line([1.29, -115.74], %)
-  |> line([-87.24, -47.08], %, $seg03)
-  |> tangentialArcTo([56.15, -94.58], %)
-  |> tangentialArcTo([14.68, -104.52], %, $seg07)
-  |> lineTo([profileStartX(%), profileStartY(%)], %)
-  |> close(%)
-const extrude001 = extrude(50, sketch001)
-  |> fillet({ radius: 5, tags: [seg03] }, %)
-  |> fillet({ radius: 15, tags: [seg05] }, %)
-  |> fillet({ radius: 7, tags: [seg07] }, %)`
-
-    await runFilletTest(
-      code,
-      segmentSnippet,
-      extrudeSnippet,
-      radius,
-      expectedCode
-    )
-  })


I removed these tests because I didn't think they made much sense as the only way to "select" the right source range is for the user to manually put their cursor in the right place, the original edge itself was not selectable since the edge got filleted, so editing the existing fillet I don't think made much sense.

I would however like the fillet itself to be selectable and editable, but that will require some engine updates too.

Irev-Dev · 2024-07-16T05:15:32Z

src/lang/modifyAst/addFillet.ts

-  const pathToSketch = [pathToSegmentNode[0], pathToSegmentNode[1]]
-  const sketchNode = getNodeFromPath(_node, pathToSketch) as {
-    node: VariableDeclaration
-  }
-  const sketchName = sketchNode.node.declarations[0].id.name
-
-  // 2. Find the extrue expression with the same name
-  // TODO: Has to work with Pipe Expressions, Lofts, etc...
-
-  let extrudeRange: [number, number] | undefined = undefined
-  _node.body.forEach((node) => {
-    traverse(node, {
-      enter(node) {
-        if (node.type === 'CallExpression') {
-          if (node.callee.name === 'extrude') {
-            if (
-              node.arguments[1].type === 'Identifier' &&
-              node.arguments[1].name === sketchName
-            ) {
-              extrudeRange = [node.start, node.end]
-            }
-          }
-        }
-      },
-    })
-  })
-
-  if (extrudeRange === undefined) {
-    return new Error('No valid body found')
-  }
-
-  const preselectedPathToExtrudeTag = getNodePathFromSourceRange(
-    _node,
-    extrudeRange
-  )
-  const preselectedPathToExtrudeNode = [
-    preselectedPathToExtrudeTag[0],
-    preselectedPathToExtrudeTag[1],
-  ]
-  if (pathToExtrudeNode.length < 2) {
-    pathToExtrudeNode = preselectedPathToExtrudeNode
-  }


There was no point in having the pathToExtrudeNode as an input if we were not going to use it, instead I move the logic for finding this path to node into the AST fillet action. And the way I got the node was with the same method we use for find the extrude for sketch on face, to keep it consistent.

Irev-Dev · 2024-07-16T05:16:59Z

src/lang/modifyAst/addFillet.ts

+  const getPathToNodeOfFilletLiteral = (
+    pathToExtrudeNode: PathToNode,
+    extrudeDeclarator: VariableDeclarator,
+    tag: string
+  ): PathToNode => {
+    let pathToFilletObj: any
+    let inFillet = false
+    traverse(extrudeDeclarator.init, {
+      enter(node, path) {
+        if (node.type === 'CallExpression' && node.callee.name === 'fillet') {
+          inFillet = true
+        }
+        if (inFillet && node.type === 'ObjectExpression') {
+          const hasTag = node.properties.some((prop) => {
+            const isTagProp = prop.key.name === 'tags'
+            if (isTagProp && prop.value.type === 'ArrayExpression') {
+              return prop.value.elements.some((element) => {
+                // console.log()
+                return element.type === 'Identifier' && element.name === tag
+              })
+            }
+            return false
+          })
+          if (!hasTag) return false
+          pathToFilletObj = path
+          node.properties.forEach((prop, index) => {
+            if (prop.key.name === 'radius') {
+              pathToFilletObj.push(
+                ['properties', 'ObjectExpression'],
+                [index, 'index'],
+                ['value', 'Property']
+              )
+            }
+          })
+        }


While a little more verbose, this not hardcoding values for the path to node should make it more robust. I made a task on #2606 to write a test for this util

Irev-Dev · 2024-07-16T05:22:46Z

src/lang/modifyAst/addFillet.ts

@@ -218,58 +207,20 @@ export function addFillet(
      return new Error('Expected an ObjectExpression node')
    }

-    if (filletTag === tag) {


I ended up removing the edit case for the same reason as ad54158#r1678768111

Irev-Dev linked an issue Jun 20, 2024 that may be closed by this pull request

write AST-mod for adding fillet, piped to extrude variable expression #2607

Closed

Irev-Dev commented Jun 20, 2024

View reviewed changes

vercel bot deployed to Preview June 20, 2024 06:35 View deployment

vercel bot deployed to Preview June 20, 2024 06:46 View deployment

max-mrgrsk self-assigned this Jun 20, 2024

vercel bot deployed to Preview June 23, 2024 16:24 View deployment

max-mrgrsk linked an issue Jun 23, 2024 that may be closed by this pull request

Fillet UI #2606

Open

37 tasks

vercel bot deployed to Preview June 23, 2024 19:52 View deployment

Irev-Dev changed the title ~~2607 ast mod for fillet~~ Fillet UI Jun 24, 2024

vercel bot deployed to Preview June 29, 2024 14:07 View deployment

vercel bot deployed to Preview July 4, 2024 20:39 View deployment

vercel bot deployed to Preview July 7, 2024 19:18 View deployment

vercel bot deployed to Preview July 8, 2024 01:19 View deployment

vercel bot deployed to Preview July 9, 2024 19:19 View deployment

vercel bot deployed to Preview July 13, 2024 20:31 View deployment

vercel bot deployed to Preview July 14, 2024 15:11 View deployment

vercel bot deployed to Preview July 14, 2024 17:32 View deployment

vercel bot deployed to Preview July 14, 2024 18:11 View deployment

max-mrgrsk and others added 3 commits July 15, 2024 12:30

draft: fillet ast mod + test

8a492b1

Kurt's rejig

ad54158

playwright

fbb86c5

Irev-Dev force-pushed the 2607-ast-mod-for-fillet branch from 4c83444 to fbb86c5 Compare July 15, 2024 02:34

vercel bot deployed to Preview July 15, 2024 02:56 View deployment

Irev-Dev added 2 commits July 15, 2024 15:39

update button enable logic

daf2b80

remove fillet button in production build

59b2f68

Irev-Dev force-pushed the 2607-ast-mod-for-fillet branch from 5134bb2 to 59b2f68 Compare July 15, 2024 05:59

github-actions bot and others added 3 commits July 15, 2024 06:04

A snapshot a day keeps the bugs away! 📷🐛 (OS: ubuntu)

7720316

trigger CI

1a50eaa

fix typo

c325a95

vercel bot deployed to Preview July 15, 2024 06:31 View deployment

Irev-Dev marked this pull request as ready for review July 15, 2024 07:01

give a way to turn on fillets

69778a5

vercel bot deployed to Preview July 15, 2024 07:24 View deployment

Irev-Dev merged commit a1df3d0 into main Jul 15, 2024
15 checks passed

Irev-Dev deleted the 2607-ast-mod-for-fillet branch July 15, 2024 09:20

Irev-Dev mentioned this pull request Jul 16, 2024

fillet ui follow up #3035

Merged

Irev-Dev commented Jul 16, 2024

View reviewed changes

max-mrgrsk mentioned this pull request Jul 16, 2024

Fillet UI #2606

Open

37 tasks

Irev-Dev mentioned this pull request Jul 17, 2024

Cut release v0.24.2 #3042

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fillet UI #2718

Fillet UI #2718

Irev-Dev commented Jun 20, 2024 •

edited

Loading

qa-wolf bot commented Jun 20, 2024

vercel bot commented Jun 20, 2024 •

edited

Loading

Irev-Dev Jun 20, 2024

max-mrgrsk Jun 23, 2024

lf94 Jun 23, 2024 •

edited

Loading

Irev-Dev Jun 20, 2024

max-mrgrsk Jun 23, 2024

Irev-Dev Jun 24, 2024

Irev-Dev Jun 20, 2024

max-mrgrsk commented Jun 23, 2024

Irev-Dev commented Jun 24, 2024

codecov bot commented Jun 29, 2024

max-mrgrsk commented Jul 7, 2024 •

edited

Loading

Irev-Dev commented Jul 8, 2024 •

edited

Loading

max-mrgrsk commented Jul 8, 2024

Irev-Dev commented Jul 9, 2024

Irev-Dev commented Jul 15, 2024

Irev-Dev Jul 16, 2024

Irev-Dev Jul 16, 2024

Irev-Dev Jul 16, 2024

Irev-Dev Jul 16, 2024

Fillet UI #2718

Fillet UI #2718

Conversation

Irev-Dev commented Jun 20, 2024 • edited Loading

qa-wolf bot commented Jun 20, 2024

vercel bot commented Jun 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lf94 Jun 23, 2024 • edited Loading

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

max-mrgrsk commented Jun 23, 2024

Irev-Dev commented Jun 24, 2024

codecov bot commented Jun 29, 2024

Codecov Report

max-mrgrsk commented Jul 7, 2024 • edited Loading

Big Update

Button

Icon

States

Command Bar

Selection Text

Selection Types

Selecting the body

Tests

Further Steps

Standby

Irev-Dev commented Jul 8, 2024 • edited Loading

max-mrgrsk commented Jul 8, 2024

Irev-Dev commented Jul 9, 2024

Irev-Dev commented Jul 15, 2024

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

Irev-Dev commented Jun 20, 2024 •

edited

Loading

vercel bot commented Jun 20, 2024 •

edited

Loading

lf94 Jun 23, 2024 •

edited

Loading

max-mrgrsk commented Jul 7, 2024 •

edited

Loading

Irev-Dev commented Jul 8, 2024 •

edited

Loading