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: Improve Detection of "Drillable" Holes #9038

Closed
wants to merge 0 commits into from

Conversation

jimzim111
Copy link
Contributor

This PR includes two fixes to address valid/drillable holes not being included: an off-by-one list index when identifying potentially drillable cylinders, and incorrectly treating some through-holes as blind holes.

Detailed explanation and example screenshots are at https://forum.freecad.org/viewtopic.php?style=5&p=670298#p670298.

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 Mar 25, 2023
@freecadci
Copy link

pipeline status for feature branch PR_9038. Pipeline 817485875 was triggered at 61759ee. All CI branches and pipelines.

@luzpaz luzpaz requested a review from sliptonic March 25, 2023 18:44
@sliptonic
Copy link
Member

This seems to fail one of the unit tests.

======================================================================
FAIL: test20 (PathTests.TestPathDrillable.TestPathDrillable)
Test getDrillableTargets

Traceback (most recent call last):
File "/home/brad/FCD/FC/Mod/Path/PathTests/TestPathDrillable.py", line 269, in test20
self.assertEqual(len(results), 15)
AssertionError: 17 != 15

Please consider adding additional unit tests for the corner cases you're correcting. Thank you!

@luzpaz
Copy link
Contributor

luzpaz commented Apr 1, 2023

why was this PR closed ?

@jimzim111
Copy link
Contributor Author

I mixed a couple of changes in this PR by accident, including one that's incomplete (and causes the test failure). So to avoid it becoming cluttered I closed this PR and opened #9146 with the one fix that's complete/ready, while I continue troubleshooting the other.

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

4 participants