Add edit flow for point-and-click Chamfer and Fillet#5946
Conversation
|
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 ↗︎
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5946 +/- ##
=======================================
Coverage 85.82% 85.82%
=======================================
Files 113 113
Lines 44484 44484
=======================================
Hits 38177 38177
Misses 6307 6307
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
pierremtb
changed the title
pierremtb
changed the title
pierremtb
changed the title
Irev-Dev
left a comment
Irev-Dev
left a comment
There was a problem hiding this comment.
I think it would be good to have these work for chamfers/fillets not in pipes.
I don't think it would be too much extra work?
| if (err(edgeArtifact)) { | ||
| return baseCommand | ||
| } |
There was a problem hiding this comment.
What happens here exactly when there's an error and it returns the baseCommand?
Does the edit still work?
There was a problem hiding this comment.
Good call, the command bar gets opened up but is missing all the prefilled data which is confusing. I believe @franknoirot intended for us to use
modeling-app/src/lib/operations.ts
Line 28 in 44a62ca
There was a problem hiding this comment.
Yeah you're right @pierremtb there are probably places in operations.ts that you took from that are also not using that pattern yet, apologies it was a pattern added while we've been building the system but we should report a failure reason everywhere instead of returning baseCommand silently.
| const { nodeToEdit, selection, radius } = input | ||
|
|
||
| // If this is an edit flow, first we're going to remove the old one | ||
| const pipeIndex = 5 | ||
| if ( | ||
| nodeToEdit && | ||
| nodeToEdit[pipeIndex][0] && | ||
| typeof nodeToEdit[pipeIndex][0] === 'number' | ||
| ) { | ||
| const r = locateExtrudeDeclarator(ast, nodeToEdit) | ||
| if (!err(r) && r.extrudeDeclarator.init.type === 'PipeExpression') { | ||
| r.extrudeDeclarator.init.body.splice(nodeToEdit[pipeIndex][0], 1) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Okay so I'm guessing that this only works for editing fillets/chamfers that are in a pipeExpression?
Is that easy to fix for ones that are not in a pipe?, because while our code-mods might put them in a pipe, it would be good if the edit still worked without the pipe because either the user or AI might not put it in a pipe.
With that said this magic 5 makes me nervous, I'm guessing 5 is how deep in the editNode the pipeExpression is on the most common cases. I know we're trying to get the basic cases working because editing stuff within function definitions or in if statements is whole can of worms we don't the band width for right now, but maybe using
const pipeIndex = nodeToEdit.findIndex(([_, type]) => type === 'PipeExpression') would make this a little more robust?
There was a problem hiding this comment.
Yes, only PipeExpression is supported as this is what the point-and-click addition adds. As discussed on Slack I'll try to see how much of a lift it would be to add support for standard calls that would have been ai or user typed. But for now I added a specific catch for it that will get toasted and prevent edition. That was related to your other comment about erroring too and I think now it's more in line with how @franknoirot had things set up. Some of the changes here with return { reason: ... will have to be done on other edit flows as well.
Good point, I didn't like it either which is why I made it explicit in its own variable as a starter haha. Changed it to a findIndex call, much better.
| const isPipeExpression = nodeToEdit.some( | ||
| ([_, type]) => type === 'PipeExpression' | ||
| ) | ||
| if (!isPipeExpression) { | ||
| return { | ||
| reason: 'Only chamfer in pipe expressions are supported for edits', | ||
| } | ||
| } |
There was a problem hiding this comment.
This is our new guard for now @Irev-Dev. I'm honestly thinking we should merge as is, and try to expand later. Got too many branches at once for these flows and I'm afraid I'll get lost haha.
4 participants
Fixes #5521 and #5950, using a common prepareToEdit function for both chamfer and fillet.
Cases:
Video of the new e2e test step for each at play:
Screen.Recording.2025-03-23.at.6.26.29.AM.mov