Feedback on a prototype for rest machining

Here's the place for discussion related to CAM/CNC and the development of the Path module.
Forum rules
Be nice to others! Respect the FreeCAD code of conduct!
davidgilkaufman
Posts: 21
Joined: Fri Jun 02, 2023 4:46 pm
Contact:

Feedback on a prototype for rest machining

Post by davidgilkaufman »

Hi everyone! I'm new to the community, and I'm excited to make my first contribution!

I prototyped a minimum-viable-product feature for rest machining in FreeCAD. The code is currently rough and messy, but it seems to work as I intended, so I'm posting now to ask for input on what the final feature should look like.

I've attached a screenshot showing how the prototype feature is used and what it does. My example job consists of two operations: a normal pocket operation with a 6mm tool, and a second pocket operation run on the same area with a 1.5mm tool with my feature enabled. I created a new property "PrevDiameter" (default 0mm = feature disabled). When this property is set, it is assumed the pocket region has already been machined with a cylindrical tool of the specified diameter, and restricts the area to be machined based on that information. These operations produce two tool paths, a large rectangular path in the center (6mm tool) and four little triangle-ish regions in the corners (1.5mm tool).

So far I've only tested with the Pocket operation, and on this one model. I have not tried Adaptive or 3D Pocket yet, but I think this feature should apply there too. I will look into this before making a PR.

I hope to make a follow-up feature that automatically infers PrevDiameter from previous operations and handles non-cylindrical previous-operation tools, but imo this is not part of the MVP feature. I intend for both the feature I've prototyped and the follow-up I've planned to be exposed to the user: a manual mode and an automatic mode for specifying the previously cleared area. This follow-up will not be a part of my initial PR, but I'm still interested feedback on it and want to provide context for my ideas.


# Limitations:

* This feature reduces the area to be machined, but otherwise does not take advantage of knowing that some of the region has already been cleared. Most notably, the tool enters each corner region from the top, not from the side. In an Adaptive operation, it will helix down inside the tiny region of each corner.

* Related to the above, this feature still machines slightly more area than strictly necessary. There is no reason to trace the part of the outline of the reduced region that is on the interior of the full region, but it does so anyway. This limitation and the one above it both result from me reducing the area that is fed into the path-generation code, but not otherwise modifying or otherwise informing the path generation algorithm.


# Specific feedback requests:

* Naming/presentation/usability: I don't think a property called PrevDiameter hidden away in the property list is a good way to advertise this feature to users. I'd appreciate ideas on how this feature should be invoked and what it should be called. Perhaps it belongs in the pocket GUI somewhere?

* Testing: what are the norms for testing features like this? Is there a standard set of models? Should I mark this feature as experimental for now (and if so how)?

* Follow-up feature: any comments on my plans for follow-up work?

* Coding help: App/Area.cpp uses a lot of macros like PARAM_ENUM_CONVERT and PARAM_FIELDS to interact with properties. I get the sense that my code should be using them too when I access user parameters and configurations for clipper operations, but I don't understand what I should do with them and have instead been using parameters directly (i.e. myParams.PrevDiameter, myParams.ToolRadius) and invoking clipper offset and difference operations with default parameters. Is there any documentation on these macros?

Thanks in advance for your comments and help!

David
Attachments
restDemo.png
restDemo.png (74.47 KiB) Viewed 11102 times
chrisb
Veteran
Posts: 53785
Joined: Tue Mar 17, 2015 9:14 am

Re: Feedback on a prototype for rest machining

Post by chrisb »

Hi and welcome to the forum!

Nice to see you starting right away with a contribution.
A Sketcher Lecture with in-depth information is available in English, auf Deutsch, en français, en español.
Dimitrios2
Posts: 104
Joined: Tue Feb 21, 2023 9:30 pm

Re: Feedback on a prototype for rest machining

Post by Dimitrios2 »

Very useful! Clear out corners maybe?
User avatar
Kunda1
Veteran
Posts: 13434
Joined: Thu Jan 05, 2017 9:03 pm

Re: Feedback on a prototype for rest machining

Post by Kunda1 »

