Fix sketchOnFace point&click for booleans by Irev-Dev · Pull Request #6713 · KittyCAD/modeling-app

Irev-Dev · 2025-05-06T08:23:54Z

The fix is changing this to adding the new expression instead of after the extrusion, instead at the end of the file, because the ME's wanted that anyway. And than some modifications to findAllChildrenAndOrderByPlaceInCode allows it to find the last declared variable so that it uses the solid001 etc for the startSketchOn

Fix bug
Add test

qa-wolf · 2025-05-06T08:23:57Z

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 · 2025-05-06T08:23:57Z

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

Name	Status	Preview	Comments	Updated (UTC)
modeling-app	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	May 6, 2025 9:58pm

Irev-Dev · 2025-05-06T08:25:33Z

src/hooks/useEngineConnectionSubscriptions.ts

-                  type: 'Select default plane',
+                  type: 'Select sketch plane',


This has been a bad name for a while, thought I should fix it up while I was in here.

Irev-Dev · 2025-05-06T08:25:46Z

src/hooks/useEngineConnectionSubscriptions.ts

              }

-              // Artifact is likely an extrusion face
+              // Artifact is likely an sweep face


Little bit of clean up, using the more generic term.

Irev-Dev · 2025-05-06T10:13:34Z

src/lang/modifyAst/boolean.ts

+  const pathToCompositeSolidMap: { [key: string]: string[] } = {}
+  for (const [id, artifact] of artifactGraph) {
+    if (artifact.type === 'compositeSolid') {
+      for (const pathId of artifact.solidIds) {
+        if (pathToCompositeSolidMap[pathId]) {
+          pathToCompositeSolidMap[pathId].push(id)
+        } else {
+          pathToCompositeSolidMap[pathId] = [id]
+        }
+      }
+      for (const pathId of artifact.toolIds) {
+        if (pathToCompositeSolidMap[pathId]) {
+          pathToCompositeSolidMap[pathId].push(id)
+        } else {
+          pathToCompositeSolidMap[pathId] = [id]
+        }
+      }
+    }
+  }


@jtran I wanted to ask you about this

I'm building up a little map to go from a pathId to compositeSolids if they exist, because the composite solids in the artifactGraph done have edges going both ways, so there's no way to go from a path to its compositeSolid

I did try adding the edge going the other way in https://github.com/KittyCAD/modeling-app/compare/kurt-composite-attempt?expand=1
But I danno it just didn't work and I couldn't figure out why, I thought maybe I screwed up the merge logic, but it looks okay to me?

But what do you think, solve this in rust, or just keep what I've got?

I'll wait for @jtran's input before merging

When you say "it just didn't work", what symptoms are you actually seeing? The Mermaid chart generation wasn't updated, but when I did that, the charts show that the edges are bidirectional for the sim tests we have. But for some reason, edges in the graph are lost. Here's the diff of the intersect_cubes sim test. The path (3) correctly gains the edge to the composite (17), but it loses the edge to the solid2d (13). Same thing with the other path (4).

Wait, I think I fixed it. The above diff made it obvious. I'll push to your branch.

Pushed a few commits. This one is the fix: e23e6d2.

Thank you!

Looking now.

Oh, what a silly mistake :/

Okay, I've removed my little mapping, just waiting for CI to be green.

pierremtb

Diff looks good and local testing went well!

There are still faces not available for sketching depending on the cut we make with CSG, and extrudes tend to get added weirdly up in the ast sometimes but that's out of scope here (and that could be addressed in #6437).

pierremtb · 2025-05-06T12:17:23Z

e2e/playwright/fixtures/toolbarFixture.ts

+  async waitForFeatureTreeToBeBuilt() {
+    await this.openFeatureTreePane()
+    await expect(this.page.getByText('Building feature tree')).not.toBeVisible()
+    await this.closeFeatureTreePane()
+  }


Oh this looks handy

Yeah seems like a good proxy for execution finished

pierremtb · 2025-05-06T12:20:35Z

e2e/playwright/sketch-tests.spec.ts

+      await expect
+        .poll(async () => {
+          const circleBtn = page.getByRole('button', { name: 'circle Circle' })
+          return circleBtn.getAttribute('aria-pressed')
+        })
+        .toBe('true')


Out of curiosity: is this meant to ensure we're ready to start sketching? As in, the highlight on the button is async after click, I guess as we "equip" the tool yes?

jtran · 2025-05-06T17:37:51Z

src/lang/modifyAst/boolean.ts

