Skip to content
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

[Problem] CAM: Incorrect paths generated by adaptive clearing #6217

Open
FreeCAD-Bug-Importer opened this issue Feb 7, 2022 · 17 comments
Open
Labels
Bug This issue or PR is related to a bug WB CAM Related to the CAM/Path Workbench

Comments

@FreeCAD-Bug-Importer
Copy link
Collaborator

FreeCAD-Bug-Importer commented Feb 7, 2022

Issue imported from https://tracker.freecad.org/view.php?id=4658

  • Reporter: dmitry_sushkov
  • Date submitted: 5/12/2021
  • FreeCAD version: 0.19
  • Category: Bug
  • Status: confirmed
  • Tags:

Original report text

Adaptive clearing without "use outline" generating incomplete path. With "use outline" it goes out of boundary in one place.

here is the file:Isses-6217.zip
originally shared via https://drive.google.com/file/d/1kwrOw5bVSPOU0QoOWJmXbtGHRwC3RMcf/view?usp=sharing

Here is the forum discussion:
https://forum.freecadweb.org/viewtopic.php?f=15&t=58547&p=503248#p503248

Steps to reproduce

Use attached file and create adaptive clearing operation with settings from screenshots.
Both paths with "use outline" and without are incorrect.

Other bug information

  • Priority: normal
  • Severity: major
  • Category: Bug
  • Updated: 9/3/2021

Discussion from Mantis ticket

Comment by chrisb 2021-05-13 10:01

I have simplified the file so that it still shows the error. The file couldn't be added to the tracker, I add it to the forum.


Comment by Kunda1 2021-08-29 13:46

Added chrisb's simplified example


Comment by russ4262 2021-09-03 22:27

Confirmed. The Adaptive operation has some internal bugs related to path generation, mainly of two types.
The first type is an odd lateral path from the correct clearing area outward to the perimeter of the target area. The second type is a larger path generation bug resulting in a partial path set for a given target area, as though part of the target area is ignored or truncated. This second type seems to have some correlation to complex shapes and curves.

@FreeCAD-Bug-Importer FreeCAD-Bug-Importer added Bug This issue or PR is related to a bug WB CAM Related to the CAM/Path Workbench labels Feb 7, 2022
@luzpaz luzpaz changed the title Incorrect paths generated by adaptive clearing [PATH] Incorrect paths generated by adaptive clearing Feb 9, 2022
@Russ4262
Copy link
Contributor

Russ4262 commented Apr 7, 2022

0.20 pre-release analysis with Revision 28638:

This screen shot is from the above revision.
Snip macro screenshot-70ab79
The Adaptive algorithm has problems. That said, even in its current state, Adaptive is very useful.

The key issue that I see reported is the erroneous lateral paths that are produced, as seen in the screenshots in the forum post. However, there is a work around by adding a small Stock to leave value, and then adding a finishing Profile pass to clean up. There was a PR in play, #5276, to address the lateral bug. I do not know why it was closed. Anyhow, because of the existing work around, the priority is low in my opinion.

@sundtek
Copy link
Contributor

sundtek commented May 2, 2022

#5276 is still valid and fixes the issue, I'm using that modification since December last year.
I just don't want to spend more time on it, so someone else might pick it up and rewrite it.

@luzpaz
Copy link
Contributor

luzpaz commented May 8, 2022

#5276 wasn't merged per #6217 (comment)
This issue is still open.

@sliptonic
Copy link
Member

Tested with 0.21 and the path looks correct. Can we close?
image

@sundtek
Copy link
Contributor

sundtek commented Aug 13, 2023

the issue is still there, use an 8mm end-mill.

On the other side the issue including fix are well documented by now I'm still using the fixed code on a regular basis :-)

@Russ4262
Copy link
Contributor

Russ4262 commented Aug 13, 2023 via email

@Russ4262
Copy link
Contributor

Russ4262 commented Aug 13, 2023 via email

@sundtek
Copy link
Contributor

sundtek commented Aug 14, 2023

https://github.com/FreeCAD/FreeCAD/pull/5276/files

