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
NURBS boolean improvement. #673
Conversation
I agree that your change to use the exact curve where available should be a strict improvement. For debugging, you could try printing/plotting the results of both your new curve-surface intersection and the old surface-surface-surface intersection, to see where they differ. |
After some more tweaks and using the exact version over in surfinter.cpp this fixes the above test model. It also fixes another one I have and a simplified version of issue #315 My head needs a rest now. I'll clean this up a bit tomorrow or this weekend and see if it fixes any other github issues. |
BTW, not sure if it's measureable but this will result in a reduction in runtime vs the more general function. Performance and correctness! |
Indeed. I suspect that commercial kernels get a lot of their robustness from special cases, a lot of them yet specialer than your change (specific code for circle-cylinder intersection, circle-cone, cylinder-cylinder, etc.). I see no downside to such special cases beyond the considerable work to develop and test them. |
@whitequark There are a couple incidental changes in this too. I can split them out if you want, otherwise I it should be good to go. |
I'd prefer if you split them, if this change causes any issues, this will make bisecting work better. |
The 3-plane intersection code fails to converge when a curve joins two tangent NURBS patches. This adds a new function for intersecting exact curves with a surface to avoid those failures. Fixes simplified test model for issue solvespace#315.
…try harder to converge.
Done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I'd like to get input on this from @jwesthues @ruevs and @Evil-Spirit
The code in Boolean tries to intersect a curve with a surface. It uses the PWL representation of the curve (the more general case) and the 2 surfaces it trims. To do the intersection it calls SSurface::PointOnSurfaces() which finds the U,V intersection of the 3 surfaces. I noticed that function will not converge (it gives up) if 2 of the surfaces are tangent along the curve, which is the case for Lathe and Helical surfaces joined every 90 or less degrees. In that case we always have an exact representation of the curve, so I created a new function to intersect an exact curve with a surface which can be used in the (common) special case that we have and exact curve. I thought this may result in better accuracy finding the intersection points and might fix some of the boolean issues.
Unfortunately I have not seen any broken boolean start working, but I do see this broken one being more consistent:
The first picture is before this change. The red lines tend to flicker between the upper missing 90 degree surface patch and the lower one, but very rarely will all the curves show in red. It flickers back and forth as you drag the cylinder. With the change it very consistently shows all the curves. In neither case does it ever show the surfaces. The thing is I'm not sure this is consistently better than before or consistently worse. From a theoretical point of view this change should be better.
What do you think?
BTW I have not seen any regressions with 10-20 sketches.
BTW2 the above sketch is a lathed object (spool) with a circle/cylinder cut out to demo boolean failure.