ByoungSooPark/modeling-app
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Add edit flow for Shell thickness (#5525)

Browse Source 
* WIP: Add edit flow for Shell thickness
Would fix #5406 but not like this :sad:

* Early win with working edit on shell

* Extend to logic to walls with tags

* Remove skip

* Add wall test

* Fixing inconsistencies in code. Tests not working on win

* Refactor addShell for consitency

* Clean up

* More clean up

* Add validation on both params

* A snapshot a day keeps the bugs away! 📷🐛 (OS: namespace-profile-ubuntu-8-cores)

* Passing tests

* Prettier

* Change from test.skip to comment

* Clean up for review

* Add review suggestions and disable thickness validator

* A snapshot a day keeps the bugs away! 📷🐛 (OS: namespace-profile-ubuntu-8-cores)

* A snapshot a day keeps the bugs away! 📷🐛 (OS: namespace-profile-ubuntu-8-cores)

* Remove uneccessary changes

* Add edit flow for Shell thickness
Fixes #5406

* Enable edit step in test that works only if the start code is properly formatted

* Clean up for review

---------

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
This commit is contained in:
Pierre Jacquier
2025-03-14 16:05:41 -04:00
committed by GitHub
parent e9806b83d7
commit 4741d9592b
7 changed files with 420 additions and 162 deletions
Download Patch File Download Diff File Expand all files Collapse all files

84
e2e/playwright/point-click.spec.ts
View File

@ -2271,8 +2271,8 @@ chamfer04 = chamfer(extrude001, length = 5, tags = [getOppositeEdge(seg02)])
cmdBar,
}) => {
const initialCode = `sketch001 = startSketchOn('XZ')
|> circle(center = [0, 0], radius = 30)
extrude001 = extrude(sketch001, length = 30)
|> circle(center = [0, 0], radius = 30)
extrude001 = extrude(sketch001, length = 30)
`
await context.addInitScript((initialCode) => {
localStorage.setItem('persistCode', initialCode)
@ -2286,6 +2286,8 @@ chamfer04 = chamfer(extrude001, length = 5, tags = [getOppositeEdge(seg02)])
const [clickOnCap] = scene.makeMouseHelpers(testPoint.x, testPoint.y)
const shellDeclaration =
"shell001 = shell(extrude001, faces = ['end'], thickness = 5)"
const editedShellDeclaration =
"shell001 = shell(extrude001, faces = ['end'], thickness = 2)"
await test.step(`Look for the grey of the shape`, async () => {
await scene.expectPixelColor([127, 127, 127], testPoint, 15)
@ -2352,6 +2354,45 @@ chamfer04 = chamfer(extrude001, length = 5, tags = [getOppositeEdge(seg02)])
})
await scene.expectPixelColor([146, 146, 146], testPoint, 15)
})
await test.step('Edit shell via feature tree selection works', async () => {
await toolbar.closePane('code')
await toolbar.openPane('feature-tree')
const operationButton = await toolbar.getFeatureTreeOperation(
'Shell',
0
)
await operationButton.dblclick()
await cmdBar.expectState({
stage: 'arguments',
currentArgKey: 'thickness',
currentArgValue: '5',
headerArguments: {
Thickness: '5',
},
highlightedHeaderArg: 'thickness',
commandName: 'Shell',
})
await page.keyboard.insertText('2')
await cmdBar.progressCmdBar()
await cmdBar.expectState({
stage: 'review',
headerArguments: {
Thickness: '2',
},
commandName: 'Shell',
})
await cmdBar.progressCmdBar()
await toolbar.closePane('feature-tree')
await scene.expectPixelColor([150, 150, 150], testPoint, 15)
await toolbar.openPane('code')
await editor.expectEditor.toContain(editedShellDeclaration)
await editor.expectState({
diagnostics: [],
activeLines: [editedShellDeclaration],
highlightedCode: '',
})
})
})
})
@ -2387,6 +2428,8 @@ extrude001 = extrude(sketch001, length = 40)
const mutatedCode = 'xLine(length = -40, tag = $seg01)'
const shellDeclaration =
"shell001 = shell(extrude001, faces = ['end', seg01], thickness = 5)"
const editedShellDeclaration =
"shell001 = shell(extrude001, faces = ['end', seg01], thickness = 1)"
await test.step(`Look for the grey of the shape`, async () => {
await scene.expectPixelColor([99, 99, 99], testPoint, 15)
@ -2435,6 +2478,41 @@ extrude001 = extrude(sketch001, length = 40)
await scene.expectPixelColor([49, 49, 49], testPoint, 15)
})
await test.step('Edit shell via feature tree selection works', async () => {
await editor.closePane()
const operationButton = await toolbar.getFeatureTreeOperation('Shell', 0)
await operationButton.dblclick({ button: 'left' })
await cmdBar.expectState({
stage: 'arguments',
currentArgKey: 'thickness',
currentArgValue: '5',
headerArguments: {
Thickness: '5',
},
highlightedHeaderArg: 'thickness',
commandName: 'Shell',
})
await page.keyboard.insertText('1')
await cmdBar.progressCmdBar()
await cmdBar.expectState({
stage: 'review',
headerArguments: {
Thickness: '1',
},
commandName: 'Shell',
})
await cmdBar.progressCmdBar()
await toolbar.closePane('feature-tree')
await scene.expectPixelColor([150, 150, 150], testPoint, 15)
await toolbar.openPane('code')
await editor.expectEditor.toContain(editedShellDeclaration)
await editor.expectState({
diagnostics: [],
activeLines: [editedShellDeclaration],
highlightedCode: '',
})
})
await test.step('Delete shell via feature tree selection', async () => {
await editor.closePane()
const operationButton = await toolbar.getFeatureTreeOperation('Shell', 0)
@ -2529,7 +2607,7 @@ extrude002 = extrude(sketch002, length = 50)
highlightedCode: '',
})
await toolbar.closePane('code')
await scene.expectPixelColor([73, 73, 73], testPoint, 15)
await scene.expectPixelColor([80, 80, 80], testPoint, 15)
})
})
})

