-
Notifications
You must be signed in to change notification settings - Fork 119
Fix sketchOnFace point&click for booleans #6713
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.
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
Changes from 2 commits
d994b18
83a951f
09a9613
a0cfa6e
614bfe7
d9b8b72
5cab762
649e934
2876a6c
1ff012f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -34,6 +34,7 @@ import type { | |
| ExtrudeFacePlane, | ||
| } from '@src/machines/modelingMachine' | ||
| import toast from 'react-hot-toast' | ||
| import { findAllChildrenAndOrderByPlaceInCode } from '@src/lang/modifyAst/boolean' | ||
|
|
||
| export function useEngineConnectionSubscriptions() { | ||
| const { send, context, state } = useModelingContext() | ||
|
|
@@ -146,7 +147,7 @@ export function useEngineConnectionSubscriptions() { | |
| } | ||
|
|
||
| sceneInfra.modelingSend({ | ||
| type: 'Select default plane', | ||
| type: 'Select sketch plane', | ||
| data: { | ||
| type: 'defaultPlane', | ||
| planeId: planeId, | ||
|
|
@@ -163,7 +164,7 @@ export function useEngineConnectionSubscriptions() { | |
| const planeInfo = | ||
| await sceneEntitiesManager.getFaceDetails(planeOrFaceId) | ||
| sceneInfra.modelingSend({ | ||
| type: 'Select default plane', | ||
| type: 'Select sketch plane', | ||
| data: { | ||
| type: 'offsetPlane', | ||
| zAxis: [ | ||
|
|
@@ -192,7 +193,7 @@ export function useEngineConnectionSubscriptions() { | |
| return | ||
| } | ||
|
|
||
| // Artifact is likely an extrusion face | ||
| // Artifact is likely an sweep face | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Little bit of clean up, using the more generic term. |
||
| const faceId = planeOrFaceId | ||
| const extrusion = getSweepFromSuspectedSweepSurface( | ||
| faceId, | ||
|
|
@@ -316,15 +317,30 @@ export function useEngineConnectionSubscriptions() { | |
| } | ||
| : { type: 'wall' } | ||
|
|
||
| if (err(extrusion)) { | ||
| return Promise.reject( | ||
| new Error(`Extrusion is not a valid artifact: ${extrusion}`) | ||
| ) | ||
| } | ||
|
|
||
| const lastChild = findAllChildrenAndOrderByPlaceInCode( | ||
| { type: 'sweep', ...extrusion }, | ||
| kclManager.artifactGraph | ||
| )[0] | ||
| const lastChildCodeRef = | ||
| lastChild.type === 'compositeSolid' | ||
| ? lastChild.codeRef.range | ||
| : null | ||
|
Irev-Dev marked this conversation as resolved.
Outdated
|
||
|
|
||
| const extrudePathToNode = !err(extrusion) | ||
| ? getNodePathFromSourceRange( | ||
| kclManager.ast, | ||
| extrusion.codeRef.range | ||
| lastChildCodeRef || extrusion.codeRef.range | ||
| ) | ||
| : [] | ||
|
|
||
| sceneInfra.modelingSend({ | ||
| type: 'Select default plane', | ||
| type: 'Select sketch plane', | ||
| data: { | ||
| type: 'extrudeFace', | ||
| zAxis: [z_axis.x, z_axis.y, z_axis.z], | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -167,6 +167,26 @@ export function findAllChildrenAndOrderByPlaceInCode( | |
| artifact: Artifact, | ||
| artifactGraph: ArtifactGraph | ||
| ): Artifact[] { | ||
| const pathToCompositeSolidMap: { [key: string]: string[] } = {} | ||
| for (const [id, artifact] of artifactGraph) { | ||
| if (artifact.type === 'compositeSolid') { | ||
| for (const pathId of artifact.solidIds) { | ||
| if (pathToCompositeSolidMap[pathId]) { | ||
| pathToCompositeSolidMap[pathId].push(id) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it possible for a path to get used in multiple CSG ops? If so,
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, but not yet I don't think, it would be the |
||
| } else { | ||
| pathToCompositeSolidMap[pathId] = [id] | ||
| } | ||
| } | ||
| for (const pathId of artifact.toolIds) { | ||
| if (pathToCompositeSolidMap[pathId]) { | ||
| pathToCompositeSolidMap[pathId].push(id) | ||
| } else { | ||
| pathToCompositeSolidMap[pathId] = [id] | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jtran I wanted to ask you about this I'm building up a little map to go from a pathId to compositeSolids if they exist, because the composite solids in the artifactGraph done have edges going both ways, so there's no way to go from a path to its compositeSolid I did try adding the edge going the other way in https://github.com/KittyCAD/modeling-app/compare/kurt-composite-attempt?expand=1 But what do you think, solve this in rust, or just keep what I've got?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll wait for @jtran's input before merging
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When you say "it just didn't work", what symptoms are you actually seeing? The Mermaid chart generation wasn't updated, but when I did that, the charts show that the edges are bidirectional for the sim tests we have. But for some reason, edges in the graph are lost. Here's the diff of the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wait, I think I fixed it. The above diff made it obvious. I'll push to your branch.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pushed a few commits. This one is the fix: e23e6d2.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you! Looking now.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, what a silly mistake :/
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay, I've removed my little mapping, just waiting for CI to be green. |
||
|
|
||
| const result: string[] = [] | ||
| const stack: string[] = [artifact.id] | ||
|
|
||
|
|
@@ -211,6 +231,13 @@ export function findAllChildrenAndOrderByPlaceInCode( | |
| pushToSomething(currentId, current?.segIds) | ||
| } else if (current?.type === 'sweep') { | ||
| pushToSomething(currentId, current?.surfaceIds) | ||
| const path = artifactGraph.get(current.pathId) | ||
| if (path && path.type === 'path') { | ||
| const compositeSolidIds = pathToCompositeSolidMap[current.pathId] | ||
| if (compositeSolidIds) { | ||
| result.push(...compositeSolidIds) | ||
| } | ||
| } | ||
| } else if (current?.type === 'wall' || current?.type === 'cap') { | ||
| pushToSomething(currentId, current?.pathIds) | ||
| } else if (current?.type === 'segment') { | ||
|
|
||

There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has been a bad name for a while, thought I should fix it up while I was in here.