From 3d042b0375a96b94ecd27397561922edb1c4dec4 Mon Sep 17 00:00:00 2001 From: max Date: Fri, 3 Oct 2025 20:02:07 +0200 Subject: [PATCH 01/12] Handle edgeCut selection in getSweepArtifactFromSelection Added logic to support 'edgeCut' artifact type in getSweepArtifactFromSelection. The function now retrieves the associated sweep artifact by resolving the consumed edge, handling both 'segment' and 'sweepEdge' cases. --- src/lang/std/artifactGraph.ts | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/src/lang/std/artifactGraph.ts b/src/lang/std/artifactGraph.ts index 8a105eec005..5bb3ed7d15d 100644 --- a/src/lang/std/artifactGraph.ts +++ b/src/lang/std/artifactGraph.ts @@ -515,6 +515,35 @@ export function getSweepArtifactFromSelection( ) if (err(_artifact)) return _artifact sweepArtifact = _artifact + } else if (selection.artifact?.type === 'edgeCut') { + // Handle edgeCut by getting its consumed edge (segment or sweepEdge) + const segOrEdge = getArtifactOfTypes( + { + key: selection.artifact.consumedEdgeId, + types: ['segment', 'sweepEdge'], + }, + artifactGraph + ) + if (err(segOrEdge)) return segOrEdge + + // If it's a segment, traverse through path to get sweep + if (segOrEdge.type === 'segment') { + const path = getArtifactOfTypes( + { key: segOrEdge.pathId, types: ['path'] }, + artifactGraph + ) + if (err(path)) return path + if (!path.sweepId) return new Error('Path does not have a sweepId') + return getArtifactOfTypes( + { key: path.sweepId, types: ['sweep'] }, + artifactGraph + ) + } + // Otherwise it's a sweepEdge, get sweep directly + return getArtifactOfTypes( + { key: segOrEdge.sweepId, types: ['sweep'] }, + artifactGraph + ) } if (!sweepArtifact) return new Error('No sweep artifact found') From 53970fed12dcb7b0a94c47807f118d0a15885ac3 Mon Sep 17 00:00:00 2001 From: max Date: Fri, 3 Oct 2025 20:02:55 +0200 Subject: [PATCH 02/12] Add function to coerce selections to body artifacts Introduces coerceSelectionsToBody to convert selections containing faces or edges to their parent body artifacts. This utility helps commands that require body-level selections by ensuring only body artifacts are returned, handling various artifact types and avoiding duplicates. --- src/lang/std/artifactGraph.ts | 82 ++++++++++++++++++++++++++++++++++- 1 file changed, 81 insertions(+), 1 deletion(-) diff --git a/src/lang/std/artifactGraph.ts b/src/lang/std/artifactGraph.ts index 5bb3ed7d15d..d124a02cba1 100644 --- a/src/lang/std/artifactGraph.ts +++ b/src/lang/std/artifactGraph.ts @@ -29,7 +29,7 @@ import type { SweepEdge, WallArtifact, } from '@src/lang/wasm' -import type { Selection } from '@src/lib/selections' +import type { Selection, Selections } from '@src/lib/selections' import { err } from '@src/lib/trap' export type { Artifact, ArtifactId, SegmentArtifact } from '@src/lang/wasm' @@ -868,3 +868,83 @@ export function getFaceCodeRef( } return null } + +/** + * Coerce selections that may contain faces or edges to their parent body (sweep/compositeSolid). + * This is useful for commands that only work with bodies, but users may have faces or edges selected. + * + * @param selections - The selections to coerce + * @param artifactGraph - The artifact graph to use for lookups + * @returns A new Selections object with only body artifacts, or an Error if coercion fails + */ +export function coerceSelectionsToBody( + selections: Selections, + artifactGraph: ArtifactGraph +): Selections | Error { + const bodySelections: Selection[] = [] + const seenBodyIds = new Set() + + for (const selection of selections.graphSelections) { + if (!selection.artifact) { + continue + } + + let bodyArtifact: Artifact | null = null + let bodyCodeRef: CodeRef | null = null + + // If it's already a body type, use it directly + if ( + selection.artifact.type === 'sweep' || + selection.artifact.type === 'compositeSolid' || + selection.artifact.type === 'path' + ) { + bodyArtifact = selection.artifact + bodyCodeRef = selection.codeRef + } else { + // Get the parent body (sweep) from faces, edges, or edgeCuts + // getSweepArtifactFromSelection handles all common types: sweepEdge, segment, wall, cap, edgeCut + const sweep = getSweepArtifactFromSelection(selection, artifactGraph) + + if (err(sweep)) { + return new Error( + `Unable to find parent body for selected artifact: ${selection.artifact.type}` + ) + } + + // Prefer the path over the sweep for the final selection + // The engine selects the path (not the sweep) when the object filter is active, + // so we follow the same convention for consistency + const sweepArtifact = sweep as Artifact + if ('pathId' in sweepArtifact && sweepArtifact.pathId) { + const path = getArtifactOfTypes( + { key: sweepArtifact.pathId, types: ['path'] }, + artifactGraph + ) + if (!err(path)) { + bodyArtifact = path + bodyCodeRef = path.codeRef + } else { + bodyArtifact = sweepArtifact + bodyCodeRef = sweep.codeRef + } + } else { + bodyArtifact = sweepArtifact + bodyCodeRef = sweep.codeRef + } + } + + // Add to the result, avoiding duplicates + if (bodyArtifact && bodyCodeRef && !seenBodyIds.has(bodyArtifact.id)) { + seenBodyIds.add(bodyArtifact.id) + bodySelections.push({ + artifact: bodyArtifact, + codeRef: bodyCodeRef, + }) + } + } + + return { + graphSelections: bodySelections, + otherSelections: selections.otherSelections, + } +} From 8d09e80301c4d711b1849e9648ff3461456c3409 Mon Sep 17 00:00:00 2001 From: max Date: Fri, 3 Oct 2025 20:26:27 +0200 Subject: [PATCH 03/12] Coerce selections to bodies for body-only arguments Adds logic to automatically coerce selections to body types (path, sweep, compositeSolid) when the argument only accepts bodies. Updates selection filter handling to ensure correct selection state after coercion, improving support for body-only commands in the command bar. --- .../CommandBarSelectionMixedInput.tsx | 60 +++++++++++++++++-- 1 file changed, 56 insertions(+), 4 deletions(-) diff --git a/src/components/CommandBar/CommandBarSelectionMixedInput.tsx b/src/components/CommandBar/CommandBarSelectionMixedInput.tsx index f17ee5a8f14..3a8cb0a76a9 100644 --- a/src/components/CommandBar/CommandBarSelectionMixedInput.tsx +++ b/src/components/CommandBar/CommandBarSelectionMixedInput.tsx @@ -10,6 +10,8 @@ import { } from '@src/lib/selections' import { kclManager } from '@src/lib/singletons' import { commandBarActor, useCommandBarState } from '@src/lib/singletons' +import { coerceSelectionsToBody } from '@src/lang/std/artifactGraph' +import { err } from '@src/lib/trap' const selectionSelector = (snapshot: any) => snapshot?.context.selectionRanges @@ -26,11 +28,52 @@ export default function CommandBarSelectionMixedInput({ const commandBarState = useCommandBarState() const [hasSubmitted, setHasSubmitted] = useState(false) const [hasAutoSkipped, setHasAutoSkipped] = useState(false) + const [hasCoercedSelections, setHasCoercedSelections] = useState(false) const selection: Selections = useSelector(arg.machineActor, selectionSelector) const selectionsByType = useMemo(() => { return getSelectionCountByType(selection) }, [selection]) + + // Coerce selections to bodies if this argument requires bodies + useEffect(() => { + // Only run once per component mount + if (hasCoercedSelections) return + + // Mark as attempted to prevent infinite loops + setHasCoercedSelections(true) + + if (!selection || selection.graphSelections.length === 0) return + + // Check if this argument only accepts body types (path, sweep, compositeSolid) + // These are the artifact types that represent 3D bodies/objects + const onlyAcceptsBodies = arg.selectionTypes?.every( + (type) => type === 'sweep' || type === 'compositeSolid' || type === 'path' + ) + + if (onlyAcceptsBodies && arg.machineActor) { + const coercedSelections = coerceSelectionsToBody( + selection, + kclManager.artifactGraph + ) + + if (!err(coercedSelections)) { + // Immediately update the modeling machine state with coerced selection + // This needs to happen BEFORE the selection filter is applied + if (arg.machineActor) { + arg.machineActor.send({ + type: 'Set selection', + data: { + selectionType: 'completeSelection', + selection: coercedSelections, + }, + }) + } + } + } + // eslint-disable-next-line react-hooks/exhaustive-deps -- Only run on mount + }, []) + const isArgRequired = arg.required instanceof Function ? arg.required(commandBarState.context) @@ -67,11 +110,20 @@ export default function CommandBarSelectionMixedInput({ }, [arg.name]) // Set selection filter if needed, and reset it when the component unmounts + // This runs after coercion completes and updates the selection useEffect(() => { - arg.selectionFilter && kclManager.setSelectionFilter(arg.selectionFilter) - return () => kclManager.defaultSelectionFilter(selection) - // eslint-disable-next-line react-hooks/exhaustive-deps -- TODO: blanket-ignored fix me! - }, [arg.selectionFilter]) + if (arg.selectionFilter && hasCoercedSelections) { + // Pass the current selection to restore it after applying the filter + // This is critical for body-only commands where we've coerced face/edge selections to bodies + kclManager.setSelectionFilter(arg.selectionFilter, selection) + } + return () => { + if (arg.selectionFilter && hasCoercedSelections) { + kclManager.defaultSelectionFilter(selection) + } + } + // eslint-disable-next-line react-hooks/exhaustive-deps -- Need to react to selection changes after coercion + }, [arg.selectionFilter, selection, hasCoercedSelections]) // Watch for outside teardowns of this component // (such as clicking another argument in the command palette header) From 8eae2a493d6c0103b574244ed5e8151360cc2552 Mon Sep 17 00:00:00 2001 From: max Date: Fri, 3 Oct 2025 20:42:30 +0200 Subject: [PATCH 04/12] Update setSelectionFilter to accept selectionsToRestore Added an optional selectionsToRestore parameter to setSelectionFilter in KclManager and passed it to setSelectionFilter implementation. This allows restoring selections when setting the filter. --- src/lang/KclSingleton.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/lang/KclSingleton.ts b/src/lang/KclSingleton.ts index a7fb87d70af..20aa4191bfe 100644 --- a/src/lang/KclSingleton.ts +++ b/src/lang/KclSingleton.ts @@ -790,8 +790,8 @@ export class KclManager extends EventTarget { setSelectionFilterToDefault(this.engineCommandManager, selectionsToRestore) } /** TODO: this function is hiding unawaited asynchronous work */ - setSelectionFilter(filter: EntityType[]) { - setSelectionFilter(filter, this.engineCommandManager) + setSelectionFilter(filter: EntityType[], selectionsToRestore?: Selections) { + setSelectionFilter(filter, this.engineCommandManager, selectionsToRestore) } // Determines if there is no KCL code which means it is executing a blank KCL file From 2e70319880a416d9b0f96ccaf68e36576d76adec Mon Sep 17 00:00:00 2001 From: max Date: Fri, 3 Oct 2025 22:00:03 +0200 Subject: [PATCH 05/12] Update Selection types import source --- src/lang/std/artifactGraph.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/lang/std/artifactGraph.ts b/src/lang/std/artifactGraph.ts index dad3e40eb61..627ccb22c74 100644 --- a/src/lang/std/artifactGraph.ts +++ b/src/lang/std/artifactGraph.ts @@ -29,8 +29,7 @@ import type { SweepEdge, WallArtifact, } from '@src/lang/wasm' -import type { Selections } from '@src/lib/selections' -import type { Selection } from '@src/machines/modelingSharedTypes' +import type { Selection, Selections } from '@src/machines/modelingSharedTypes' import { err } from '@src/lib/trap' export type { Artifact, ArtifactId, SegmentArtifact } from '@src/lang/wasm' From 00ab3749459fb142cca54dd580e905d173832a21 Mon Sep 17 00:00:00 2001 From: max Date: Sat, 4 Oct 2025 14:32:35 +0200 Subject: [PATCH 06/12] Add tests for artifactGraph selection utilities Introduces unit tests for getSweepArtifactFromSelection and coerceSelectionsToBody functions, verifying correct artifact resolution and selection coercion behavior in artifactGraph. --- src/lang/std/artifactGraph.test.ts | 185 +++++++++++++++++++++++++++++ 1 file changed, 185 insertions(+) create mode 100644 src/lang/std/artifactGraph.test.ts diff --git a/src/lang/std/artifactGraph.test.ts b/src/lang/std/artifactGraph.test.ts new file mode 100644 index 00000000000..6cd7c8d0914 --- /dev/null +++ b/src/lang/std/artifactGraph.test.ts @@ -0,0 +1,185 @@ +import { describe, it, expect } from 'vitest' +import { + coerceSelectionsToBody, + getSweepArtifactFromSelection, + type Artifact, +} from './artifactGraph' +import type { ArtifactGraph } from '@src/lang/wasm' +import type { Selections, Selection } from '@src/machines/modelingSharedTypes' + +describe('getSweepArtifactFromSelection', () => { + it('should return sweep from edgeCut selection', () => { + const artifactGraph: ArtifactGraph = new Map() + + // Create path -> sweep -> segment -> edgeCut chain + const path: Artifact = { + type: 'path', + id: 'path-1', + codeRef: { range: [0, 0, 0], pathToNode: [], nodePath: { steps: [] } }, + planeId: 'plane-1', + segIds: ['segment-1'], + sweepId: 'sweep-1', + } + + const sweep: Artifact = { + type: 'sweep', + id: 'sweep-1', + codeRef: { + range: [0, 0, 0], + pathToNode: [], + nodePath: { steps: [] }, + }, + pathId: 'path-1', + subType: 'extrusion', + surfaceIds: [], + edgeIds: [], + } + + const segment: Artifact = { + type: 'segment', + id: 'segment-1', + pathId: 'path-1', + edgeIds: [], + commonSurfaceIds: [], + codeRef: { + range: [0, 0, 0], + pathToNode: [], + nodePath: { steps: [] }, + }, + } + + const edgeCut: Artifact = { + type: 'edgeCut', + id: 'edge-cut-1', + consumedEdgeId: 'segment-1', + subType: 'chamfer', + edgeIds: [], + codeRef: { + range: [0, 0, 0], + pathToNode: [], + nodePath: { steps: [] }, + }, + } + + artifactGraph.set('path-1', path) + artifactGraph.set('sweep-1', sweep) + artifactGraph.set('segment-1', segment) + artifactGraph.set('edge-cut-1', edgeCut) + + const selection: Selection = { + artifact: edgeCut, + codeRef: { range: [0, 0, 0], pathToNode: [] }, + } + + const result = getSweepArtifactFromSelection(selection, artifactGraph) + + expect(result).not.toBeInstanceOf(Error) + if (!(result instanceof Error)) { + expect('type' in result ? result.type : undefined).toBe('sweep') + expect(result.id).toBe('sweep-1') + } + }) +}) + +describe('coerceSelectionsToBody', () => { + it('should pass through path artifact unchanged', () => { + const artifactGraph: ArtifactGraph = new Map() + + const path: Artifact = { + type: 'path', + id: 'path-1', + codeRef: { range: [0, 100, 0], pathToNode: [], nodePath: { steps: [] } }, + planeId: 'plane-1', + segIds: [], + } + artifactGraph.set('path-1', path) + + const selections: Selections = { + graphSelections: [ + { + artifact: path, + codeRef: { range: [0, 100, 0], pathToNode: [] }, + }, + ], + otherSelections: [], + } + + const result = coerceSelectionsToBody(selections, artifactGraph) + + expect(result).not.toBeInstanceOf(Error) + if (!(result instanceof Error)) { + expect(result.graphSelections).toHaveLength(1) + expect(result.graphSelections[0].artifact?.type).toBe('path') + expect(result.graphSelections[0].artifact?.id).toBe('path-1') + } + }) + + it('should coerce edgeCut selection to parent path', () => { + const artifactGraph: ArtifactGraph = new Map() + + const path: Artifact = { + type: 'path', + id: 'path-1', + codeRef: { range: [0, 100, 0], pathToNode: [], nodePath: { steps: [] } }, + planeId: 'plane-1', + segIds: ['segment-1'], + sweepId: 'sweep-1', + } + + const sweep: Artifact = { + type: 'sweep', + id: 'sweep-1', + codeRef: { + range: [100, 200, 0], + pathToNode: [], + nodePath: { steps: [] }, + }, + pathId: 'path-1', + subType: 'extrusion', + surfaceIds: [], + edgeIds: [], + } + + const segment: Artifact = { + type: 'segment', + id: 'segment-1', + pathId: 'path-1', + edgeIds: [], + commonSurfaceIds: [], + codeRef: { range: [10, 20, 0], pathToNode: [], nodePath: { steps: [] } }, + } + + const edgeCut: Artifact = { + type: 'edgeCut', + id: 'edge-cut-1', + consumedEdgeId: 'segment-1', + subType: 'chamfer', + edgeIds: [], + codeRef: { range: [90, 95, 0], pathToNode: [], nodePath: { steps: [] } }, + } + + artifactGraph.set('path-1', path) + artifactGraph.set('sweep-1', sweep) + artifactGraph.set('segment-1', segment) + artifactGraph.set('edge-cut-1', edgeCut) + + const selections: Selections = { + graphSelections: [ + { + artifact: edgeCut, + codeRef: { range: [90, 95, 0], pathToNode: [] }, + }, + ], + otherSelections: [], + } + + const result = coerceSelectionsToBody(selections, artifactGraph) + + expect(result).not.toBeInstanceOf(Error) + if (!(result instanceof Error)) { + expect(result.graphSelections).toHaveLength(1) + expect(result.graphSelections[0].artifact?.type).toBe('path') + expect(result.graphSelections[0].artifact?.id).toBe('path-1') + } + }) +}) From bff8495fc3da9d0576c2d1b3d46af50ae5d63aa9 Mon Sep 17 00:00:00 2001 From: max Date: Sat, 4 Oct 2025 15:43:11 +0200 Subject: [PATCH 07/12] Pass handleSelectionBatch to setSelectionFilter Updates setSelectionFilter to explicitly pass handleSelectionBatchFn as an argument, ensuring the correct function is used for selection batch handling. --- src/lang/KclSingleton.ts | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/lang/KclSingleton.ts b/src/lang/KclSingleton.ts index 211cdcd69e8..dcc75e24659 100644 --- a/src/lang/KclSingleton.ts +++ b/src/lang/KclSingleton.ts @@ -51,7 +51,7 @@ import type { PlaneVisibilityMap, Selections, } from '@src/machines/modelingSharedTypes' -import { type handleSelectionBatch as handleSelectionBatchFn } from '@src/lib/selections' +import { handleSelectionBatch as handleSelectionBatchFn } from '@src/lib/selections' interface ExecuteArgs { ast?: Node @@ -793,7 +793,12 @@ export class KclManager extends EventTarget { } /** TODO: this function is hiding unawaited asynchronous work */ setSelectionFilter(filter: EntityType[], selectionsToRestore?: Selections) { - setSelectionFilter(filter, this.engineCommandManager, selectionsToRestore) + setSelectionFilter( + filter, + this.engineCommandManager, + selectionsToRestore, + handleSelectionBatchFn + ) } // Determines if there is no KCL code which means it is executing a blank KCL file From 8777cc9857efa0bc7ffcf61e8974d01bb67316dc Mon Sep 17 00:00:00 2001 From: max Date: Wed, 15 Oct 2025 17:52:18 +0200 Subject: [PATCH 08/12] Add test for translate with segment-to-body coercion Introduces a Playwright test verifying that selecting a segment and invoking the translate command coerces the selection to the parent body. Also adds the translate button to the ToolbarFixture for use in tests. --- e2e/playwright/fixtures/toolbarFixture.ts | 2 + e2e/playwright/point-click.spec.ts | 109 ++++++++++++++++++++++ 2 files changed, 111 insertions(+) diff --git a/e2e/playwright/fixtures/toolbarFixture.ts b/e2e/playwright/fixtures/toolbarFixture.ts index c357ce6867d..7dc786c6bef 100644 --- a/e2e/playwright/fixtures/toolbarFixture.ts +++ b/e2e/playwright/fixtures/toolbarFixture.ts @@ -28,6 +28,7 @@ export class ToolbarFixture { revolveButton!: Locator offsetPlaneButton!: Locator helixButton!: Locator + translateButton!: Locator patternCircularButton!: Locator patternLinearButton!: Locator startSketchBtn!: Locator @@ -72,6 +73,7 @@ export class ToolbarFixture { this.revolveButton = page.getByTestId('revolve') this.offsetPlaneButton = page.getByTestId('plane-offset') this.helixButton = page.getByTestId('helix') + this.translateButton = page.getByTestId('translate') this.patternCircularButton = page.getByTestId('pattern-circular-3d') this.patternLinearButton = page.getByTestId('pattern-linear-3d') this.startSketchBtn = page.getByTestId('sketch') diff --git a/e2e/playwright/point-click.spec.ts b/e2e/playwright/point-click.spec.ts index 596f5442a05..0b66d0ef350 100644 --- a/e2e/playwright/point-click.spec.ts +++ b/e2e/playwright/point-click.spec.ts @@ -2548,6 +2548,115 @@ sketch002 = startSketchOn(extrude001, face = rectangleSegmentA001) }) }) + test(`Translate point-and-click with segment-to-body coercion`, async ({ + context, + page, + homePage, + scene, + editor, + toolbar, + cmdBar, + }) => { + const initialCode = `sketch = startSketchOn(XY) +profile = startProfile(sketch, at = [-5, -10]) + |> xLine(length = 10) + |> yLine(length = 20) + |> xLine(length = -10) + |> close() +box = extrude(profile, length = 30)` + const expectedTranslateCode = `translate(box, x = 50)` + const segmentToSelect = `yLine(length = 20)` + + await test.step('Settle the scene', async () => { + await context.addInitScript((initialCode) => { + localStorage.setItem('persistCode', initialCode) + }, initialCode) + await page.setBodyDimensions({ width: 1000, height: 500 }) + await homePage.goToModelingScene() + await scene.settled(cmdBar) + }) + + await test.step('Select an edge first (before opening translate)', async () => { + await editor.selectText(segmentToSelect) + await expect(toolbar.selectionStatus).toContainText('1 segment') + }) + + await test.step('Open translate via context menu and verify coercion', async () => { + await toolbar.translateButton.click() + + // When translate opens with a segment selected, it should coerce to the parent body + // The segment belongs to the 'profile' path, which is extruded into 'box' + // So the selection should coerce from segment to path (body) + await cmdBar.expectState({ + commandName: 'Translate', + currentArgKey: 'objects', + currentArgValue: '', + headerArguments: { + Objects: '', + }, + highlightedHeaderArg: 'objects', + stage: 'arguments', + }) + + await expect(page.getByText('1 path selected')).toBeVisible() + await expect(toolbar.selectionStatus).toContainText('1 path') + }) + + await test.step('Complete command flow', async () => { + await test.step('Progress to review since object is already selected', async () => { + await cmdBar.progressCmdBar() + await cmdBar.expectState({ + stage: 'review', + headerArguments: { + Objects: '1 path', + }, + commandName: 'Translate', + }) + }) + + await test.step('Add x translation', async () => { + await cmdBar.clickOptionalArgument('x') + await cmdBar.expectState({ + stage: 'arguments', + currentArgKey: 'x', + currentArgValue: '0', + headerArguments: { + Objects: '1 path', + X: '', + }, + highlightedHeaderArg: 'x', + commandName: 'Translate', + }) + await page.keyboard.insertText('50') + await cmdBar.progressCmdBar() + }) + + await test.step('Review and submit', async () => { + await cmdBar.expectState({ + stage: 'review', + headerArguments: { + Objects: '1 path', + X: '50', + }, + commandName: 'Translate', + }) + await cmdBar.submit() + await scene.settled(cmdBar) + }) + }) + + await test.step('Verify code was added correctly', async () => { + await toolbar.closePane('feature-tree') + await toolbar.openPane('code') + await editor.expectEditor.toContain(expectedTranslateCode) + await editor.expectState({ + diagnostics: [], + activeLines: [expectedTranslateCode], + highlightedCode: '', + }) + }) + }) + test(`Appearance point-and-click`, async ({ context, page, From f3f59b5e5adca8f0642d6b2a88da5bf1001a4381 Mon Sep 17 00:00:00 2001 From: max Date: Wed, 15 Oct 2025 19:44:59 +0200 Subject: [PATCH 09/12] Refactor body selection logic in coerceSelectionsToBody Simplifies and clarifies the process of determining and collecting body artifacts from selections. Removes intermediate variables and streamlines duplicate checking, improving readability and maintainability. --- src/lang/std/artifactGraph.ts | 64 +++++++++++++++++------------------ 1 file changed, 32 insertions(+), 32 deletions(-) diff --git a/src/lang/std/artifactGraph.ts b/src/lang/std/artifactGraph.ts index 627ccb22c74..8c3e63d2ba5 100644 --- a/src/lang/std/artifactGraph.ts +++ b/src/lang/std/artifactGraph.ts @@ -889,58 +889,58 @@ export function coerceSelectionsToBody( continue } - let bodyArtifact: Artifact | null = null - let bodyCodeRef: CodeRef | null = null - // If it's already a body type, use it directly if ( selection.artifact.type === 'sweep' || selection.artifact.type === 'compositeSolid' || selection.artifact.type === 'path' ) { - bodyArtifact = selection.artifact - bodyCodeRef = selection.codeRef + if (!seenBodyIds.has(selection.artifact.id)) { + seenBodyIds.add(selection.artifact.id) + bodySelections.push({ + artifact: selection.artifact, + codeRef: selection.codeRef, + }) + } } else { // Get the parent body (sweep) from faces, edges, or edgeCuts - // getSweepArtifactFromSelection handles all common types: sweepEdge, segment, wall, cap, edgeCut - const sweep = getSweepArtifactFromSelection(selection, artifactGraph) + const maybeSweep = getSweepArtifactFromSelection(selection, artifactGraph) - if (err(sweep)) { + if (err(maybeSweep)) { return new Error( `Unable to find parent body for selected artifact: ${selection.artifact.type}` ) } // Prefer the path over the sweep for the final selection - // The engine selects the path (not the sweep) when the object filter is active, - // so we follow the same convention for consistency - const sweepArtifact = sweep as Artifact - if ('pathId' in sweepArtifact && sweepArtifact.pathId) { - const path = getArtifactOfTypes( - { key: sweepArtifact.pathId, types: ['path'] }, + const maybePath = getArtifactOfTypes( + { key: maybeSweep.pathId, types: ['path'] }, + artifactGraph + ) + if (!err(maybePath)) { + // Successfully got the path from the sweep + if (!seenBodyIds.has(maybePath.id)) { + seenBodyIds.add(maybePath.id) + bodySelections.push({ + artifact: maybePath, + codeRef: maybePath.codeRef, + }) + } + } else { + // Couldn't get path, use the sweep itself + const sweepWithType = getArtifactOfTypes( + { key: maybeSweep.id, types: ['sweep'] }, artifactGraph ) - if (!err(path)) { - bodyArtifact = path - bodyCodeRef = path.codeRef - } else { - bodyArtifact = sweepArtifact - bodyCodeRef = sweep.codeRef + if (!err(sweepWithType) && !seenBodyIds.has(sweepWithType.id)) { + seenBodyIds.add(sweepWithType.id) + bodySelections.push({ + artifact: sweepWithType, + codeRef: maybeSweep.codeRef, + }) } - } else { - bodyArtifact = sweepArtifact - bodyCodeRef = sweep.codeRef } } - - // Add to the result, avoiding duplicates - if (bodyArtifact && bodyCodeRef && !seenBodyIds.has(bodyArtifact.id)) { - seenBodyIds.add(bodyArtifact.id) - bodySelections.push({ - artifact: bodyArtifact, - codeRef: bodyCodeRef, - }) - } } return { From 9a0e87c552ad5818e21e8b55355823a5a735b5cc Mon Sep 17 00:00:00 2001 From: max Date: Wed, 15 Oct 2025 19:50:31 +0200 Subject: [PATCH 10/12] Update import path for artifactGraph in test file Changed the import path for artifactGraph in artifactGraph.test.ts to use an absolute path. This improves consistency and may help with module resolution. --- src/lang/std/artifactGraph.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/lang/std/artifactGraph.test.ts b/src/lang/std/artifactGraph.test.ts index 6cd7c8d0914..a49eb37936a 100644 --- a/src/lang/std/artifactGraph.test.ts +++ b/src/lang/std/artifactGraph.test.ts @@ -3,7 +3,7 @@ import { coerceSelectionsToBody, getSweepArtifactFromSelection, type Artifact, -} from './artifactGraph' +} from '@src/lang/std/artifactGraph' import type { ArtifactGraph } from '@src/lang/wasm' import type { Selections, Selection } from '@src/machines/modelingSharedTypes' From 2503bfb8b143c61976150f975972efa591222db0 Mon Sep 17 00:00:00 2001 From: max Date: Wed, 15 Oct 2025 21:09:39 +0200 Subject: [PATCH 11/12] unfuck imported geometry Adds logic to include selections without artifacts (such as imported modules) in bodySelections if their code reference range is non-zero. This prepares for future handling of imports and their associated edges and faces. --- src/lang/std/artifactGraph.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/lang/std/artifactGraph.ts b/src/lang/std/artifactGraph.ts index 8c3e63d2ba5..7ef4a5152ae 100644 --- a/src/lang/std/artifactGraph.ts +++ b/src/lang/std/artifactGraph.ts @@ -886,6 +886,12 @@ export function coerceSelectionsToBody( for (const selection of selections.graphSelections) { if (!selection.artifact) { + // Handle selections without artifacts (e.g., imported modules) + // TODO: coerce to body when we have ranges for imports + // TODO: coerce edges and faces of imported bodies + if (selection.codeRef.range[1] - selection.codeRef.range[0] !== 0) { + bodySelections.push(selection) + } continue } From 88162cd19a898dbd85192760d4b20146305b3702 Mon Sep 17 00:00:00 2001 From: max Date: Wed, 15 Oct 2025 22:14:18 +0200 Subject: [PATCH 12/12] unfuck circular dep Introduces a helper to batch selection filter changes with selection restoration, preventing UI flickering when coercing selections. Updates CommandBarSelectionMixedInput to use this helper and simplifies KclManager's setSelectionFilter signature. --- .../CommandBarSelectionMixedInput.tsx | 69 ++++++++++++++++++- src/lang/KclSingleton.ts | 9 +-- 2 files changed, 68 insertions(+), 10 deletions(-) diff --git a/src/components/CommandBar/CommandBarSelectionMixedInput.tsx b/src/components/CommandBar/CommandBarSelectionMixedInput.tsx index 50580913df0..8273f45a864 100644 --- a/src/components/CommandBar/CommandBarSelectionMixedInput.tsx +++ b/src/components/CommandBar/CommandBarSelectionMixedInput.tsx @@ -6,15 +6,74 @@ import { canSubmitSelectionArg, getSelectionCountByType, getSelectionTypeDisplayText, + handleSelectionBatch, } from '@src/lib/selections' import { kclManager, engineCommandManager } from '@src/lib/singletons' import { commandBarActor, useCommandBarState } from '@src/lib/singletons' import { coerceSelectionsToBody } from '@src/lang/std/artifactGraph' import { err } from '@src/lib/trap' import type { Selections } from '@src/machines/modelingSharedTypes' +import { uuidv4 } from '@src/lib/utils' +import type { EntityType, ModelingCmdReq } from '@kittycad/lib' const selectionSelector = (snapshot: any) => snapshot?.context.selectionRanges +/** + * Helper to set selection filter and restore selections in a single batch + * This prevents flickering by batching the filter change with selection restoration + */ +function setFilterAndRestoreSelection( + filter: EntityType[], + selectionsToRestore?: Selections +) { + if (!selectionsToRestore) { + // No selections to restore, just set the filter + kclManager.setSelectionFilter(filter) + return + } + + // Get the commands to restore selections + const { engineEvents } = handleSelectionBatch({ + selections: selectionsToRestore, + }) + + if (!engineEvents || engineEvents.length === 0) { + // No restoration commands, just set the filter + kclManager.setSelectionFilter(filter) + return + } + + // Build the batch request with filter + restoration commands + const modelingCmd: ModelingCmdReq[] = [] + engineEvents.forEach((event) => { + if (event.type === 'modeling_cmd_req') { + modelingCmd.push({ + cmd_id: uuidv4(), + cmd: event.cmd, + }) + } + }) + + // Send as batch to prevent flickering + engineCommandManager + .sendSceneCommand({ + type: 'modeling_cmd_batch_req', + batch_id: uuidv4(), + requests: [ + { + cmd_id: uuidv4(), + cmd: { + type: 'set_selection_filter', + filter, + }, + }, + ...modelingCmd, + ], + responses: false, + }) + .catch((error) => console.error('Failed to set filter and restore:', error)) +} + export default function CommandBarSelectionMixedInput({ arg, stepBack, @@ -127,13 +186,17 @@ export default function CommandBarSelectionMixedInput({ // This runs after coercion completes and updates the selection useEffect(() => { if (arg.selectionFilter && hasCoercedSelections) { - // Pass the current selection to restore it after applying the filter + // Use our helper to batch the filter change with selection restoration // This is critical for body-only commands where we've coerced face/edge selections to bodies - kclManager.setSelectionFilter(arg.selectionFilter, selection) + setFilterAndRestoreSelection(arg.selectionFilter, selection) } return () => { if (arg.selectionFilter && hasCoercedSelections) { - kclManager.defaultSelectionFilter(selection) + // Restore default filter with selections on cleanup + setFilterAndRestoreSelection( + ['face', 'edge', 'solid2d', 'curve', 'object'], + selection + ) } } // eslint-disable-next-line react-hooks/exhaustive-deps -- Need to react to selection changes after coercion diff --git a/src/lang/KclSingleton.ts b/src/lang/KclSingleton.ts index dcc75e24659..211cdcd69e8 100644 --- a/src/lang/KclSingleton.ts +++ b/src/lang/KclSingleton.ts @@ -51,7 +51,7 @@ import type { PlaneVisibilityMap, Selections, } from '@src/machines/modelingSharedTypes' -import { handleSelectionBatch as handleSelectionBatchFn } from '@src/lib/selections' +import { type handleSelectionBatch as handleSelectionBatchFn } from '@src/lib/selections' interface ExecuteArgs { ast?: Node @@ -793,12 +793,7 @@ export class KclManager extends EventTarget { } /** TODO: this function is hiding unawaited asynchronous work */ setSelectionFilter(filter: EntityType[], selectionsToRestore?: Selections) { - setSelectionFilter( - filter, - this.engineCommandManager, - selectionsToRestore, - handleSelectionBatchFn - ) + setSelectionFilter(filter, this.engineCommandManager, selectionsToRestore) } // Determines if there is no KCL code which means it is executing a blank KCL file