Add rectangle tool to sketch mode #2005
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Sweet! |
|
Pretty sure I was able to resolve the conflicts. |
There was a problem hiding this comment.
Oh right, https://github.com/KittyCAD/modeling-app/pull/1995/files#r1549121189 hasn't been merged, so maybe this will be enough 🤷
|
Alright I've taken a bit more of a look at this and I think I've got my head around it enough to ask you some questions if you have some spare time to pair soon @Irev-Dev. I think that I need to make use of Or if it would save us time I'm happy for you to wrap this up and I'll learn your approach, you probably know just what to look for. |
|
Alright I got this working, the only thing that I need to talk to you about now is how I should handle if a user tries to use the Rectangle tool in a sketch with content already. Because we make everything in sketches one PipeExpression this is problematic, so maybe for now we only allow use of the Rectangle tool if you have an empty sketch? |
|
I don't like that though |
|
Sorry I think you need to rebase, there seems to unrelated changes in here. |
| sketchGroup | ||
| ) | ||
| const originalPathToNodeStr = JSON.stringify(segPathToNode) | ||
| segPathToNode[1][0] = varDecIndex | ||
| const pathToNodeStr = JSON.stringify(segPathToNode) | ||
| // more hacks to hopefully be solved by proper pathToNode info in memory/sketchGroup segments | ||
| const group = | ||
| this.activeSegments[pathToNodeStr] || | ||
| this.activeSegments[originalPathToNodeStr] | ||
| // const prevSegment = sketchGroup.slice(index - 1)[0] | ||
| const type = group?.userData?.type | ||
| const factor = | ||
| (sceneInfra.camControls.camera instanceof OrthographicCamera | ||
| ? orthoFactor | ||
| : perspScale(sceneInfra.camControls.camera, group)) / | ||
| sceneInfra._baseUnitMultiplier | ||
| if (type === TANGENTIAL_ARC_TO_SEGMENT) { | ||
| this.updateTangentialArcToSegment({ | ||
| prevSegment: sgPaths[index - 1], | ||
| from: segment.from, | ||
| to: segment.to, | ||
| group: group, | ||
| scale: factor, | ||
| }) | ||
| } else if (type === STRAIGHT_SEGMENT) { | ||
| this.updateStraightSegment({ | ||
| from: segment.from, | ||
| to: segment.to, | ||
| group: group, | ||
| scale: factor, | ||
| }) | ||
| } else if (type === PROFILE_START) { | ||
| group.position.set(segment.from[0], segment.from[1], 0) | ||
| group.scale.set(factor, factor, factor) | ||
| } | ||
| } | ||
| updateSegment(sketchGroup.start, 0) | ||
| sgPaths.forEach(updateSegment) | ||
| ) | ||
| })() | ||
| } | ||
|
|
||
| /** | ||
| * Update the THREEjs sketch entities with new segment data | ||
| * mapping them back to the AST | ||
| * @param segment | ||
| * @param index | ||
| * @param varDecIndex | ||
| * @param modifiedAst | ||
| * @param orthoFactor | ||
| * @param sketchGroup | ||
| */ | ||
| updateSegment = ( | ||
| segment: Path | SketchGroup['start'], | ||
| index: number, | ||
| varDecIndex: number, | ||
| modifiedAst: Program, | ||
| orthoFactor: number, | ||
| sketchGroup: SketchGroup | ||
| ) => { | ||
| const segPathToNode = getNodePathFromSourceRange( | ||
| modifiedAst, | ||
| segment.__geoMeta.sourceRange | ||
| ) | ||
| const sgPaths = sketchGroup.value | ||
| const originalPathToNodeStr = JSON.stringify(segPathToNode) | ||
| segPathToNode[1][0] = varDecIndex | ||
| const pathToNodeStr = JSON.stringify(segPathToNode) | ||
| // more hacks to hopefully be solved by proper pathToNode info in memory/sketchGroup segments | ||
| const group = | ||
| this.activeSegments[pathToNodeStr] || | ||
| this.activeSegments[originalPathToNodeStr] | ||
| // const prevSegment = sketchGroup.slice(index - 1)[0] | ||
| const type = group?.userData?.type | ||
| const factor = | ||
| (sceneInfra.camControls.camera instanceof OrthographicCamera | ||
| ? orthoFactor | ||
| : perspScale(sceneInfra.camControls.camera, group)) / | ||
| sceneInfra._baseUnitMultiplier | ||
| if (type === TANGENTIAL_ARC_TO_SEGMENT) { | ||
| this.updateTangentialArcToSegment({ | ||
| prevSegment: sgPaths[index - 1], | ||
| from: segment.from, | ||
| to: segment.to, | ||
| group: group, | ||
| scale: factor, | ||
| }) | ||
| } else if (type === STRAIGHT_SEGMENT) { | ||
| this.updateStraightSegment({ | ||
| from: segment.from, | ||
| to: segment.to, | ||
| group, | ||
| scale: factor, | ||
| }) | ||
| } else if (type === PROFILE_START) { | ||
| group.position.set(segment.from[0], segment.from[1], 0) | ||
| group.scale.set(factor, factor, factor) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
This is an annoying diff to read but what happened here is updateSegment was a little helper function inside setupSketchIdleCallbacks, but in order to make use of it up in setupDraftRectangle we had to pull it out and make it a top-level method on the class.
| await expect(page.locator('.cm-content')).toHaveText( | ||
| `const part001 = startSketchOn('-XZ') | ||
| |> startProfileAt([18.2, 5.98], %) | ||
| |> angledLine([180, 9.14], %, 'rectangleSegmentA001') | ||
| |> angledLine([ | ||
| segAng('rectangleSegmentA001', %) + 90, | ||
| 18.2 | ||
| ], %, 'rectangleSegmentB001') | ||
| |> angledLine([ | ||
| segAng('rectangleSegmentA001', %), | ||
| -segLen('rectangleSegmentA001', %) | ||
| ], %, 'rectangleSegmentC001') | ||
| |> close(%)` | ||
| ) | ||
|
|
||
| await expect(page).toHaveScreenshot({ | ||
| maxDiffPixels: 100, | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Trying to think about the philosophy of when we use snap shots, and I think it should be, "verify things visually when we can verify it in other ways".
And to that end asserting that the code here is something very specific makes me think that we should not need a screen shot, since we should be pretty confident that a simple sketch code like this will show up correctly in the app.
HOWEVER,
The draft lines before the second click might need verifying visually because there's no code in the app for that, and a check for "is the draft rectangle animating correctly?"
So maybe a better test is
await page.mouse.click(startXPx + PUR * 20, 500 - PUR * 30)
await page.mouse.move(startXPx + PUR * 10, 500 - PUR * 10, {steps: 5})
// take screenshot here!
await page.mouse.click(startXPx + PUR * 10, 500 - PUR * 10)
await expect(page.locator('.cm-content')).toHaveText( //...Food for thought, up to you.
There was a problem hiding this comment.
Great point, I'll make this change
There was a problem hiding this comment.
quick bug explanation for 7c35109
Screenshare.-.2024-04-19.7_28_24.AM.mp4
Potential changes are
- should rectangle tool be selectable without deselecting line tool? (mentioned in the vid above)
- A suggestion about the snapshot test
But nothing I feel so strongly about that should block the merge.
Really happy with the both the UI and the code!
lol I had reverted that bitbecause I thought it was what you meant by unrelated code 😆. Sorry I should have tested after that change! |
Resolves #1500. Adds a rectangle tool to sketch mode.
Demo
Screenshare.-.2024-04-17.8_42_11.PM-compressed.mp4