Feedback on a prototype for rest machining

Mods CAM/Path
1

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

2

Hi and welcome to the forum!

Nice to see you starting right away with a contribution.

3

Very useful! Clear out corners maybe?

4

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/master/src/Mod/Path/PathTests)

5

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!!

6

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.

7

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

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.

Check out my function getRestAreas [2] and its invocation in Area.py [3]

[2] https://github.com/FreeCAD/FreeCAD/pull/9782/files#diff-0f2835cc48a7f4d50e288faeb16aa1c23354e965404671387cf7a5d29bb55417R499
[3] https://github.com/FreeCAD/FreeCAD/pull/9782/files#diff-b2a3bcb36474c6df487412c9fdbcadb48a8dafcc12b784f51ca274ec57132bcbR241

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 :slight_smile:

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.

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

8

I’ve linked 9756 to PR 9782

9

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/9782/files#diff-b2a3bcb36474c6df487412c9fdbcadb48a8dafcc12b784f51ca274ec57132bcbR241

[2] https://github.com/FreeCAD/FreeCAD/pull/9782/files#diff-f894104bd9f7df139552181d3860990f9205ef4fc35b1e8fa94a0a5631458976R202

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.

10

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?

11

I can do that if it’s your preferred solution, but I’m reluctant for a couple reasons:

  • Path computation can be expensive, and this could make rest machining computations rather slow on freshly opened files

  • Unexpected/non-requested path regeneration could result in different/worse paths. If I jump back and forth between freecad versions, paths can generate differently (simple example: if I jump between master and the rest machining branch I get very different tool paths). I don’t know how often users would find themselves in a situation like this (does it require mismatched/changing freecad versions? Perhaps it also happens when sharing a file between machines with opencamlib features enabled and machines without it installed?) I wouldn’t want creating a new pocket somewhere pristine on a part to destroy unrelated tool paths that the user didn’t intend to touch.

These aren’t the worst of sins; I could live with them. But if there’s a good way to save the results of makeSections() when tool paths are originally generated, I’d rather do that.

12

Why not just check if the operations need recomputing and force a recompute on them if they do?

A word of warning on doing that: there is a lot of unnecessary recomputing of the 2.5D paths in FreeCAD.

It seems that the fact that most of these path are pretty fast has meant this never got attention.

13

I agree It isn’t ideal. Nor is storing additional data in the document just to avoid a recompute. That leads to bloated files and slower load times.

I’m not suggesting we should be frivolous in this area. I’m saying that path calculation is an area where the user will tolerate a few seconds waiting on the machine. And this computation will get faster in the future with faster hardware, better libraries, and maybe someday mojo.

I’m also not suggesting that we should automatically recompute every path on startup. I’m saying when the rest operation processes, it should check any operations that it depends on and recompute those if they haven’t been computed already.

If the recompute happens every time the user changes the viewport or types a key, that’s different.

14

I was not arguing either way . I agree that stuffing this into the document structure does not seem the right way forward. I was just warning about the constant recomputation, since David may not be aware of this.

IMO this may be a good time to tidy up this loose end. I don’t think it would be that hard. IIRC the mechanism is already there, it just seems that paths get marked as changed when often there is zero change. For example it seems that as soon at the path edit dlg is opened the path gets marked as dirty and even cancelling out without the slightest edit triggers a recomp.

Cancelling out , without “Apply” having been used should not trigger a recomp. even if some fields have been changed in the dlg.

That would probably be less work than trying to add all the remainder info to the document structure which , as you say, is not really appropriate anyway.

15

It might be helpful to look at the removalshape. The property is defined in ObjectOp (Base class shared by all operations that use Area. It stores a shape in the document. This isn’t the sections but you could either recompute the sections or look at how ObjectOp stores the shape.

https://github.com/FreeCAD/FreeCAD/blob/master/src/Mod/Path/Path/Op/Area.py#L90

16

Thank you! That was exactly the example I needed! My PR is now ready for review: https://github.com/FreeCAD/FreeCAD/pull/9782

17

Not to be pushy or anything, but looking at github the last items appeared to be “passed” status.

So when might it appear in a weekly build?

18

I believe there is a feature freeze until v0.21 is released..

19

I am now running version 0.22.0 dev, should this start showing up now?

I went to “Pocket Shape” and looked for an option under “Opertion” and did not see it.

Where might I find documentation??

20

It hasn’t merged yet; keep an eye on https://github.com/FreeCAD/FreeCAD/pull/9782 for progress updates. If you’d like to test it before it merges, check out that branch and build it locally (or perhaps there’s a way to access github’s builds? I haven’t found a way, but perhaps it’s possible)