davidgilkaufman wrote: Fri Jun 02, 2023 5:51 pm * Testing: what are the norms for testing features like this? Is there a standard set of models? Should I mark this feature as experimental for now (and if so how)?
Question to devs: Is there documentation for Path wb on how to write tests? (beside looking at other Path tests in the source: https://github.com/FreeCAD/FreeCAD/tree ... /PathTests)
Alone you go faster. Together we go farther
Please mark thread [Solved]
Want to contribute back to FC? Checkout:
'good first issues' | Open TODOs and FIXMEs | How to Help FreeCAD | How to report Bugs
User avatar
sliptonic
Veteran
Posts: 3453
Joined: Tue Oct 25, 2011 10:46 pm
Location: Columbia, Missouri
Contact:

Re: Feedback on a prototype for rest machining

Post by sliptonic »

davidgilkaufman wrote: Fri Jun 02, 2023 5:51 pm I prototyped a minimum-viable-product feature for rest machining in FreeCAD. The code is currently rough and messy, but it seems to work as I intended, so I'm posting now to ask for input on what the final feature should look like.
Welcome! This is really great. I've wanted to see a rest capability for a very long time.
I'm interested to know more about how you're detecting the areas that need refinement. What's your algorithm?
So far I've only tested with the Pocket operation, and on this one model. I have not tried Adaptive or 3D Pocket yet, but I think this feature should apply there too. I will look into this before making a PR.
Right. The challenge is detecting the areas to work on. Once that is done and reliable, the same solution should apply to any operations that need it.
I hope to make a follow-up feature that automatically infers PrevDiameter from previous operations and handles non-cylindrical previous-operation tools, but imo this is not part of the MVP feature. I intend for both the feature I've prototyped and the follow-up I've planned to be exposed to the user: a manual mode and an automatic mode for specifying the previously cleared area. This follow-up will not be a part of my initial PR, but I'm still interested feedback on it and want to provide context for my ideas.
Basically, all the operations work in a similar pattern. They take the base geometry, interpret it to compute the target area to work on, and then generate an appropriate toolpath for the targeted area.
I always envisioned a rest capability would work similarly. You would create the first operation with the big tool. Then create another operation and mark it as a refinement by identifying the predecessor. The refinement op wouldn't have base geometry. Instead, it would look at the result of the predecessor and compute the target from what wasn't touched. From there on, it would do exactly the same thing as the original operation. In this way, you could build a chain of refinements as long as needed. Also, you could do a traditional pocket and then an adaptive refinement, for example.
And by linking this way, there's no need for a PrevDiameter. It can get the previous diameter from the predecessor ops toolcontroller.tool.
* This feature reduces the area to be machined, but otherwise does not take advantage of knowing that some of the region has already been cleared. Most notably, the tool enters each corner region from the top, not from the side. In an Adaptive operation, it will helix down inside the tiny region of each corner.
Acceptable.
* Related to the above, this feature still machines slightly more area than strictly necessary. There is no reason to trace the part of the outline of the reduced region that is on the interior of the full region, but it does so anyway. This limitation and the one above it both result from me reducing the area that is fed into the path-generation code, but not otherwise modifying or otherwise informing the path-generation algorithm.
Ok.
# Specific feedback requests:

* Naming/presentation/usability: I don't think a property called PrevDiameter hidden away in the property list is a good way to advertise this feature to users. I'd appreciate ideas on how this feature should be invoked and what it should be called. Perhaps it belongs in the pocket GUI somewhere?
Agreed. As noted above, it should be a reference to the predecessor operation. If the user changes the first ops toolcontroller.tool, (eg selecting a larger tool) then the change would propagate through to the refinement. Just storing a PrevDiameter value would result in a crash.
* Testing: what are the norms for testing features like this? Is there a standard set of models? Should I mark this feature as experimental for now (and if so how)?
At least in Python, We're working to improve code coverage and also to refactor code into more testable modules. There are unit tests in /Mod/Path/PathTests. Don't worry about testing the UI. It's more important to test the lower-level logic. If your algorithm for identifying target geometry can be abstracted to a very clean (ideally pure) function, then implement it as a separate module and write a unit test module for it.
* Coding help: App/Area.cpp uses a lot of macros like PARAM_ENUM_CONVERT and PARAM_FIELDS to interact with properties. I get the sense that my code should be using them too when I access user parameters and configurations for clipper operations, but I don't understand what I should do with them and have instead been using parameters directly (i.e. myParams.PrevDiameter, myParams.ToolRadius) and invoking clipper offset and difference operations with default parameters. Is there any documentation on these macros?
I don't know much about the C++ code but I know macros are discouraged.

This is really great and I'm thrilled to see it. If I can be of any help, let me know. I've created an issue in the tracker. We can continue to discuss the use-case and problem there. When you have a PR ready (even a draft PR) please mark it to fix this issue. Then we can talk about code-specific issues in the PR.
https://github.com/FreeCAD/FreeCAD/issues/9756

Thank you!!
User avatar
freman
Veteran
Posts: 2188
Joined: Tue Nov 27, 2018 10:30 pm

Re: Feedback on a prototype for rest machining

Post by freman »

Interesting. One small point, maybe use the term remainder or residual instead of rest. Rest could mean a support or a pause: a rest, to rest not the rest.
It would also make it lot easier for translators to work out which meaning to use.

Bear in mind that paths can be re-ordered via job edit dialogue, so what was "previous" may suddenly change.
davidgilkaufman
Posts: 21
Joined: Fri Jun 02, 2023 4:46 pm
Contact:

Re: Feedback on a prototype for rest machining

Post by davidgilkaufman »

Thanks everyone! I appreciate the enthusiasm for this feature.

I've created a draft PR with the code in its current state (still messy!). We can continue the discussion on that PR [1].

[1] https://github.com/FreeCAD/FreeCAD/pull/9782
freman wrote: Tue Jun 13, 2023 11:30 pm One small point, maybe use the term remainder or residual instead of rest. Rest could mean a support or a pause: a rest, to rest not the rest.
It would also make it lot easier for translators to work out which meaning to use.
As someone fairly new to the machining world, I agree with the sentiment here -- I find the word "rest" confusing. However, as best as I can tell this term is an accepted standard in machining, so I am using it here. I don't know how the term is translated, or what I should do to cue translators to pick the appropriate translation.
sliptonic wrote: Sun Jun 11, 2023 1:40 pm I'm interested to know more about how you're detecting the areas that need refinement. What's your algorithm?
Check out my function getRestAreas [2] and its invocation in Area.py [3]

[2] https://github.com/FreeCAD/FreeCAD/pull ... b55417R499
[3] https://github.com/FreeCAD/FreeCAD/pull ... 132bcbR241
sliptonic wrote: Sun Jun 11, 2023 1:40 pm And by linking this way, there's no need for a PrevDiameter. It can get the previous diameter from the predecessor ops toolcontroller.tool.
I like this idea -- linking the previous operation is much better than having the user enter a diameter. I'm not sure how to do that yet, though; the UI elements are all new to me :)

That being said, the PreviousDiameter feature I've written is a little different than "infer cleared area from this operation". In particular, the feature I've written assumes that on each layer the current operation mills that the area has been maximally cleared by a tool of the specified diameter. This might not be true, even the tool diameter and projected area match a previous operation, because the previous operation may not have done a pass at the same height. Passes at higher heights would not clear area, and passes at lower heights could clear a different (likely equal or larger) area depending on the tool diameter at the appropriate height. The next feature I write (for which I've made a UI entry in my PR, but haven't implemented yet) will handle all of these things correctly, but the feature I've written so far takes none of this into account and I want the difference to be clear to the user.

The easiest solution to this problem might be to hold off on merging until I write the more complete feature, and release only that mode for rest machining. My plan for that feature is:

- enumerate plausibly-relevant operations; for each:
- getSections(); for each:
- compute the delta height between the section height and the current cutting height
- if it's between 0 and the max cutting height, find the tool diameter at the appropriate height and invoke (a slightly modified version of) getRestAreas() with that diameter. Set the working area to the resulting region and continue enumerating

This seems pretty tractable and likely to work; my main delays will be my personal schedule and my lack of familiarity with the code base.
sliptonic wrote: Sun Jun 11, 2023 1:40 pm When you have a PR ready (even a draft PR) please mark it to fix this issue
I don't think I have sufficient permissions to do this -- there's nothing for me to click under "Development // Successfully merging this pull request may close these issues. // None yet". Can you link the issue for me?

Thanks!
David
User avatar
Kunda1
Veteran
Posts: 13434
Joined: Thu Jan 05, 2017 9:03 pm

Re: Feedback on a prototype for rest machining

Post by Kunda1 »

davidgilkaufman wrote: Thu Jun 15, 2023 8:08 pm I don't think I have sufficient permissions to do this -- there's nothing for me to click under "Development // Successfully merging this pull request may close these issues. // None yet". Can you link the issue for me?
I've linked issue #9756 to PR issue #9782
Alone you go faster. Together we go farther
Please mark thread [Solved]
Want to contribute back to FC? Checkout:
'good first issues' | Open TODOs and FIXMEs | How to Help FreeCAD | How to report Bugs
davidgilkaufman
Posts: 21
Joined: Fri Jun 02, 2023 4:46 pm
Contact:

Re: Feedback on a prototype for rest machining

Post by davidgilkaufman »

Thank you!

@sliptonic the PR is mostly ready now, and I would like your help with the last bugs. I'm happy to discuss/coordinate on the PR, but I'm posting here for now because it's not clear to me that you've seen the PR or get updates if I post there.

In the PR's current state, the rest machining feature works on Pocket and 3D Pocket operations (not Adaptive, because that uses a different workflow; perhaps we can revisit that in a future PR). Attached: screenshot of the feature + UI. First a normal pocket operation machines the left and right surfaces with a large tool, then a 3D Pocket operation with a small tool clears the full part but skips the regions that are already cleared. (A better tool path would clear much of the middle region with the large tool too, but I did it this way to test functionality)

I have one major bug left to fix before merging, which is that the feature currently only takes into account operations that have generated paths since the file was last opened (i.e. generate -> save -> close -> open doesn't count). When pocket operations execute I save metadata about the space they clear to a variable [1], but the contents of that variable don't get saved/restored with the rest of the document. Could you help me figure out how to do that? I played around with creating a property in the operation object [2], but I couldn't figure out what type everything needed to be to assign to it. Also it seemed like I might need to add my regions to the document tree somewhere? I don't particularly want to do that. Ideally these areas should be serialized next to the generated path itself; they are fundamentally paired with the generated path, and invalidated each time it is changed.

[1] https://github.com/FreeCAD/FreeCAD/pull ... 132bcbR241

[2] https://github.com/FreeCAD/FreeCAD/pull ... 458976R202

As a more minor bug, I'm not sure the UI I designed makes sense. I created a separate panel for configuring rest machining modes, but since I did that I deleted my tool-diameter mode and the only options now are "disabled" and "auto, from all prior operations". Perhaps this panel should become a single enable/disable check box in another panel? The only reason I can think to keep the separate panel around is if we want to add more options to rest machining, perhaps for specifying which of the prior operations to use in rest machining (as a time-saving mechanism that might (idk) be important for complicated jobs with many operations). Or maybe to leave space for more explanatory text/icons/etc than would be appropriate in another panel. Or to merge an otherwise ready feature and not get too lost in the weeds tweaking minor details. What do you think?

Other than that I have some bookkeeping-type cleanup to do, and it's ready for review and merging.
Attachments
restMachining.png
restMachining.png (50.64 KiB) Viewed 10361 times
User avatar
sliptonic
Veteran
Posts: 3453
Joined: Tue Oct 25, 2011 10:46 pm
Location: Columbia, Missouri
Contact:

Re: Feedback on a prototype for rest machining

Post by sliptonic »

davidgilkaufman wrote: Mon Jun 26, 2023 9:15 pm
@sliptonic the PR is mostly ready now, and I would like your help with the last bugs. I'm happy to discuss/coordinate on the PR, but I'm posting here for now because it's not clear to me that you've seen the PR or get updates if I post there.
Thanks. I have a saved URL to view PR's but I'm filtering PRs in draft status right now. No hurry on finishing this up. It won't get merged until after V0.21 is released and the feature freeze is lifted.

I've built your branch and everything seems to work as described. Very nice!

Regarding the UI, I agree. Having a whole panel for what is basically one checkbox control is overkill. I suggest elmininating it and just adding a checkbox to the operation tab. Suggested label: "Enable Rest Optimization"
I have one major bug left to fix before merging, which is that the feature currently only takes into account operations that have generated paths since the file was last opened (i.e. generate -> save -> close -> open doesn't count). When pocket operations execute I save metadata about the space they clear to a variable [1], but the contents of that variable don't get saved/restored with the rest of the document. Could you help me figure out how to do that? I played around with creating a property in the operation object [2], but I couldn't figure out what type everything needed to be to assign to it. Also it seemed like I might need to add my regions to the document tree somewhere? I don't particularly want to do that. Ideally these areas should be serialized next to the generated path itself; they are fundamentally paired with the generated path, and invalidated each time it is changed.
Why not just check if the operations need recomputing and force a recompute on them if they do?
Post Reply