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

NURBS boolean improvement. #673

Merged
merged 2 commits into from Aug 13, 2020
Merged

NURBS boolean improvement. #673

merged 2 commits into from Aug 13, 2020

Conversation

phkahler
Copy link
Member

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:
Bug1_before
Bug1_after

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.

@jwesthues
Copy link
Member

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.

@phkahler
Copy link
Member Author

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.

@phkahler
Copy link
Member Author

BTW, not sure if it's measureable but this will result in a reduction in runtime vs the more general function. Performance and correctness!

@jwesthues
Copy link
Member

jwesthues commented Aug 13, 2020

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.

@phkahler phkahler changed the title Possible NURBS boolean improvement. The 3-plane intersection code fai… NURBS boolean improvement. Aug 13, 2020
@phkahler phkahler removed the request for review from Evil-Spirit August 13, 2020 17:12
@phkahler
Copy link
Member Author

@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.

@whitequark
Copy link
Contributor

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.
@phkahler
Copy link
Member Author

Done.

Copy link
Contributor

@whitequark whitequark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Naked edges in bezier lathe difference
4 participants