Rotated holes result in different sized Helix tool paths.

[IMback!]

I found something more severe, where FreeCAD crashes. If I use my model from above ...

I can confirm the issue

... and turn the clone Job->Model->Extrude by 90° around Z axis, then FreeCAD crashes immediately when leaving the field which induces a recompute. If I remove the Helix operation before turning there is no crash.

Can anyone confirm?

OS: macOS High Sierra (10.13)
Word size of OS: 64-bit
Word size of FreeCAD: 64-bit
Version: 0.19.17512 (Git)
Build type: Release
Branch: master
Hash: 7b315d3a15f6c0a3e796b2bf000cb791092de079
Python version: 3.7.3
Qt version: 5.9.7
Coin version: 4.0.0a
OCC version: 7.3.0
Locale: English/Germany (en_DE)

I cant reproduce the crash exactly like this, but i had a bunch of crashes rotating things while testing things for this thread, so something is up.

[chrisb]

Please test.

I can test as soon as it is available in the MacOS DMG.

Looking at the code it seems reasonable. I have created a semi-pathological example where an inner edge is spherical as well and it may be falsely detected by the algorithm. I did, however, not succeed in creating such a 3D dent where the inner edge has a smaller edge number than the top.

As this is a 3D pocket it is currently not handled correctly anyway and thus it does not have to be handled by the new version. The other pocket is currently also not handled correctly, and it is to be discussed if it has to be helixable at all.

Bildschirmfoto 2019-08-11 um 18.37.00.png (12.27 KiB) Viewed 1122 times

[rainharvester]

Just an FYI,
With My file:
I tried "mark to recompute" and then F5. It didn't fix the bug; holes are still too small.
Also, as noted before, closing / reloading the file didn't fix the bug.
I'm running V.18 (no other fixes, just the download that is available to everyone).

With the "test_holes00_cb.fcstd", "mark to recompute" then F5 makes the problem go away.

Attached is my file (those holes should be 4mm):

[IMback!]

Please test.

I can test as soon as it is available in the MacOS DMG.

Looking at the code it seems reasonable. I have created a semi-pathological example where an inner edge is spherical as well and it may be falsely detected by the algorithm. I did, however, not succeed in creating such a 3D dent where the inner edge has a smaller edge number than the top.

As this is a 3D pocket it is currently not handled correctly anyway and thus it does not have to be handled by the new version. The other pocket is currently also not handled correctly, and it is to be discussed if it has to be helixable at all.
Bildschirmfoto 2019-08-11 um 18.37.00.png

Ideally both of those cases should result in a warning to the user. But anyways - no reason to not make the algorithm more robust. I updated the pr should catch almost all of those cases now.

[chrisb]

I updated the pr should catch almost all of those cases now.

and thanks.

[mlampert]

I'm not able to reproduce the issue other than verify that your file @chrisb does have that issue.

Attached is the file I used to create some unit tests. They rotate the body in 5 degree increments and the resulting helices are all correct. They also rotate the Job's model clone in 5 degree increments and the results are also correct. Finally, the create a clone of the Body, rotate the clone in 5 degree increments and generate helixes based off that - also all of them correct.
I will not push the tests as they are since they add ~30 seconds of unit test runtime to our build, so for the final PR I'll reduce the test samples. But if anybody wants to have a look, and play with it the tests are in github

Even tough this works most of the time ATM, relying on the tessellated surface having a polygonal maximum in the x direction as the current code dose is fragile and may break at any time if the tesselation algorithm is changed, or the assumption is wrong for other reasons, like rotation after tesselation. Therefore i have the following PR: 2416 this fixes all test cases in this thread for me. Please test.

So instead of testing the fix that already exists you write another one instead? No unit tests? And what you "fixed" wasn't broken in the first place.

[IMback!]

So instead of testing the fix that already exists you write another one instead?

What fix? Im sorry, im currently unable to find it.

No unit tests?

well you already wrote a unit test, do you believe we need to test more cases?

And what you "fixed" wasn't broken in the first place.

well rainharvester's issue is real and reproducible for me, even after a recompute. Even if it was not and the current code worked perfectly, looking at the code for the boundBox in occ its pretty clear that relying on the boundBox to always be accurate in the x dimension is very fragile, so its worth improving even in this case.

If you have a better way to solve this please by all means..

[mlampert]

AFAICT the issue is not with Path, it seems to be with the model. If it is with Path, write a unit test that shows the problem and proves the fix.

[IMback!]

AFAICT the issue is not with Path, it seems to be with the model. If it is with Path, write a unit test that shows the problem and proves the fix.

Why? This seams excessive. You are wrong, this is a problem in path. OpenCASCADE documentation makes is clear that the BoundBox is a rough approximation at best, note the difference between Add() and AddOptimal(). The use of the boundingbox with add() in path where an exact value is needed is wrong and a bug. As AddOptimal() functionality is AFAIK not exposed in freecad's python bindings at the moment and also because looking at the occ code using it would be really slow when machining many holes, i did the best thing available to my knowledge.

"it seems to be with the model" Is also ridiculous on a conceptional level as such a simple model cant somehow be "wrong" without it being a severe bug in freecad/occ.

Its really getting on my nerves that there is so much resistance to change in this forum, there is always a lot of blow back even when fixing things that are clearly bugs.

[chrisb]

I think that using the the geometry of the model directly is better because it avoids the additional step of using the bound box.