+    if (artifact.type === 'compositeSolid') {
+      for (const pathId of artifact.solidIds) {
+        if (pathToCompositeSolidMap[pathId]) {
+          pathToCompositeSolidMap[pathId].push(id)


Is it possible for a path to get used in multiple CSG ops? If so, composite_solid_id: Option<ArtifactId>, would need to get converted to composite_solid_ids: Vec<ArtifactId>,.

yes, but not yet I don't think, it would be the keepTool situation.

* composite attempt * Add forward edge to CSG artifacts in artifact graph * Fix comment * Move the doc comments above the attributes * Fix to update the correct field of Path * Update output --------- Co-authored-by: Jonathan Tran <jonnytran@gmail.com>

src/hooks/useEngineConnectionSubscriptions.ts

public/kcl-samples/screenshots/dodecahedron.png

rust/kcl-lib/tests/import_transform/rendered_model.png

Co-authored-by: graphite-app[bot] <96075541+graphite-app[bot]@users.noreply.github.com>

codspeed-hq · 2025-05-06T20:12:29Z

CodSpeed Instrumentation Performance Report

Merging #6713 will not alter performance

_{Comparing kurt-bool-sketchonface-6307 (1ff012f) with main (1841e63)}

Summary

✅ 54 untouched benchmarks

fix bool sketchOnFace

d994b18

Irev-Dev changed the title ~~fix bool sketchOnFace~~ Fix sketchOnFace point&click for booleans May 6, 2025

Merge branch 'main' into kurt-bool-sketchonface-6307

83a951f

Irev-Dev commented May 6, 2025

View reviewed changes

vercel bot deployed to Preview May 6, 2025 08:51 View deployment

Irev-Dev added 2 commits May 6, 2025 19:44

fix chamfer test

09a9613

add test

a0cfa6e

vercel bot deployed to Preview May 6, 2025 09:57 View deployment

Irev-Dev marked this pull request as ready for review May 6, 2025 10:10

Irev-Dev requested a review from a team May 6, 2025 10:10

Irev-Dev commented May 6, 2025

View reviewed changes

pierremtb approved these changes May 6, 2025

View reviewed changes

jtran reviewed May 6, 2025

View reviewed changes

Irev-Dev and others added 3 commits May 7, 2025 05:45

Kurt composite attempt (#6722)

614bfe7

* composite attempt * Add forward edge to CSG artifacts in artifact graph * Fix comment * Move the doc comments above the attributes * Fix to update the correct field of Path * Update output --------- Co-authored-by: Jonathan Tran <jonnytran@gmail.com>

Merge remote-tracking branch 'origin' into kurt-bool-sketchonface-6307

d9b8b72

use rust defined composite solid edges instead

5cab762

graphite-app bot reviewed May 6, 2025

View reviewed changes

src/hooks/useEngineConnectionSubscriptions.ts Outdated Show resolved Hide resolved

jtran reviewed May 6, 2025

View reviewed changes

public/kcl-samples/screenshots/dodecahedron.png Show resolved Hide resolved

jtran reviewed May 6, 2025

View reviewed changes

rust/kcl-lib/tests/import_transform/rendered_model.png Show resolved Hide resolved

Update src/hooks/useEngineConnectionSubscriptions.ts

649e934

Co-authored-by: graphite-app[bot] <96075541+graphite-app[bot]@users.noreply.github.com>

vercel bot deployed to Preview May 6, 2025 20:10 View deployment

jtran added 2 commits May 6, 2025 17:46

Revert PNG screenshots

2876a6c

Fix TS formatting

1ff012f

vercel bot deployed to Preview May 6, 2025 21:58 View deployment

Irev-Dev merged commit d187a29 into main May 6, 2025
68 checks passed

Irev-Dev deleted the kurt-bool-sketchonface-6307 branch May 6, 2025 22:25

pierremtb mentioned this pull request May 9, 2025

[BUG]: SketchOnFace on a CSG face, generates code BEFORE the CSG #6307

Closed

Irev-Dev mentioned this pull request May 12, 2025

[BUG] Sketch on face code code-mods are wrong #6692

Closed

Comments

Conversation

Irev-Dev commented May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qa-wolf bot commented May 6, 2025

Uh oh!

vercel bot commented May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Irev-Dev May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pierremtb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codspeed-hq bot commented May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging #6713 will not alter performance

Summary

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Irev-Dev commented May 6, 2025 •

edited

Loading

vercel bot commented May 6, 2025 •

edited

Loading

Irev-Dev May 6, 2025 •

edited

Loading

codspeed-hq bot commented May 6, 2025 •

edited

Loading