Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hook up chamfer UI with AST-mod #4694

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Hook up chamfer UI with AST-mod #4694

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
90e1351
button
max-mrgrsk Dec 8, 2024
1f18c59
config
max-mrgrsk Dec 8, 2024
2a1f60f
hook up with ast
max-mrgrsk Dec 8, 2024
fc4c612
cmd bar test
max-mrgrsk Dec 8, 2024
c6e819d
button states fix and test
max-mrgrsk Dec 8, 2024
31a5d00
little naming fix
max-mrgrsk Dec 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 43 additions & 0 deletions e2e/playwright/command-bar-tests.spec.ts
View file
Open in desktop
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[optional] we can come back and rewrite this test in our new fixture-based approach later, but since I'm already asking you to switch out the XState actor to an action I figure I'd give you a heads up on this one too.

Pierre has a good example of a Playwright test that uses our new fixture-based approach here in his Shell PR, and we're going to try to make any new tests use it, not least of all because Lee's work on migrating our tests to Electron uses it and then all our tests can run on both desktop and browser at-will.

I can show you how to rewrite this test in the new style if you need a hand or anything, although I think it would be pretty trivial for you!

Copy link
Collaborator Author

@max-mrgrsk max-mrgrsk Dec 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good poits, thanks. Is it ok if I'll do that in the following pr so I can tackle one thing at a time ? Then I can fix both chamfer and fillet in one batch

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've seen Pierre's states too and was curious about it. Thanks for clarifying:)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@franknoirot I've created an issue for it (#4749). The current chamfer PR will wait until the older fillet code is updated to the latest XState requirements. Once that's done, I'll base current chamfer PR on the updated code so we don't mix everything into a single pull request.

Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,49 @@ extrude001 = extrude(-10, sketch001)`
)
})

test('Chamfer from command bar', async ({ page }) => {
await page.addInitScript(async () => {
localStorage.setItem(
'persistCode',
`sketch001 = startSketchOn('XY')
|> startProfileAt([-5, -5], %)
|> line([0, 10], %)
|> line([10, 0], %)
|> line([0, -10], %)
|> lineTo([profileStartX(%), profileStartY(%)], %, $seg01)
|> close(%)
extrude001 = extrude(-10, sketch001)
|> fillet({
radius = 3,
tags = [getNextAdjacentEdge(seg01)]
}, %)`
)
})

const u = await getUtils(page)
await page.setViewportSize({ width: 1000, height: 500 })
await u.waitForAuthSkipAppStart()
await u.openDebugPanel()
await u.expectCmdLog('[data-message-type="execution-done"]')
await u.closeDebugPanel()

const selectSegment = () => page.getByText(`line([0, -10], %)`).click()

await selectSegment()
await page.waitForTimeout(100)
await page.getByRole('button', { name: 'Chamfer' }).click()
await page.waitForTimeout(100)
await page.keyboard.press('Enter') // skip selection
await page.waitForTimeout(100)
await page.keyboard.press('Enter') // accept default radius
await page.waitForTimeout(100)
await page.keyboard.press('Enter') // submit
await page.waitForTimeout(100)
await expect(page.locator('.cm-activeLine')).toContainText(
`chamfer({ length = ${KCL_DEFAULT_LENGTH}, tags = [seg02] }, %)`
)
})

test('Command bar can change a setting, and switch back and forth between arguments', async ({
page,
}) => {
Expand Down
11 changes: 8 additions & 3 deletions e2e/playwright/testing-selections.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -904,7 +904,7 @@ sketch002 = startSketchOn(extrude001, $seg01)
).not.toBeDisabled()
})

test('Fillet button states test', async ({ page }) => {
test('Fillet and Chamfer button states test', async ({ page }) => {
const u = await getUtils(page)
await page.addInitScript(async () => {
localStorage.setItem(
Expand All @@ -929,23 +929,28 @@ sketch002 = startSketchOn(extrude001, $seg01)
const selectClose = () => page.getByText(`close(%)`).click()
const clickEmpty = () => page.mouse.click(950, 100)

// expect fillet button without any bodies in the scene
// test without any bodies in the scene
await selectSegment()
await expect(page.getByRole('button', { name: 'Fillet' })).toBeDisabled()
await expect(page.getByRole('button', { name: 'Chamfer' })).toBeDisabled()
await clickEmpty()
await expect(page.getByRole('button', { name: 'Fillet' })).toBeDisabled()
await expect(page.getByRole('button', { name: 'Chamfer' })).toBeDisabled()

// test fillet button with the body in the scene
// add a body and retest
const codeToAdd = `${await u.codeLocator.allInnerTexts()}
extrude001 = extrude(10, sketch001)`
await u.codeLocator.clear()
await u.codeLocator.fill(codeToAdd)
await selectSegment()
await expect(page.getByRole('button', { name: 'Fillet' })).toBeEnabled()
await expect(page.getByRole('button', { name: 'Chamfer' })).toBeEnabled()
await selectClose()
await expect(page.getByRole('button', { name: 'Fillet' })).toBeDisabled()
await expect(page.getByRole('button', { name: 'Chamfer' })).toBeDisabled()
await clickEmpty()
await expect(page.getByRole('button', { name: 'Fillet' })).toBeEnabled()
await expect(page.getByRole('button', { name: 'Chamfer' })).toBeEnabled()
})

const removeAfterFirstParenthesis = (inputString: string) => {
Expand Down
29 changes: 27 additions & 2 deletions src/lib/commandBarConfigs/modelingCommandConfig.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,10 +39,13 @@ export type ModelingCommandSchema = {
angle: KclCommandValue
}
Fillet: {
// todo
selection: Selections
radius: KclCommandValue
}
Chamfer: {
selection: Selections
length: KclCommandValue
}
'Offset plane': {
plane: Selections
distance: KclCommandValue
Expand Down Expand Up @@ -317,7 +320,7 @@ export const modelingMachineCommandConfig: StateMachineCommandSetConfig<
},
Fillet: {
description: 'Fillet edge',
icon: 'fillet',
icon: 'fillet3d',
status: 'development',
needsReview: true,
args: {
Expand All @@ -337,6 +340,28 @@ export const modelingMachineCommandConfig: StateMachineCommandSetConfig<
},
},
},
Chamfer: {
description: 'Chamfer edge',
icon: 'chamfer3d',
status: 'development',
needsReview: true,
args: {
selection: {
inputType: 'selection',
selectionTypes: ['segment', 'sweepEdge', 'edgeCutEdge'],
multiple: true,
required: true,
skip: false,
warningMessage:
'Chamfers cannot touch other chamfers yet. This is under development.',
},
length: {
inputType: 'kcl',
defaultValue: KCL_DEFAULT_LENGTH,
required: true,
},
},
},
'Text-to-CAD': {
description: 'Use the Zoo Text-to-CAD API to generate part starters.',
icon: 'chat',
Expand Down
9 changes: 7 additions & 2 deletions src/lib/toolbar.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,9 +180,14 @@ export const toolbarConfig: Record<ToolbarModeName, ToolbarMode> = {
},
{
id: 'chamfer',
onClick: () => console.error('Chamfer not yet implemented'),
onClick: ({ commandBarSend }) =>
commandBarSend({
type: 'Find and select command',
data: { name: 'Chamfer', groupId: 'modeling' },
}),
icon: 'chamfer3d',
status: 'kcl-only',
status: DEV ? 'available' : 'kcl-only',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Not specific to this PR, no change requested here)

@franknoirot I think we might want to start checking against another variable than DEV here? Something that could resolve to true on app.zoo.dev and nightly builds, but not release builds. Seems to me that DEV might be a tad too restrictive here. But I could be wrong too.

Copy link
Collaborator Author

@max-mrgrsk max-mrgrsk Dec 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! I’d be happy to follow this path for fillets and chamfers while waiting for the engine team to unblock them for revolves.

Overall, I think shipping semi-working features for nightly builds could be a good practice, as we have enough warning messages in place. Including the Command Bar UI warnings. We could add an extra indication for the buttons that are "nightly only".

Besides, DEV blocks part of our team like Nick and Josh from checking and testing new stuff.

On the other hand, it doesn’t fully align with the general rule: “Don’t ship shit.” : D

@jessfraz, @Irev-Dev , what do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this idea, we should add another environment variable for this. @pierremtb would you be able to do that?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For sure! And should be a separate PR too

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created #4709

disabled: (state) => !state.can({ type: 'Chamfer' }),
title: 'Chamfer',
hotkey: 'C',
description: 'Bevel the edges of a 3D solid.',
Expand Down
33 changes: 33 additions & 0 deletions src/machines/modelingMachine.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ import {
} from 'lang/modifyAst'
import {
applyEdgeTreatmentToSelection,
ChamferParameters,
EdgeTreatmentType,
FilletParameters,
} from 'lang/modifyAst/addEdgeTreatment'
Expand Down Expand Up @@ -262,6 +263,7 @@ export type ModelingMachineEvent =
| { type: 'Loft'; data?: ModelingCommandSchema['Loft'] }
| { type: 'Revolve'; data?: ModelingCommandSchema['Revolve'] }
| { type: 'Fillet'; data?: ModelingCommandSchema['Fillet'] }
| { type: 'Chamfer'; data?: ModelingCommandSchema['Chamfer'] }
| { type: 'Offset plane'; data: ModelingCommandSchema['Offset plane'] }
| { type: 'Text-to-CAD'; data: ModelingCommandSchema['Text-to-CAD'] }
| {
Expand Down Expand Up @@ -768,6 +770,30 @@ export const modelingMachine = setup({
// eslint-disable-next-line @typescript-eslint/no-floating-promises
codeManager.updateEditorWithAstAndWriteToFile(kclManager.ast)
},
'AST chamfer': ({ event }) => {
if (event.type !== 'Chamfer') return
if (!event.data) return

// Extract inputs
const ast = kclManager.ast
const { selection, length } = event.data
const parameters: ChamferParameters = {
type: EdgeTreatmentType.Chamfer,
length,
}

// Apply chamfer to selection
const applyEdgeTreatmentToSelectionResult = applyEdgeTreatmentToSelection(
ast,
selection,
parameters
)
if (err(applyEdgeTreatmentToSelectionResult))
return applyEdgeTreatmentToSelectionResult

// eslint-disable-next-line @typescript-eslint/no-floating-promises
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're trying to prevent using these ESLint disabling comments in any new code. Would you mind converting this AST chamfer from an XState action, which is inherently synchronous, to an actor, which can be asynchronous so that we can appropriately await things like this?

There is a fresh example here from Pierre's shell PR. As you can see, it requires adding a bit of extra code and another state for the machine to sit in while the async code runs. I'm happy to pair with you or do a video walkthrough of Pierre's code if you need!

Then we can go back and refactor the Fillet code in another small PR.

codeManager.updateEditorWithAstAndWriteToFile(kclManager.ast)
},
'set selection filter to curves only': () => {
;(async () => {
await engineCommandManager.sendSceneCommand({
Expand Down Expand Up @@ -1637,6 +1663,13 @@ export const modelingMachine = setup({
reenter: false,
},

Chamfer: {
target: 'idle',
guard: 'has valid edge treatment selection',
actions: ['AST chamfer'],
reenter: false,
},

Export: {
target: 'idle',
reenter: false,
Expand Down
Loading