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

[BUG] Grab bag of sketch mode bug fixes (#6229)

Browse Source 
* use artifact id for sketch mode entry plane

* fix up re-eval as best as possible

* remove some async dodgyness

* fmt

* fix old sycronous re-execute shit

* add a test

* fix existing test

* add toast for error state

* spelling

* test stuff

* fmt

* fix toast

* test fix

* some other fix ups

* fix test
This commit is contained in:
Kurt Hutten
2025-04-15 23:27:50 +10:00
committed by GitHub
parent c51db5bd4a
commit 3cca4a30af
8 changed files with 583 additions and 202 deletions
Download Patch File Download Diff File Expand all files Collapse all files

3
e2e/playwright/import-ui.spec.ts
View File

@ -3,7 +3,7 @@ import * as fsp from 'fs/promises'
import path from 'path'
test.describe('Import UI tests', () => {
test('shows toast when trying to sketch on imported face', async ({
test('shows toast when trying to sketch on imported face, and hovering over imported geometry should NOT highlight any code', async ({
context,
page,
homePage,
@ -93,7 +93,6 @@ sketch002 = startSketchOn(extrude001, face = seg01)`
await toolbar.startSketchPlaneSelection()
// Click on a face from the imported model
// await new Promise(() => {})
await importedFaceClick()
// Verify toast appears with correct content

377
e2e/playwright/sketch-tests.spec.ts
View File

@ -1363,73 +1363,6 @@ profile001 = startProfileAt([${roundOff(scale * 69.6)}, ${roundOff(
})
})
test.describe('Sketch mode should be toleratant to syntax errors', () => {
test(
'adding a syntax error, recovers after fixing',
{ tag: ['@skipWin'] },
async ({ page, homePage, context, scene, editor, toolbar }) => {
const file = await fs.readFile(
path.resolve(
__dirname,
'../../',
'./rust/kcl-lib/e2e/executor/inputs/e2e-can-sketch-on-chamfer.kcl'
),
'utf-8'
)
await context.addInitScript((file) => {
localStorage.setItem('persistCode', file)
}, file)
await homePage.goToModelingScene()
const [objClick] = scene.makeMouseHelpers(600, 250)
const arrowHeadLocation = { x: 706, y: 129 } as const
const arrowHeadWhite = TEST_COLORS.WHITE
const backgroundGray: [number, number, number] = [28, 28, 28]
const verifyArrowHeadColor = async (c: [number, number, number]) =>
scene.expectPixelColor(c, arrowHeadLocation, 15)
await test.step('check chamfer selection changes cursor positon', async () => {
await expect(async () => {
// sometimes initial click doesn't register
await objClick()
await editor.expectActiveLinesToBe([
'|> startProfileAt([75.8, 317.2], %) // [$startCapTag, $EndCapTag]',
])
}).toPass({ timeout: 15_000, intervals: [500] })
})
await test.step('enter sketch and sanity check segments have been drawn', async () => {
await toolbar.editSketch()
// this checks sketch segments have been drawn
await verifyArrowHeadColor(arrowHeadWhite)
})
await test.step('Make typo and check the segments have Disappeared and there is a syntax error', async () => {
await editor.replaceCode('line(endAbsolute = [pro', 'badBadBadFn([pro')
await editor.expectState({
activeLines: [],
diagnostics: ['memoryitemkey`badBadBadFn`isnotdefined'],
highlightedCode: '',
})
// this checks sketch segments have failed to be drawn
await verifyArrowHeadColor(backgroundGray)
})
await test.step('', async () => {
await editor.replaceCode('badBadBadFn([pro', 'line(endAbsolute = [pro')
await editor.expectState({
activeLines: [],
diagnostics: [],
highlightedCode: '',
})
// this checks sketch segments have been drawn
await verifyArrowHeadColor(arrowHeadWhite)
})
await page.waitForTimeout(100)
}
)
})
test.describe(`Sketching with offset planes`, () => {
test(`Can select an offset plane to sketch on`, async ({
context,
@ -1623,6 +1556,7 @@ profile002 = startProfileAt([117.2, 56.08], sketch001)
localStorage.setItem(
'persistCode',
`@settings(defaultLengthUnit = in)
sketch001 = startSketchOn(XZ)
profile002 = startProfileAt([40.68, 87.67], sketch001)
|> xLine(length = 239.17)
@ -3109,3 +3043,312 @@ profile001 = startProfileAt([0, 0], sketch001)
)
})
})
test.describe('manual edits during sketch mode', () => {
test('Can edit sketch through feature tree with variable modifications', async ({
page,
context,
homePage,
scene,
editor,
toolbar,
cmdBar,
}) => {
const initialCode = `myVar1 = 5
myVar2 = 6
sketch001 = startSketchOn(XZ)
profile001 = startProfileAt([106.68, 89.77], sketch001)
|> line(end = [132.34, 157.8])
|> line(end = [67.65, -460.55], tag = $seg01)
|> line(endAbsolute = [profileStartX(%), profileStartY(%)])
|> close()
extrude001 = extrude(profile001, length = 500)
sketch002 = startSketchOn(extrude001, face = seg01)
profile002 = startProfileAt([83.39, 329.15], sketch002)
|> angledLine(angle = 0, length = 119.61, tag = $rectangleSegmentA001)
|> angledLine(angle = segAng(rectangleSegmentA001) - 90, length = 156.54, angle = -28)
|> angledLine(
angle = segAng(rectangleSegmentA001),
length = -segLen(rectangleSegmentA001),
angle = -151,
length = 116.27,
)
|> line(endAbsolute = [profileStartX(%), profileStartY(%)])
|> close()
profile003 = startProfileAt([-201.08, 254.17], sketch002)
|> line(end = [103.55, 33.32])
|> line(end = [48.8, -153.54])`
await context.addInitScript((initialCode) => {
localStorage.setItem('persistCode', initialCode)
}, initialCode)
await homePage.goToModelingScene()
await scene.connectionEstablished()
await scene.settled(cmdBar)
const expectSketchOriginToBeDrawn = async () => {
await scene.expectPixelColor(TEST_COLORS.WHITE, { x: 672, y: 193 }, 15)
}
await test.step('Open feature tree and edit second sketch', async () => {
await toolbar.openFeatureTreePane()
const sketchButton = await toolbar.getFeatureTreeOperation('Sketch', 1)
await sketchButton.dblclick()
await page.waitForTimeout(700) // Wait for engine animation
await expectSketchOriginToBeDrawn()
})
await test.step('Add new variable and wait for re-execution', async () => {
await page.waitForTimeout(500) // wait for deferred execution
await editor.replaceCode('myVar2 = 6', 'myVar2 = 6\nmyVar3 = 7')
await page.waitForTimeout(2000) // wait for deferred execution
await expectSketchOriginToBeDrawn()
})
const handle1Location = { x: 843, y: 235 }
await test.step('Edit sketch by dragging handle', async () => {
await page.waitForTimeout(500)
await editor.expectEditor.toContain('length = 156.54, angle = -28')
await page.mouse.move(handle1Location.x, handle1Location.y)
await page.mouse.down()
await page.mouse.move(handle1Location.x + 50, handle1Location.y + 50, {
steps: 5,
})
await page.mouse.up()
await editor.expectEditor.toContain('length = 231.59, angle = -34')
// await page.waitForTimeout(1000) // Wait for update
})
await test.step('Delete variables and wait for re-execution', async () => {
await page.waitForTimeout(500)
await editor.replaceCode('myVar3 = 7', '')
await page.waitForTimeout(50)
await editor.replaceCode('myVar2 = 6', '')
await page.waitForTimeout(2000) // Wait for deferred execution
await expectSketchOriginToBeDrawn()
})
const handle2Location = { x: 872, y: 273 }
await test.step('Edit sketch again', async () => {
await editor.expectEditor.toContain('length = 231.59, angle = -34')
await page.waitForTimeout(500)
await page.mouse.move(handle2Location.x, handle2Location.y)
await page.mouse.down()
await page.mouse.move(handle2Location.x, handle2Location.y - 50, {
steps: 5,
})
await page.mouse.up()
await editor.expectEditor.toContain('length = 167.36, angle = -14')
})
await test.step('add whole other sketch before current sketch', async () => {
await page.waitForTimeout(500)
await editor.replaceCode(
`myVar1 = 5`,
`myVar1 = 5
sketch003 = startSketchOn(XY)
profile004 = circle(sketch003, center = [143.91, 136.89], radius = 71.63)`
)
await page.waitForTimeout(2000) // Wait for deferred execution
await expectSketchOriginToBeDrawn()
})
const handle3Location = { x: 844, y: 212 }
await test.step('edit sketch again', async () => {
await editor.expectEditor.toContain('length = 167.36, angle = -14')
await page.mouse.move(handle3Location.x, handle3Location.y)
await page.mouse.down()
await page.mouse.move(handle3Location.x, handle3Location.y + 110, {
steps: 5,
})
await page.mouse.up()
await editor.expectEditor.toContain('length = 219.2, angle = -56')
})
// exit sketch and assert whole code
await test.step('Exit sketch and assert code', async () => {
await toolbar.exitSketch()
await editor.expectEditor.toContain(
`myVar1 = 5
sketch003 = startSketchOn(XY)
profile004 = circle(sketch003, center = [143.91, 136.89], radius = 71.63)
sketch001 = startSketchOn(XZ)
profile001 = startProfileAt([106.68, 89.77], sketch001)
|> line(end = [132.34, 157.8])
|> line(end = [67.65, -460.55], tag = $seg01)
|> line(endAbsolute = [profileStartX(%), profileStartY(%)])
|> close()
extrude001 = extrude(profile001, length = 500)
sketch002 = startSketchOn(extrude001, face = seg01)
profile002 = startProfileAt([83.39, 329.15], sketch002)
|> angledLine(angle = 0, length = 119.61, tag = $rectangleSegmentA001)
|> angledLine(angle = segAng(rectangleSegmentA001) - 90, length = 219.2, angle = -56)
|> angledLine(
angle = segAng(rectangleSegmentA001),
length = -segLen(rectangleSegmentA001),
angle = -151,
length = 116.27,
)
|> line(endAbsolute = [profileStartX(%), profileStartY(%)])
|> close()
profile003 = startProfileAt([-201.08, 254.17], sketch002)
|> line(end = [103.55, 33.32])
|> line(end = [48.8, -153.54])
`,
{ shouldNormalise: true }
)
await editor.expectState({
activeLines: [],
diagnostics: [],
highlightedCode: '',
})
})
})
test('Will exit out of sketch mode for some incompatible edits', async ({
page,
context,
homePage,
scene,
editor,
toolbar,
cmdBar,
}) => {
const initialCode = `myVar1 = 5
myVar2 = 6
sketch001 = startSketchOn(XZ)
profile001 = startProfileAt([106.68, 89.77], sketch001)
|> line(end = [132.34, 157.8])
|> line(end = [67.65, -460.55], tag = $seg01)
|> line(endAbsolute = [profileStartX(%), profileStartY(%)])
|> close()
extrude001 = extrude(profile001, length = 500)
sketch002 = startSketchOn(extrude001, face = seg01)
profile002 = startProfileAt([83.39, 329.15], sketch002)
|> angledLine(angle = 0, length = 119.61, tag = $rectangleSegmentA001)
|> angledLine(angle = segAng(rectangleSegmentA001) - 90, length = 156.54, angle = -28)
|> angledLine(
angle = segAng(rectangleSegmentA001),
length = -segLen(rectangleSegmentA001),
angle = -151,
length = 116.27,
)
|> line(endAbsolute = [profileStartX(%), profileStartY(%)])
|> close()
profile003 = startProfileAt([-201.08, 254.17], sketch002)
|> line(end = [103.55, 33.32])
|> line(end = [48.8, -153.54])`
await context.addInitScript((initialCode) => {
localStorage.setItem('persistCode', initialCode)
}, initialCode)
await homePage.goToModelingScene()
await scene.connectionEstablished()
await scene.settled(cmdBar)
const expectSketchOriginToBeDrawn = async () => {
await scene.expectPixelColor(TEST_COLORS.WHITE, { x: 672, y: 193 }, 15)
}
await test.step('Open feature tree and edit second sketch', async () => {
await toolbar.openFeatureTreePane()
const sketchButton = await toolbar.getFeatureTreeOperation('Sketch', 1)
await sketchButton.dblclick()
await page.waitForTimeout(700) // Wait for engine animation
await expectSketchOriginToBeDrawn()
})
await test.step('rename variable of current sketch, sketch002 to changeSketchNamePartWayThrough', async () => {
await editor.replaceCode('sketch002', 'changeSketchNamePartWayThrough')
await page.waitForTimeout(100)
// three times to rename the declaration and it's use
await editor.replaceCode('sketch002', 'changeSketchNamePartWayThrough')
await page.waitForTimeout(100)
await editor.replaceCode('sketch002', 'changeSketchNamePartWayThrough')
await expect(
page.getByText('Unable to maintain sketch mode')
).toBeVisible()
})
})
test(
'adding a syntax error, recovers after fixing',
{ tag: ['@skipWin'] },
async ({ page, homePage, context, scene, editor, toolbar, cmdBar }) => {
const file = await fs.readFile(
path.resolve(
__dirname,
'../../',
'./rust/kcl-lib/e2e/executor/inputs/e2e-can-sketch-on-chamfer.kcl'
),
'utf-8'
)
await context.addInitScript((file) => {
localStorage.setItem('persistCode', file)
}, file)
await homePage.goToModelingScene()
const [objClick] = scene.makeMouseHelpers(600, 250)
const arrowHeadLocation = { x: 706, y: 129 } as const
const arrowHeadWhite = TEST_COLORS.WHITE
const backgroundGray: [number, number, number] = [28, 28, 28]
const verifyArrowHeadColor = async (c: [number, number, number]) =>
scene.expectPixelColor(c, arrowHeadLocation, 15)
// wait for scene to load
await scene.settled(cmdBar)
await test.step('check chamfer selection changes cursor positon', async () => {
await expect(async () => {
// sometimes initial click doesn't register
await objClick()
await editor.expectActiveLinesToBe([
'|> startProfileAt([75.8, 317.2], %) // [$startCapTag, $EndCapTag]',
])
}).toPass({ timeout: 15_000, intervals: [500] })
})
await test.step('enter sketch and sanity check segments have been drawn', async () => {
await toolbar.editSketch()
// this checks sketch segments have been drawn
await verifyArrowHeadColor(arrowHeadWhite)
})
await test.step('Make typo and check the segments have Disappeared and there is a syntax error', async () => {
await editor.replaceCode(
'line(endAbsolute = [pro',
'badBadBadFn(endAbsolute = [pro'
)
await editor.expectState({
activeLines: [],
diagnostics: ['memoryitemkey`badBadBadFn`isnotdefined'],
highlightedCode: '',
})
await expect(
page.getByText(
"Error in kcl script, sketch cannot be drawn until it's fixed"
)
).toBeVisible()
// this checks sketch segments have failed to be drawn
await verifyArrowHeadColor(backgroundGray)
})
await test.step('', async () => {
await editor.replaceCode(
'badBadBadFn(endAbsolute = [pro',
'line(endAbsolute = [pro'
)
await editor.expectState({
activeLines: [],
diagnostics: [],
highlightedCode: '',
})
// this checks sketch segments have been drawn
await verifyArrowHeadColor(arrowHeadWhite)
})
await page.waitForTimeout(100)
}
)
})

17
src/components/ModelingMachineProvider.tsx
View File

@ -551,6 +551,9 @@ export const ModelingMachineProvider = ({
if (selectionRanges.graphSelections.length <= 0) return false
return true
},
'is-error-free': () => {
return kclManager.errors.length === 0 && !kclManager.hasErrors()
},
'Selection is on face': ({ context: { selectionRanges }, event }) => {
if (event.type !== 'Enter sketch') return false
if (event.data?.forceNewSketch) return false
@ -854,6 +857,7 @@ export const ModelingMachineProvider = ({
: plane?.pathIds[0]
let sketch: KclValue | null = null
let planeVar: Plane | null = null
for (const variable of Object.values(
kclManager.execState.variables
)) {
@ -884,6 +888,7 @@ export const ModelingMachineProvider = ({
planeVar = variable.value
}
}
if (!sketch || sketch.type !== 'Sketch') {
if (artifact?.type !== 'plane')
return Promise.reject(new Error('No sketch'))
@ -1911,18 +1916,6 @@ export const ModelingMachineProvider = ({
}
}, [modelingActor])
useEffect(() => {
kclManager.registerExecuteCallback(() => {
modelingSend({ type: 'Re-execute' })
})
// Before this component unmounts, call the 'Cancel'
// event to clean up any state in the modeling machine.
return () => {
modelingSend({ type: 'Cancel' })
}
}, [modelingSend])
// Give the state back to the editorManager.
useEffect(() => {
editorManager.modelingSend = modelingSend

6
src/hooks/useEngineConnectionSubscriptions.ts
View File

@ -200,9 +200,8 @@ export function useEngineConnectionSubscriptions() {
)
if (!err(extrusion)) {
if (!isTopLevelModule(extrusion.codeRef.range)) {
const fileIndex = getModuleId(extrusion.codeRef.range)
const importDetails =
kclManager.execState.filenames[fileIndex]
const moduleId = getModuleId(extrusion.codeRef.range)
const importDetails = kclManager.execState.filenames[moduleId]
if (!importDetails) {
toast.error("can't sketch on this face")
return
@ -217,6 +216,7 @@ export function useEngineConnectionSubscriptions() {
) {
toast.error("can't sketch on this face")
} else {
// force tsc error if more cases are added
const _exhaustiveCheck: never = importDetails
}
}

52
src/lang/KclSingleton.ts
View File

@ -27,7 +27,6 @@ import type {
ExecState,
PathToNode,
Program,
SourceRange,
VariableMap,
} from '@src/lang/wasm'
import { emptyExecState, getKclVersion, parse, recast } from '@src/lang/wasm'
@ -48,7 +47,7 @@ import type {
import { jsAppSettings } from '@src/lib/settings/settingsUtils'
import { err, reportRejection } from '@src/lib/trap'
import { deferExecution, isOverlap, uuidv4 } from '@src/lib/utils'
import { deferExecution, uuidv4 } from '@src/lib/utils'
interface ExecuteArgs {
ast?: Node<Program>
@ -88,6 +87,21 @@ export class KclManager {
preComments: [],
commentStart: 0,
}
_lastAst: Node<Program> = {
body: [],
shebang: null,
start: 0,
end: 0,
moduleId: 0,
nonCodeMeta: {
nonCodeNodes: {},
startNodes: [],
},
innerAttrs: [],
outerAttrs: [],
preComments: [],
commentStart: 0,
}
private _execState: ExecState = emptyExecState()
private _variables: VariableMap = {}
lastSuccessfulVariables: VariableMap = {}
@ -115,13 +129,16 @@ export class KclManager {
private _kclErrorsCallBack: (errors: KCLError[]) => void = () => {}
private _diagnosticsCallback: (errors: Diagnostic[]) => void = () => {}
private _wasmInitFailedCallback: (arg: boolean) => void = () => {}
private _executeCallback: () => void = () => {}
sceneInfraBaseUnitMultiplierSetter: (unit: BaseUnit) => void = () => {}
get ast() {
return this._ast
}
set ast(ast) {
if (this._ast.body.length !== 0) {
// last intact ast, if the user makes a typo with a syntax error, we want to keep the one before they made that mistake
this._lastAst = structuredClone(this._ast)
}
this._ast = ast
this._astCallBack(ast)
}
@ -242,6 +259,8 @@ export class KclManager {
await this.safeParse(this.singletons.codeManager.code).then((ast) => {
if (ast) {
this.ast = ast
// on setup, set _lastAst so it's populated.
this._lastAst = ast
}
})
})
@ -272,9 +291,6 @@ export class KclManager {
this._isExecutingCallback = setIsExecuting
this._wasmInitFailedCallback = setWasmInitFailed
}
registerExecuteCallback(callback: () => void) {
this._executeCallback = callback
}
clearAst() {
this._ast = {
@ -338,24 +354,6 @@ export class KclManager {
}
}
// Some "objects" have the same source range, such as sketch_mode_start and start_path.
// So when passing a range, we need to also specify the command type
private mapRangeToObjectId(
range: SourceRange,
commandTypeToTarget: string
): string | undefined {
for (const [artifactId, artifact] of this.artifactGraph) {
if (
'codeRef' in artifact &&
artifact.codeRef &&
isOverlap(range, artifact.codeRef.range)
) {
if (commandTypeToTarget === artifact.type) return artifactId
}
}
return undefined
}
async safeParse(code: string): Promise<Node<Program> | null> {
const result = parse(code)
this.diagnostics = []
@ -474,10 +472,9 @@ export class KclManager {
this.lastSuccessfulVariables = execState.variables
this.lastSuccessfulOperations = execState.operations
}
this.ast = { ...ast }
this.ast = structuredClone(ast)
// updateArtifactGraph relies on updated executeState/variables
await this.updateArtifactGraph(execState.artifactGraph)
this._executeCallback()
if (!isInterrupted) {
this.singletons.sceneInfra.modelingSend({
type: 'code edit during sketch',
@ -557,8 +554,7 @@ export class KclManager {
return
}
this.ast = { ...ast }
return this.executeAst()
return this.executeAst({ ast })
}
async format() {

56
src/lang/queryAst.ts
View File

@ -996,3 +996,59 @@ function pathToNodeKeys(pathToNode: PathToNode): (string | number)[] {
export function stringifyPathToNode(pathToNode: PathToNode): string {
return JSON.stringify(pathToNodeKeys(pathToNode))
}
/**
* Updates PathToNodes to account for changes in body indices when variables are added/removed above
* This is specifically for handling the common case where a user adds/removes variables above a node,
* which changes the body index in the PathToNode
* @param oldAst The AST before user edits
* @param newAst The AST after user edits
* @param pathToUpdate Array of PathToNodes that need to be updated
* @returns updated PathToNode, or Error if any path couldn't be updated
*/
export function updatePathToNodesAfterEdit(
oldAst: Node<Program>,
newAst: Node<Program>,
pathToUpdate: PathToNode
): PathToNode | Error {
// First, let's find all topLevel the variable declarations in both ASTs
// and map their name to their body index
const oldVarDecls = new Map<string, number>()
const newVarDecls = new Map<string, number>()
const maxBodyLength = Math.max(oldAst.body.length, newAst.body.length)
for (let bodyIndex = 0; bodyIndex < maxBodyLength; bodyIndex++) {
const oldNode = oldAst.body[bodyIndex]
const newNode = newAst.body[bodyIndex]
if (oldNode?.type === 'VariableDeclaration') {
oldVarDecls.set(oldNode.declaration.id.name, bodyIndex)
}
if (newNode?.type === 'VariableDeclaration') {
newVarDecls.set(newNode.declaration.id.name, bodyIndex)
}
}
// For the path, get the variable name this path points to
const oldNodeResult = getNodeFromPath<VariableDeclaration>(
oldAst,
pathToUpdate,
'VariableDeclaration'
)
if (err(oldNodeResult)) return oldNodeResult
const oldNode = oldNodeResult.node
const varName = oldNode.declaration.id.name
console.log('varName', varName)
// Find the old and new indices for this variable
const oldIndex = oldVarDecls.get(varName)
const newIndex = newVarDecls.get(varName)
if (oldIndex === undefined || newIndex === undefined) {
return new Error(`Could not find variable ${varName} in one of the ASTs`)
}
// Create a new path with the updated body index
const newPath = structuredClone(pathToUpdate)
newPath[1][0] = newIndex // Update the body index
return newPath
}

2
src/lang/std/artifactGraph.ts
View File

@ -738,7 +738,7 @@ const onlyConsecutivePaths = (
}
export function getPathsFromPlaneArtifact(
planeArtifact: PlaneArtifact,
planeArtifact: PlaneArtifact | WallArtifact | CapArtifact,
artifactGraph: ArtifactGraph,
ast: Program
): PathToNode[] {

272
src/machines/modelingMachine.ts
View File

File diff suppressed because one or more lines are too long