just to recall doFilter will fix it. I didn't want to spend more time on it because I'm quite overloaded with other things on my side; So I quickly erased this topic from my head once it was done. I use this modification since I fixed it.
So asking me about details of the patch is worthless, it just filters nearby values from a list of numbers and performs well doing so.

Back then freecad needed a substantial amount of modifications to make it useful for my project. Multiple times I've reached the situation where I almost had to give up my project with freecad after months designing hundreds of parts with it.
I designed a pick and place machine back then with 8mm feeders which I have machined from Aluminum, I also did all the firmware and software for it.
Path needs quite a few modifications to perform in a useful way for such a project, first of all the path extensions have to be rewritten and all the auto detection which is a bottleneck has to be removed - handing over more options to the user.
Also I had to cope with various other FreeCAD related issues.

@sliptonic
Copy link
Member

sliptonic commented Aug 14, 2023 via email

@berniev
Copy link
Contributor

berniev commented Aug 14, 2023

the concern was about code style

I don't think so. IMO More like an overzealous maintainer completely missing the forest for the trees. This used to happen a lot. Thankfully not so much nowadays.

Result: Issue STILL not fixed, OP cheesed off, and discussion consuming everyone's time 18 months later.

All that for 34 lines of code? Really?

@sliptonic
Copy link
Member

All that for 34 lines of code? Really?

The single biggest point of contention when we wrote the CONTRIBUTING policy was about code quality. People wanted the maintainers to be activist about ensuring that contributions are well written. We tried to strike a balance but it's a judgment call. You yourself have been a loud voice for code modernization.

In this case, the old C-style was flagged as problematic. The contributor was even given sample code and declined to continue working on it. That puts the work on someone else. You're right it's a small fix. Can you tell me how big a contribution should be before we start caring about code quality?

@sundtek
Copy link
Contributor

sundtek commented Aug 14, 2023

I would change the rules accordingly:

№1 a bug should be fixed
№2 if someone wants to have something look better he should step up and contribute, that was always my opinion (for over 20 years) and will always be my opinion.

If someone comes across and says but it should look like that, I just say good go for it I have no objection but I won't be the one spending more time on it.

I did my part by finding, understanding, resolving and heavily testing this issue.

@berniev
Copy link
Contributor

berniev commented Aug 14, 2023

You yourself have been a loud voice for code modernization

Absolutely! But that's a different topic, and certainly not an excuse to block a PR.

old C-style was flagged as problematic

Freecad is seriously riddled with old style stuff so that does not make a PR "bad quality"? Just fitting in!

Anyone can nitpick a PR to death, but what about the bigger picture?

@sliptonic
Copy link
Member

I would change the rules accordingly:

You can do that. Submit an issue describing the problem with the policy. Then submit a PR to change the contributing.md policy and solve the problem.

Anyone can nitpick a PR to death, but what about the bigger picture?

How big should a contribution be before code quality becomes a consideration?

@berniev
Copy link
Contributor

berniev commented Aug 14, 2023

What is "code quality"?

Perhaps:

  1. Solves a problem
  2. Does what it says
  3. Compiles clean
  4. Has tests
  5. Passes tests
  6. Advances the overall project
  7. Is reasonably structured
  8. Does not massively violate established programming principles
    ...
    ...
    ...
    ... and way down the list:
    Is to the latest code style specs
    Uses only the latest language features
    Uses only variable names approved by maintainer
    Demonstrates a willingness to bend to the whims (one-upmanship) of a maintainer

@djmdjm
Copy link
Contributor

djmdjm commented Feb 29, 2024

Please take a look at PR #12661 AFAIK it addresses all the feedback on @sundtek's original PR and implements a more conservative approach (only removing adjacent points that are close) that seems to work just as well

@maxwxyz
Copy link
Collaborator

maxwxyz commented Mar 2, 2024

will PR #12661 fix the entire issue?

@luzpaz luzpaz changed the title [PATH] Incorrect paths generated by adaptive clearing [Problem] CAM: Incorrect paths generated by adaptive clearing Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug This issue or PR is related to a bug WB CAM Related to the CAM/Path Workbench
Projects
Status: Backlog
Development

No branches or pull requests

8 participants