BIN
e2e/playwright/snapshot-tests.spec.ts-snapshots/Client-side-scene-scale-should-match-engine-scale-Inch-scale-1-Google-Chrome-linux.png
View File

Binary file not shown.
Side by Side Swipe Overlay

Before

Width:  |  Height:  |  Size: 54 KiB

After

Width:  |  Height:  |  Size: 54 KiB

58
src/lang/modifyAst.ts
View File

@ -457,6 +457,64 @@ export function loftSketches(
}
}
export function addShell({
node,
sweepName,
faces,
thickness,
insertIndex,
variableName,
}: {
node: Node<Program>
sweepName: string
faces: Expr[]
thickness: Expr
insertIndex?: number
variableName?: string
}): { modifiedAst: Node<Program>; pathToNode: PathToNode } {
const modifiedAst = structuredClone(node)
const name =
variableName ?? findUniqueName(node, KCL_DEFAULT_CONSTANT_PREFIXES.SHELL)
const shell = createCallExpressionStdLibKw(
'shell',
createIdentifier(sweepName),
[
createLabeledArg('faces', createArrayExpression(faces)),
createLabeledArg('thickness', thickness),
]
)
const variable = createVariableDeclaration(name, shell)
const insertAt =
insertIndex !== undefined
? insertIndex
: modifiedAst.body.length
? modifiedAst.body.length
: 0
if (modifiedAst.body.length) {
modifiedAst.body.splice(insertAt, 0, variable)
} else {
modifiedAst.body.push(variable)
}
const argIndex = 0
const pathToNode: PathToNode = [
['body', ''],
[insertAt, 'index'],
['declaration', 'VariableDeclaration'],
['init', 'VariableDeclarator'],
['arguments', 'CallExpressionKw'],
[argIndex, ARG_INDEX_FIELD],
['arg', LABELED_ARG_FIELD],
]
return {
modifiedAst,
pathToNode,
}
}
export function addSweep(
node: Node<Program>,
profileDeclarator: VariableDeclarator,

135
src/lang/modifyAst/addShell.ts
View File

@ -1,135 +0,0 @@
import { Selections } from 'lib/selections'
import { Expr } from '@rust/kcl-lib/bindings/Expr'
import { Program } from '@rust/kcl-lib/bindings/Program'
import { Node } from '@rust/kcl-lib/bindings/Node'
import { ArtifactGraph, PathToNode, VariableDeclarator } from 'lang/wasm'
import {
getPathToExtrudeForSegmentSelection,
mutateAstWithTagForSketchSegment,
} from './addEdgeTreatment'
import { getNodeFromPath } from 'lang/queryAst'
import { err } from 'lib/trap'
import {
createLiteral,
createIdentifier,
findUniqueName,
createArrayExpression,
createVariableDeclaration,
createCallExpressionStdLibKw,
createLabeledArg,
} from 'lang/modifyAst'
import { KCL_DEFAULT_CONSTANT_PREFIXES } from 'lib/constants'
export function addShell({
node,
selection,
artifactGraph,
thickness,
}: {
node: Node<Program>
selection: Selections
artifactGraph: ArtifactGraph
thickness: Expr
}): Error | { modifiedAst: Node<Program>; pathToNode: PathToNode } {
const modifiedAst = structuredClone(node)
// Look up the corresponding extrude
const clonedAstForGetExtrude = structuredClone(modifiedAst)
const expressions: Expr[] = []
let pathToExtrudeNode: PathToNode | undefined = undefined
for (const graphSelection of selection.graphSelections) {
const extrudeLookupResult = getPathToExtrudeForSegmentSelection(
clonedAstForGetExtrude,
graphSelection,
artifactGraph
)
if (err(extrudeLookupResult)) {
return new Error("Couldn't find extrude")
}
// TODO: this assumes the segment is piped directly from the sketch, with no intermediate `VariableDeclarator` between.
// We must find a technique for these situations that is robust to intermediate declarations
const extrudeNode = getNodeFromPath<VariableDeclarator>(
modifiedAst,
extrudeLookupResult.pathToExtrudeNode,
'VariableDeclarator'
)
const segmentNode = getNodeFromPath<VariableDeclarator>(
modifiedAst,
extrudeLookupResult.pathToSegmentNode,
'VariableDeclarator'
)
if (err(extrudeNode) || err(segmentNode)) {
return new Error("Couldn't find extrude")
}
if (
extrudeNode.node.init.type === 'CallExpression' ||
extrudeNode.node.init.type === 'CallExpressionKw'
) {
pathToExtrudeNode = extrudeLookupResult.pathToExtrudeNode
} else if (segmentNode.node.init.type === 'PipeExpression') {
pathToExtrudeNode = extrudeLookupResult.pathToSegmentNode
} else {
return new Error("Couldn't find extrude")
}
const selectedArtifact = graphSelection.artifact
if (!selectedArtifact) {
return new Error('Bad artifact')
}
// Check on the selection, and handle the wall vs cap casees
let expr: Expr
if (selectedArtifact.type === 'cap') {
expr = createLiteral(selectedArtifact.subType)
} else if (selectedArtifact.type === 'wall') {
const tagResult = mutateAstWithTagForSketchSegment(
modifiedAst,
extrudeLookupResult.pathToSegmentNode
)
if (err(tagResult)) return tagResult
const { tag } = tagResult
expr = createIdentifier(tag)
} else {
continue
}
expressions.push(expr)
}
if (!pathToExtrudeNode) return new Error('No extrude found')
const extrudeNode = getNodeFromPath<VariableDeclarator>(
modifiedAst,
pathToExtrudeNode,
'VariableDeclarator'
)
if (err(extrudeNode)) {
return extrudeNode
}
const name = findUniqueName(node, KCL_DEFAULT_CONSTANT_PREFIXES.SHELL)
const shell = createCallExpressionStdLibKw(
'shell',
createIdentifier(extrudeNode.node.id.name),
[
createLabeledArg('faces', createArrayExpression(expressions)),
createLabeledArg('thickness', thickness),
]
)
const declaration = createVariableDeclaration(name, shell)
// TODO: check if we should append at the end like here or right after the extrude
modifiedAst.body.push(declaration)
const pathToNode: PathToNode = [
['body', ''],
[modifiedAst.body.length - 1, 'index'],
['declaration', 'VariableDeclaration'],
['init', 'VariableDeclarator'],
['unlabeled', 'CallExpressionKw'],
]
return {
modifiedAst,
pathToNode,
}
}

12
src/lib/commandBarConfigs/modelingCommandConfig.ts
View File

@ -58,6 +58,9 @@ export type ModelingCommandSchema = {
selection: Selections
}
Shell: {
// Enables editing workflow
nodeToEdit?: PathToNode
// KCL stdlib arguments
selection: Selections
thickness: KclCommandValue
}
@ -382,18 +385,25 @@ export const modelingMachineCommandConfig: StateMachineCommandSetConfig<
icon: 'shell',
needsReview: true,
args: {
nodeToEdit: {
description:
'Path to the node in the AST to edit. Never shown to the user.',
skip: true,
inputType: 'text',
required: false,
},
selection: {
inputType: 'selection',
selectionTypes: ['cap', 'wall'],
multiple: true,
required: true,
validation: shellValidator,
hidden: (context) => Boolean(context.argumentsToSubmit.nodeToEdit),
},
thickness: {
inputType: 'kcl',
defaultValue: KCL_DEFAULT_LENGTH,
required: true,
// TODO: add dry-run validation on thickness param
},
},
},

