-
Notifications
You must be signed in to change notification settings - Fork 101
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
Initial attempt at new 'Boolean' operations to keep a complete mesh. #578
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
src/manifold/src/boolean_result.cpp
Outdated
// Add, Subtract, Intersect, KeepP, KeepQ | ||
const int c1tab[] = { 1, 1, 0, 1, 0 }; | ||
const int c2tab[] = { 1, 0, 0, 0, 1 }; | ||
const int c3tab[] = { -1, -1, 1, 0, 0 }; |
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.
Let's move this convo to these threads. What do you mean by cA and cB? But generally I think you're thinking about this right.
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.
I was trying to use the notation from the dissertation, p. 68.
here cA==c1, cB==c2, cI==c3 (I think).
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.
Okay, thanks, I'm following now. I think you're close, but when splitting up these multipliers, I think X12 and X21 still need the same value, since they form the intersection. The difference is that instead of using that same multiplier for the winding number of X03/X30, that needs to be zero, such that A is entirely included and B is entirely not.
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.
Interesting, clearly my intuition of what these sets really contain needs to continue evolving.
These multipliers are incredibly cheap -- and we don't have to achieve the notational elegance of the dissertation.
I would be tempted to go at it with six multipliers - one for every possible term. That way, any possible operation could be defined with them.
If you choose a set of constants that produce a non-manifold result, will Manifold perform checks, or could you get it to go ahead and dump out the result?
If it will still output the result, it would be cool to create six figures, each illustrating what is really in each set. Something like two overlapping spheres would suffice.
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.
X12 is the new verts from intersections of edges of A with faces of B - X21 is faces of A with edges of B. X03 is just the retained original verts from A, while X30 is those from B. w03/w30 is the winding number of those verts with respect to the other manifold.
Let's add just enough to make this operation work. Please stop asking for non-manifold output - that's a hard no, as it'll break every assumption in the library. It's a feature (the primary feature), not a bug.
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.
I wasn't seriously asking for non-manifold output here. Sorry to have come across as not getting the message...
I was thinking here in terms of a temporary hack that could produce the figures I described. And if we came up with other reasonable outputs in the future, all possible combinations could be defined in the six constants and we wouldn't have to go through the further code changes again.
What I don't understand about X12/X21/X30/X03 is how the duplicated locations are resolved.
If X30 contains all the faces from the original A, and X12 includes split faces of A around the intersections, how does the simple combination operator figure out what to keep?
It seems that X30 may only contain the faces of A that are not involved in any sort of intersection. In that case, if we visualized it, we would see a gap in X30 about one triangle wide that goes around the intersection curve.
We would then see that X12/X21 covers that gap, but with split triangles -- on each side.
But that doesn't handle the faces of A that are entirely within B, but not near the intersection curve.
So then X30 should contain only faces from original A that are entirely outside of B.
But this would require X12 to include not only the split triangles of A, but also any faces of A buried entirely in B.
For me, a figure that illustrates this would make it immediately clear.
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.
A diagram would be great, but I don't have a good way to draw one at the moment. The fundamental misunderstanding is you're thinking of these X values as pertaining to faces - they don't, they pertain to vertices. The included verts are used to build up edges, and then faces as part of the assembly. So, the four X sets we're talking about are entirely disjoint. All operations must include all of X12 and X21 (though potentially with reversed sign/direction). The only difference is which portions of X03 and X30 to include, which for a Boolean is by winding number, but for stamp is not.
src/utilities/include/public.h
Outdated
@@ -321,7 +321,7 @@ struct Box { | |||
/** | |||
* Boolean operation type: Add (Union), Subtract (Difference), and Intersect. | |||
*/ | |||
enum class OpType { Add, Subtract, Intersect }; | |||
enum class OpType { Add, Subtract, Intersect, KeepP, KeepQ }; |
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.
I'd guess we only need one new Op - this API is private anyway. For the public Manifold API, I'm thinking something like Manifold Manifold::Stamp(const Manifold& stamp, uint32_t originalID) const
, which would apply the given originalID to the portion of the manifold that's inside the stamp manifold.
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.
I started with just one. But then I thought of the utility of getting both in one go -- like the Split() call that gives you the difference and intersection in one go.
My assumption was that to get both in one go, you would first intersect, then get a Results with KeepP, and then KeepQ.
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.
True, but can you think of a practical use for that pair of operations?
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.
My use case was that if I later came up with a non-manifold operation I wanted to perform, this would give me all of the raw materials I could need in the most computationally efficient way.
The intersection line and re-triangulation would be done on both A and B. And if appropriate tagging is performed, we would have clear marking of A out of B, A in B, B out of A, and B in A.
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.
To be clear, my request here is not asking for non-manifold output from the library. I am suggesting an API like Split()
. That returns two manifold outputs.
I haven't looked closely at the benchmarking to see if you separate the times required to do the intersection and triangulation vs. the results preparation. However I really like the idea that these steps are separated and that one could generate multiple results from a single intersection and triangulation (like in Split()
). I also like the ability to create consistent triangulations between 'matching' operations.
I don't have a use case for a master BooleanAll function that returns the Union, Intersection, A-B and B-A all in one operation -- but deep down, I really want it and I feel like I'll have a use for it someday....
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.
That's fine, but one of my design principals for this library is to not introduce code complexity (especially without full code coverage in our tests) for potential features we haven't determined are useful yet. It's easy enough to come back and add it in later - until then, think about finding a use case.
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.
Unlike the BooleanAll, I do have a use case for this one. It is to allow non-manifold operations by users (outside of the library) without breaking Manifold's design assumption / assurance of manifold only.
This particular example use case also depends on a working thin-manifold solution. If I was working with a wing with ribs and spars initially defined by planes that extend beyond the surface of the wing. First, I need to trim the ribs/spars to the wing's surface and keep the part of the ribs/spars inside the wing. I also need the wing to be 'impressed' wherever the ribs/spars intersect the wing.
Next, I need to intersect all the ribs/spars with each other -- to create connectivity. But I don't want to discard any of the tris. I would then iterate over all the 'parts' to mutually intersect them, without throwing away any tris.
Every part would be manifold before, during, and after the calls to Manifold. They would all end up appropriately trimmed and would have appropriate intersection lines impressed on them.
Finally, I could write them out to a FEM file for application of materials, boundary conditions, and loads, etc. The FEM model is non-manifold, but all the parts are manifold and Manifold never has to see the non-manifold combination.
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.
I desire this flexibility too, and having a way to do it woudl be great, and llok like @ramcdona has a way to do it without breaking Manifold's design assumption. <3
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.
It also sounds a lot like returning A-B and B-A. As for creating your own connectivity, I have the feeling that won't be as easy as it sounds, particularly because of removal of short edges, which won't happen the same way on the separate outputs.
For the wing-rib example, I don't believe this would be any different than subtracting the ribs from the wing and then reconnecting the separated parts (same problem as above). Anyway, let's start small and work our way up. You're welcome to create a Sample that demonstrates this kind of use and then we can talk concretely about how much we can improve it with various API changes. For this PR, let's just make a Stamp
method to keep it easiest to review.
While these changes look vaguely reasonable from a distance, no attempt has been made to make these truly correct. They might be close, but there is low confidence of even that.
…ered. Expanding to six coefficients allows easier experimentation on the values. Once the appropriate values for KeepP are established, any rows that are identical can be consolidated to simplify this down.
I've gone in and cleaned things up a bit. I broke the coefficient table into six values, that way all the appearances of the coefficients can be manipulated separately. Once appropriate values for KeepP are determined, then any identical rows that remain can be consolidated. I spent quite a while playing around with different values, but I have not settled on a set that gives the desired behavior. If it is possible with just these values (which seems logical), it is going to take someone who understand the different sets better than I do. |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #578 +/- ##
==========================================
- Coverage 91.18% 91.11% -0.07%
==========================================
Files 35 35
Lines 4570 4512 -58
==========================================
- Hits 4167 4111 -56
+ Misses 403 401 -2
☔ View full report in Codecov by Sentry. |
I would appreciate thoughts from those who understand this a bit better than I do. However, I think I've come to the conclusion that this isn't going to work. The problem is with the x12 and x21 verts. My understanding is that I_12 should be +1 when that point should be an end vertex and -1 when it should be a start vertex. For the KeepA operator, we need I_12 (e_A,f_B) to be both +1 and -1 as both halves of e_A need to be used in the result mesh. Conversely, the target value of I_12(f_A,e_B) is not defined as we need to use the node, but we don't actually want either side of the e_B edge. |
Yeah, I think you're right that we'll need a little more than just changing the inclusion numbers. If However, the bigger issue is that we'll need to bring in the concept of a new In any case, even if you only get as far as adding the API and some tests that appropriately fail, that will help. |
I spent about two hours reading I'm now confident that making the required changes is beyond me. I was optimistic when it was a matter of changing the inclusion coefficients, but as they say... it escalated quickly. As for creating a new/separate If the |
Let's close this for now - we can come back to it if there's more interest. |
I understand your desire to close this to keep things tidy. This feature is still of prime interest to me, but it is also beyond my ability to implement in Manifold. Perhaps you can add it to a post-it note stuck to your desk somewhere lest the idea get lost. |
This is a first idea for a new 'Boolean' mode that will intersect the geometries, but keep everything on one side and nothing on the other.