Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
max-mrgrsk
left a comment
max-mrgrsk
left a comment
There was a problem hiding this comment.
Wow, that was fast, thank you! Works locally like magic. I’ve left 2 comments just to know your opinion on these things, happy to approve otherwise.
| const extrudePathToNode = | ||
| lastChildVariable && !err(lastChildVariable) | ||
| ? lastChildVariable.pathToNode | ||
| : [] |
There was a problem hiding this comment.
can this fallback cause a silent fail?
There was a problem hiding this comment.
Maybe? I didn't touch that cause this is how it was :/
| const children = findAllChildrenAndOrderByPlaceInCode( | ||
| { type: 'sweep', ...extrusion }, | ||
| kclManager.artifactGraph | ||
| ) |
There was a problem hiding this comment.
do we need a little comment here to explain why do we need to compute children twice?
// necessary for faces with holes ( multiple children exist )
There was a problem hiding this comment.
What do you mean twice? We were doing that already? getLastVariable is taking as input the result from findAllChildrenAndOrderByPlaceInCode, as introduced by @Irev-Dev in https://github.com/KittyCAD/modeling-app/pull/5906/files#diff-0498f1baa97e5510a829a42a13959af07e67dc56384e46cd7c2a89b553516b08R165
There was a problem hiding this comment.
Yeah, I was thinking I wouldn’t guess why we’re using getLastVariable without the issue’s context. Not a blocker - just chatting.
pierremtb
deleted the
pierremtb/issue8851-Can-not-sketch-on-face-with-a-hole-in-it
branch
2 participants
Fixes #8851
Changes
While adding debugging statements and figuring out where the issue was, I noticed that this part of
selectionBodyFaceto recoverExtrudePlaneFacewasn't properly returning the last item, in our case here hole001.modeling-app/src/lib/selections.ts
Lines 1092 to 1102 in 3d82b5a
Other places in the codebase use
getLastVariableinstead of direct indexing after calls tofindAllChildrenAndOrderByPlaceInCode, so I did that and it seems to fix the issue for me!Demo
Screen.Recording.2025-11-20.at.10.06.41.AM.mov