Add offset plane point-and-click user flow #4552
Conversation
(because default planes don't have one)
Struggling to get local electron test suite running properly
franknoirot
requested review from
jtran,
lf94,
nadr0,
pierremtb and
Irev-Dev
|
QA Wolf here! As you write new code it's important that your test coverage is keeping up. |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
|
|
nitpick, does the icon for offset plane look disabled? At first I thought it wasn't enabled. |
I couldn't use the cmdBar fixture with an electron test for some reason.
| @@ -35,7 +35,7 @@ export class CmdBarFixture { | |||
| } | |||
|
|
|||
| private _serialiseCmdBar = async (): Promise<CmdBarSerialised> => { | |||
| const reviewForm = await this.page.locator('#review-form') | |||
| const reviewForm = this.page.locator('#review-form') | |||
There was a problem hiding this comment.
Playwright locators are not async in and of themselves. Only awaited expectations from them or things like .innerText() are async.
There was a problem hiding this comment.
Now that I know my issue was writing the electron version of this test I can extend it if we need. Should I do any combination of these?
- Create offsets from other planes
- Create variables for the distances of multiple planes to test the insertion order
- Sketch on an offset plane after creating it
| commandBarState.context.currentArgument?.inputType === 'selection' | ||
| ) | ||
| ) | ||
| return |
There was a problem hiding this comment.
This should live somewhere more centralized when we have an actor-based state system really driving
| programMemory: ProgramMemory, | ||
| engineCommandManager: EngineCommandManager | ||
| ) { | ||
| // eslint-disable-next-line @typescript-eslint/no-floating-promises | ||
| programMemory.hasSketchOrSolid() && |
There was a problem hiding this comment.
This check acted as a second blocker to default plane selections for the offset plane command bar, duplicate from the one found in Stream.tsx
| @@ -527,6 +527,45 @@ export function sketchOnExtrudedFace( | |||
| } | |||
| } | |||
|
|
|||
| /** | |||
| * Append an offset plane to the AST | |||
There was a problem hiding this comment.
It appends because there is no concern of inserting it intelligently after a specific item, but you can reference named constants for the distance value, so the end is the easiest place to add it.
| const varInfo = findAllPreviousVariables( | ||
| kclManager.ast, | ||
| kclManager.programMemory, | ||
| selectionRange | ||
| // If there is no selection, use the end of the code | ||
| selectionRange || [code.length, code.length] |
There was a problem hiding this comment.
This hook wouldn't work if nothing was selected within the code, which is true of operations like offset plane. I still can't get the autocompletions to work in the KCL argument input box though.
|
@jtran figured out that my TSC issues are because we're running |
| @@ -536,7 +560,8 @@ export function canSweepSelection(selection: Selections) { | |||
| } | |||
|
|
|||
| // This accounts for non-geometry selections under "other" | |||
| export type ResolvedSelectionType = [Artifact['type'] | 'other', number] | |||
| export type ResolvedSelectionType = Artifact['type'] | 'other' | |||
There was a problem hiding this comment.
is other still needed, not sure it ever get's used now?
There was a problem hiding this comment.
I used it for an Axis selection in getSelectionCountByType(), which doesn't have an Artifact type that I could see.
Super fair point, let me try a different one real quick |
| const pathToNode: PathToNode = [ | ||
| ['body', ''], | ||
| [modifiedAst.body.length - 1, 'index'], | ||
| ['declarations', 'VariableDeclaration'], | ||
| ['0', 'index'], | ||
| ['init', 'VariableDeclarator'], | ||
| ['arguments', 'CallExpression'], | ||
| [0, 'index'], | ||
| ] |
There was a problem hiding this comment.
I hate how we assume AST structure all over like this. I guess it doesn't have a source range, so we can't call getNodePathFromSourceRange(). So I don't have a better suggestion.
Closes #4468. Users can now use the Offset plane tool in the modeling mode toolbar or the corresponding command palette entry. They must select a default plane to sketch on, then enter an offset distance value.
Adds one new E2E playwright test. I couldn't make it an electron and fixture-based test, because I ran into issues with the selectors within
cmdBar.pagenever being correct in the electron context. I am unsure if this is because I attempted to add this as the first electron test in thepoint-and-click.spec.tsfile, and I was missing something about the imports orbeforeEachthat was needed for that to work, but I've made it a browser playwright test for now. @lf94 has a big migration of all our tests to electron right now, so I imagine a lot of test infra stuff will be shifted slightly in the near future that I can use to swap it back over to electron without much trouble.Limitations
One important note is that we'd like to treat the default planes differently in the near future (see #4504), so that will impact and hopefully simplify some of the special case code needed to handle selection of default planes. I had to extend the
Selectionstype, and add default planes as a possibleotherSelectionsin addition to theAxistype currently used in some sketch mode code. I've grouped them under a"NonCodeSelections"type, but some of the strategies proposed for #4504 have included actually including the default planes in the code as a sort of prelude, so further tweaks may be necessary.Demo
Screenshare.-.2024-11-22.6_03_49.PM-compressed.mp4