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

Can't cut lathe surfaces on seams #732

Closed
phkahler opened this issue Oct 9, 2020 · 10 comments · Fixed by #733
Closed

Can't cut lathe surfaces on seams #732

phkahler opened this issue Oct 9, 2020 · 10 comments · Fixed by #733

Comments

@phkahler
Copy link
Member

phkahler commented Oct 9, 2020

System information

SolveSpace version: All

Operating system: All

Expected behavior

We should be able to cut a revolved or lathed surface on the seams without failure.

Actual behavior

The surfaces in the cutting plane are missing.

Additional information

I believe this is the same root problem as #540 and #652 and the solution may impact issues #680 and #683. I've created this with a simplified test case here:
coincident4.zip
Picture:
coincident_issue

@phkahler
Copy link
Member Author

phkahler commented Oct 9, 2020

Hoping for help on this from @jwesthues and @ruevs or anyone else who can help.

I've made an attempt to fix this with the following: phkahler@6f056eb

That patch explicitly checks for curves that lie in-plane on one shell (but are not part of the surface), and are exact trims for a surface in the other shell, and adds them as intersections curves. Why this apparently doesn't happen naturally from the last phase of surfinter I'm not sure. But the result was surprising to me:
coincident2
All of the edges are added but one? And some extra horizontal ones come seemingly from nowhere. Which one is missing depends on where you drag the upper-right vertex of the triangle, which also affects the horizontal false-edges.

Notably in issue #540 the incomplete polygon has both horizontal and vertical edges, but none of the other ones. This patch adds all of the missing edges but one, and also adds extra horizontal garbage like here.

I have not verified but I suspect real horizontal edges are found via plane-plane intersection since flat lathe surfaces are converted to 1st order. Also wonder if vertical edges are found because those surfaces are recognized as cylindrical and special cased as well.

@phkahler
Copy link
Member Author

phkahler commented Oct 9, 2020

OK. So this line in my patch:

        if(fabs(n.Dot(sc->exact.ctrl[i]) - d) > LENGTH_EPS) return false;

was missing the fabs. Putting that in gets rid of all the false horizontal edges. Still not sure where the missing edge is going.

@phkahler
Copy link
Member Author

phkahler commented Oct 9, 2020

A new clue. If we constrain the left side of the rectangle in g004 to contain the origin, we get a 90 degree section of the lathe object. the new surface often works thanks to the patch, but sometimes it too is missing an edge depending where that upper right point in the base sketch is moved:
coincident3

@ruevs
Copy link
Member

ruevs commented Oct 9, 2020

As you've probably noticed I'm mostly lurking lately. It's unlikely I'll have time to debug this.
But there is one thing that remained hanging from when I worked on intersection that may be relevant.

Sometimes edges are not classified correctly.

This is the (crazy) debug code I was using at the time:
ruevs@11e2db1
Plus the edge classifications dumped on stderr from here:
ruevs@11e2db1#diff-4a9b152f2f573ec230a16aabb099fa9aR349

The problem is that sometimes instead of a expected classification (e.g. 2212) one would get crazy values (essentially garbage from memory), which means that in some cases this function:

https://github.com/ruevs/solvespace/blob/994a79d3ee9acc37161a6e8bc0466618625d03eb/src/srf/boolean.cpp#L716
fails.

The function can obviously leave 'indir' and 'outdir' uninitialized by exiting through here https://github.com/ruevs/solvespace/blob/994a79d3ee9acc37161a6e8bc0466618625d03eb/src/srf/raycast.cpp#L510 but I never understood it enough to figure out in what circumstances.

To cause this misclassification I had to move two intersecting objects around until I hit the "correct" spot - just as you are doing here. This is why I think it may be relevant.

Sorry that I can't help more lately...

@ruevs
Copy link
Member

ruevs commented Oct 9, 2020

It seems to me that by returning "false" from ClassifyEdge @jwesthues meant "tangent" but the return value is never checked... so either this case slipped through the cracks or I misunderstand it (which in not unlikely).

@phkahler
Copy link
Member Author

phkahler commented Oct 9, 2020

@ruevs no need for apologies. We're all just doing what we can as time and interest come and go. I just like to ping people who have looked at this before. These bugs are like a puzzle. The only reason I got this far with NURBS issues was by tagging them and looking for similarities between the different issues, and having a decent idea what all the steps in the boolean process are. Knowing that the edge classifier sometimes fails is IMHO good information weather it turns out to be part of this problem or not. I tend to work better if I feel like someone is out there interested and listenting, even if they don't dig in, so thank you!

@ruevs
Copy link
Member

ruevs commented Oct 9, 2020

I'm very interested (and always glad to see you getting deeper into the math and logic of NURBS booleans)! But busy enough to just lurk, since actually making any progress always takes me at least a few hours :-)

Keep pinging me :-)

@jwesthues
Copy link
Member

At least as well as I remember my intent from ten years ago, I believe your understanding of ClassifyEdge() is correct. It's a bug that I didn't check the return value; that tangent case shouldn't happen, but we shouldn't just silently take garbage when it apparently does. That would probably be a good starting point for debugging.

I'm also quite pleased to see someone else looking at this. There's a much larger set of people interested to consume these NURBS operations as you improve them, and quite few willing/able to do the math to build them.

@phkahler
Copy link
Member Author

phkahler commented Oct 9, 2020

Updated patch here: phkahler@5814ed9

It prints some debug into. For the above sketch, it seems surface 0 and 3 contribute no curves with the new checks. That's consistent with the first trim curve of the lathe being missed for some reason as it joins those two surfaces. There are 12 surfaces in shell A and 6 in shell B. By moving the print for "edge check" up one line we then start getting edge checks for surface 0. This is very strange, All 12 surfaces have one edge coincident with the cutting plane but only 10 of them find it. BTW don't do an OMP build for this or the output will come in random order.

@phkahler
Copy link
Member Author

I realized that surface handles are only unique within their own shell. So my last check on edges making sure it didn't already join the plane and curved surfaces was wrong. First of all it's not possible since those surfaces are in different shells. Second, it was causing a few curves to be arbitrarily ignored and not copied as new intersections.

The fix for this is going to close issue 540. It almost fixes 652 but it looks like one PWL segment is missing from that one - not one curve, one segment. ugh...

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

Successfully merging a pull request may close this issue.

3 participants