126
src/lib/operations.ts
View File

@ -1,5 +1,9 @@
import { CustomIconName } from 'components/CustomIcon'
import { Artifact, getArtifactOfTypes } from 'lang/std/artifactGraph'
import {
Artifact,
getArtifactOfTypes,
getCapCodeRef,
} from 'lang/std/artifactGraph'
import { Operation } from '@rust/kcl-lib/bindings/Operation'
import { codeManager, engineCommandManager, kclManager } from './singletons'
import { err } from './trap'
@ -9,7 +13,7 @@ import { CommandBarMachineEvent } from 'machines/commandBarMachine'
import { stringToKclExpression } from './kclHelpers'
import { ModelingCommandSchema } from './commandBarConfigs/modelingCommandConfig'
import { isDefaultPlaneStr } from './planes'
import { Selections } from './selections'
import { Selection, Selections } from './selections'
type ExecuteCommandEvent = CommandBarMachineEvent & {
type: 'Find and select command'
@ -116,6 +120,123 @@ const prepareToEditExtrude: PrepareToEditCallback =
}
}
/**
* Gather up the argument values for the Shell command
* to be used in the command bar edit flow.
*/
const prepareToEditShell: PrepareToEditCallback =
async function prepareToEditShell({ operation }) {
const baseCommand = {
name: 'Shell',
groupId: 'modeling',
}
if (
operation.type !== 'StdLibCall' ||
!operation.labeledArgs ||
!operation.unlabeledArg ||
operation.unlabeledArg.value.type !== 'Solid' ||
!('thickness' in operation.labeledArgs) ||
!('faces' in operation.labeledArgs) ||
!operation.labeledArgs.thickness ||
!operation.labeledArgs.faces ||
operation.labeledArgs.faces.value.type !== 'Array'
) {
return baseCommand
}
// Build an artifact map here of eligible artifacts corresponding to our current sweep
// that we can query in another loop later
const sweepId = operation.unlabeledArg.value.value.artifactId
const candidates: Map<string, Selection> = new Map()
for (const artifact of engineCommandManager.artifactGraph.values()) {
if (
artifact.type === 'cap' &&
artifact.sweepId === sweepId &&
artifact.subType
) {
const codeRef = getCapCodeRef(
artifact,
engineCommandManager.artifactGraph
)
if (err(codeRef)) {
return baseCommand
}
candidates.set(artifact.subType, {
artifact,
codeRef,
})
} else if (
artifact.type === 'wall' &&
artifact.sweepId === sweepId &&
artifact.segId
) {
const segArtifact = getArtifactOfTypes(
{ key: artifact.segId, types: ['segment'] },
engineCommandManager.artifactGraph
)
if (err(segArtifact)) {
return baseCommand
}
const { codeRef } = segArtifact
candidates.set(artifact.segId, {
artifact,
codeRef,
})
}
}
// Loop over face value to retrieve the corresponding artifacts and build the graphSelections
const faceValues = operation.labeledArgs.faces.value.value
const graphSelections: Selection[] = []
for (const v of faceValues) {
if (v.type === 'String' && v.value && candidates.has(v.value)) {
graphSelections.push(candidates.get(v.value)!)
} else if (
v.type === 'TagIdentifier' &&
v.artifact_id &&
candidates.has(v.artifact_id)
) {
graphSelections.push(candidates.get(v.artifact_id)!)
} else {
return baseCommand
}
}
// Convert the thickness argument from a string to a KCL expression
const thickness = await stringToKclExpression(
codeManager.code.slice(
operation.labeledArgs?.['thickness']?.sourceRange[0],
operation.labeledArgs?.['thickness']?.sourceRange[1]
)
)
if (err(thickness) || 'errors' in thickness) {
return baseCommand
}
// Assemble the default argument values for the Shell command,
// with `nodeToEdit` set, which will let the Extrude actor know
// to edit the node that corresponds to the StdLibCall.
const argDefaultValues: ModelingCommandSchema['Shell'] = {
thickness,
selection: {
graphSelections,
otherSelections: [],
},
nodeToEdit: getNodePathFromSourceRange(
kclManager.ast,
sourceRangeFromRust(operation.sourceRange)
),
}
return {
...baseCommand,
argDefaultValues,
}
}
const prepareToEditOffsetPlane: PrepareToEditCallback = async ({
operation,
}) => {
@ -364,6 +485,7 @@ export const stdLibMap: Record<string, StdLibCallInfo> = {
shell: {
label: 'Shell',
icon: 'shell',
prepareToEdit: prepareToEditShell,
supportsAppearance: true,
},
startSketchOn: {

167
src/machines/modelingMachine.ts
View File

@ -1,4 +1,5 @@
import {
Expr,
PathToNode,
VariableDeclaration,
VariableDeclarator,
@ -13,7 +14,7 @@ import {
Selection,
updateSelections,
} from 'lib/selections'
import { assign, fromPromise, fromCallback, setup } from 'xstate'
import { assign, fromPromise, setup } from 'xstate'
import { SidebarType } from 'components/ModelingSidebar/ModelingPanes'
import { isNodeSafeToReplacePath } from 'lang/queryAst'
import { getNodePathFromSourceRange } from 'lang/queryAstNodePathUtils'
@ -43,7 +44,10 @@ import { revolveSketch } from 'lang/modifyAst/addRevolve'
import {
addHelix,
addOffsetPlane,
addShell,
addSweep,
createIdentifier,
createLiteral,
extrudeSketch,
loftSketches,
} from 'lang/modifyAst'
@ -52,6 +56,8 @@ import {
ChamferParameters,
EdgeTreatmentType,
FilletParameters,
getPathToExtrudeForSegmentSelection,
mutateAstWithTagForSketchSegment,
} from 'lang/modifyAst/addEdgeTreatment'
import { getNodeFromPath } from '../lang/queryAst'
import {
@ -78,7 +84,6 @@ import { ToolbarModeName } from 'lib/toolbar'
import { quaternionFromUpNForward } from 'clientSideScene/helpers'
import { Mesh, Vector3 } from 'three'
import { MachineManager } from 'components/MachineManagerProvider'
import { addShell } from 'lang/modifyAst/addShell'
import { KclCommandValue } from 'lib/commandTypes'
import { ModelingMachineContext } from 'components/ModelingMachineProvider'
import {
@ -1994,42 +1999,162 @@ export const modelingMachine = setup({
// Extract inputs
const ast = kclManager.ast
const { selection, thickness } = input
const { selection, thickness, nodeToEdit } = input
let variableName: string | undefined = undefined
let insertIndex: number | undefined = undefined
// Insert the thickness variable if it exists
if (
'variableName' in thickness &&
thickness.variableName &&
thickness.insertIndex !== undefined
) {
const newBody = [...ast.body]
newBody.splice(
thickness.insertIndex,
0,
thickness.variableDeclarationAst
// If this is an edit flow, first we're going to remove the old extrusion
if (nodeToEdit && typeof nodeToEdit[1][0] === 'number') {
// Extract the plane name from the node to edit
const variableNode = getNodeFromPath<VariableDeclaration>(
ast,
nodeToEdit,
'VariableDeclaration'
)
if (err(variableNode)) {
console.error('Error extracting name')
} else {
variableName = variableNode.node.declaration.id.name
}
// Removing the old statement
const newBody = [...ast.body]
newBody.splice(nodeToEdit[1][0], 1)
ast.body = newBody
insertIndex = nodeToEdit[1][0]
}
// Turn the selection into the faces list
const clonedAstForGetExtrude = structuredClone(ast)
const faces: Expr[] = []
let pathToExtrudeNode: PathToNode | undefined = undefined
for (const graphSelection of selection.graphSelections) {
const extrudeLookupResult = getPathToExtrudeForSegmentSelection(
clonedAstForGetExtrude,
graphSelection,
engineCommandManager.artifactGraph
)
if (err(extrudeLookupResult)) {
return new Error(
"Couldn't find extrude paths from getPathToExtrudeForSegmentSelection",
{ cause: extrudeLookupResult }
)
}
const extrudeNode = getNodeFromPath<VariableDeclaration>(
ast,
extrudeLookupResult.pathToExtrudeNode,
'VariableDeclaration'
)
if (err(extrudeNode)) {
return new Error("Couldn't find extrude node from selection", {
cause: extrudeNode,
})
}
const segmentNode = getNodeFromPath<VariableDeclaration>(
ast,
extrudeLookupResult.pathToSegmentNode,
'VariableDeclaration'
)
if (err(segmentNode)) {
return new Error("Couldn't find segment node from selection", {
cause: segmentNode,
})
}
if (
extrudeNode.node.declaration.init.type === 'CallExpression' ||
extrudeNode.node.declaration.init.type === 'CallExpressionKw'
) {
pathToExtrudeNode = extrudeLookupResult.pathToExtrudeNode
} else if (
segmentNode.node.declaration.init.type === 'PipeExpression'
) {
pathToExtrudeNode = extrudeLookupResult.pathToSegmentNode
} else {
return new Error(
"Couldn't find extrude node that was either a call expression or a pipe",
{ cause: segmentNode }
)
}
const selectedArtifact = graphSelection.artifact
if (!selectedArtifact) {
return new Error('Bad artifact from selection')
}
// Check on the selection, and handle the wall vs cap casees
let expr: Expr
if (selectedArtifact.type === 'cap') {
expr = createLiteral(selectedArtifact.subType)
} else if (selectedArtifact.type === 'wall') {
const tagResult = mutateAstWithTagForSketchSegment(
ast,
extrudeLookupResult.pathToSegmentNode
)
if (err(tagResult)) {
return tagResult
}
const { tag } = tagResult
expr = createIdentifier(tag)
} else {
return new Error('Artifact is neither a cap nor a wall')
}
faces.push(expr)
}
if (!pathToExtrudeNode) {
return new Error('No path to extrude node found')
}
const extrudeNode = getNodeFromPath<VariableDeclarator>(
ast,
pathToExtrudeNode,
'VariableDeclarator'
)
if (err(extrudeNode)) {
return new Error("Couldn't find extrude node", { cause: extrudeNode })
}
// Perform the shell op
const shellResult = addShell({
const sweepName = extrudeNode.node.id.name
const addResult = addShell({
node: ast,
selection,
artifactGraph: engineCommandManager.artifactGraph,
sweepName,
faces: faces,
thickness:
'variableName' in thickness
? thickness.variableIdentifierAst
: thickness.valueAst,
insertIndex,
variableName,
})
if (err(shellResult)) {
return err(shellResult)
// Insert the thickness variable if the user has provided a variable name
if (
'variableName' in thickness &&
thickness.variableName &&
typeof addResult.pathToNode[1][0] === 'number'
) {
const insertIndex = Math.min(
addResult.pathToNode[1][0],
thickness.insertIndex
)
const newBody = [...addResult.modifiedAst.body]
newBody.splice(insertIndex, 0, thickness.variableDeclarationAst)
addResult.modifiedAst.body = newBody
// Since we inserted a new variable, we need to update the path to the extrude argument
addResult.pathToNode[1][0]++
}
const updateAstResult = await kclManager.updateAst(
shellResult.modifiedAst,
addResult.modifiedAst,
true,
{
focusPath: [shellResult.pathToNode],
focusPath: [addResult.pathToNode],
}
)