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

[PATH] Update getDrillableTargets() to evaluate all faces 1..<n> #9146

Merged
merged 1 commit into from
Apr 4, 2023

Conversation

jimzim111
Copy link
Contributor

This fixes one cause of valid holes not getting included in Drill/Helix Ops due to a list iteraton off-by-one counter. Detailed explanation at https://forum.freecad.org/viewtopic.php?style=5&t=77055 (the first cause discussed on that post, disregard the 2nd as I'm still investigating/working on that one)

Thank you for creating a pull request to contribute to FreeCAD! Place an "X" in between the brackets below to "check off" to confirm that you have satisfied the requirement, or ask for help in the FreeCAD forum if there is something you don't understand.

  • Your Pull Request meets the requirements outlined in section 5 of CONTRIBUTING.md for a Valid PR

Please remember to update the Wiki with the features added or changed once this PR is merged.
Note: If you don't have wiki access, then please mention your contribution on the 1.0 Changelog Forum Thread.


@github-actions github-actions bot added the WB CAM Related to the CAM/Path Workbench label Apr 1, 2023
@luzpaz luzpaz requested a review from sliptonic April 1, 2023 18:39
@freecadci
Copy link

pipeline status for feature branch PR_9146. Pipeline 826746112 was triggered at 8b03616. All CI branches and pipelines.

@sliptonic
Copy link
Member

I'm merging this because it passes tests and appears to resolve a real problem. However, there is no github issue, only a forum topic.
What concerns me is that the existing unit tests didn't detect the original problem and we've added no new unit tests to the suite.
Why didn't the problem surface before? Was it that the current unit tests only have a single base object in the job? Was it that the Face0 or FaceN didn't have a hole?

@sliptonic sliptonic merged commit 54ff532 into FreeCAD:master Apr 4, 2023
6 checks passed
@jimzim111
Copy link
Contributor Author

jimzim111 commented Apr 4, 2023 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WB CAM Related to the CAM/Path Workbench
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants