added alwaysAsOriginal option

[elalish]

This is more of just a test for now. I'm not a fan of using the ManifoldParams - in fact if anyone has a better pattern for those I'm totally open to suggestions. Always felt like a bit of a hack.

I'm finding this cuts the time for NonConvexNonConvexMinkowski from #666 by about 10x - from 1206ms to 111ms. It also cleans up the output, dropping the number of verts and tris by about 10x too. I was hoping it would help on other tests with lots of Boolean ops too, but instead I see making about a 10-20% increase in time, maybe from recalculating coplanar faces? I guess it probably only helps if there are lots of coplanar facing being generated. It would be helpful to see more examples of the Minkowski to see if that's actually just unique to this example.

@zalo - what do you think?

[codecov]

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (7d564cf) 91.66% compared to head (cf6e731) 91.64%.

Files	Patch %	Lines
src/manifold/src/boolean_result.cpp	80.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #671      +/-   ##
==========================================
- Coverage   91.66%   91.64%   -0.02%     
==========================================
  Files          37       37              
  Lines        4737     4742       +5     
==========================================
+ Hits         4342     4346       +4     
- Misses        395      396       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[pca006132]

Interesting, I thought AsOriginal will increase the number of triangles and vertices as the decimator cannot collapse edges from meshes with different Ids. Wonder why it is helping that much for coplanar faces.

For alternative to ManifoldParam, I think we should allow users to override it locally by adding an optional argument to every single public function... So multiple threads can have different ManifoldParam if needed. Not a clean solution but I don't think we have a lot of options with C++. Operator overload will be problematic though, maybe we can pass the param from the LHS as well. It will be a large refactor though (only a breaking change in terms of removing the global function, but can be annoying internally).

[zalo]

Interesting! If it helps in the coplanar face case, then I wonder if the Convex Decomposition-based Minkowski Sum will benefit too (since those are all coplanar 😅 (but perhaps also only in the NonConvex/NonConvex case))

I’ve tried adding a similarish arg to each function in my own CAD app (the keepShape(s) arg, since every op already has the side effect of adding its output to the current workspace, keepShape(s) prevents it from deleting the original)… I’m not thrilled with the idea anymore…

The idea of a mode or temporary property that can apply to all operations happening while it is “on” seems more sustainable… though, ideally there’s some way to limit the scope of these temporary parameter changes, so other portions of the program aren’t adversely affected.

Some way to define a “Context”, which would just be a scope where modified manifold params apply seems more ideal to me. I use them in the LibFive Unity plugin to automatically dispose of Trees that were created inside of it (since I couldn’t trust the normal C# destructors for some reason 🤔?).
The equivalent for bindings without scope entry/exit calls might be functions like startContext({options})/endContext().

[pca006132]

Thread-local variables can probably work with such startContext/endContext function, not sure if it is the ideal way to solve this.

[zalo]

I’d see context functions as pushing/popping from a stack of successively overriding ManifoldParams… I guess each thread could have its own copy of the stack? (I’m not an expert at C++ Threading so I’m not sure if this is possible 😅 )

[pca006132]

but the ManifoldParams itself is global, so you need thread_local.

[zalo]

Ahh, just looked it up; makes sense to me.

I think the current usage of ManifoldParams here is fine (since this new option is used internally for a special case where we can guarantee a ton of coincident faces).

Should possible refactors of ManifoldParams (as an arg in everything or to augment the behavior of scoped blocks of code) be another PR?

Until those go in, is this pattern where the original value is recorded, and then restored later sufficient to prevent unexpected behavior?

manifold/test/boolean_test.cpp

Line 331 in cac0610

bool oldDeterministic = ManifoldParams().deterministic;

(also, do test cases even need to restore the state of ManifoldParams upon completion, or does the test runner take care of that?)

[pca006132]

I think the refactoring should go in another thread, because when we perform internal multithreading (no pun intended) we will get the wrong thread local value, so internally we need to pass everything explicitly and it will touch many things.

[zalo]

Oh, perhaps the option to enable alwaysAsOriginal should be passed as an arg into BatchBoolean?

I can also see the sweep, fancy offset, and convex-decomposition minkowski functions benefitting from this, since they all use BatchBoolean with tons of coincidently faced manifolds.

[elalish]

Interesting discussion, but I'm still not convinced this PR is useful at all. @pca006132 AsOriginal is the opposite of what you think - it allows more edges to be collapsed because it removes all the different IDs. The more I thought about this the more I think I just happened to be using a very special edge case object. I'll be interested to see if any of you find realistic case where this actually makes a significant speedup.

I'm definitely not a fan of doing a huge refactor to support this, unless it really is a huge win for a lot of cases.

[pca006132]

Ah! Sorry I misremembered, I now get it.

I think the refactoring related to ManifoldParams is orthogonal to this PR, we probably should make this thread safe, and doing that requires refactoring.

[zalo]

Is there any way I can just override the IDs of the hulls to all be the same while creating them?
Or is it the intermediate manifolds produced during the BatchBoolean that are the issue?

Or better, if the mesh IDs of the original manifolds were preserved in the hull vertices, would that help with simplification?

[elalish]

@zalo what you're describing is basically what I'm doing here, but the expensive part is finding which faces are coplanar. Anyway, the bigger issue is the API complexity, which I'd rather just avoid. Have you found any more common examples where this actually gives a significant performance boost? If not, I'll just close this. It was an interesting idea, but I'm not so sure it's actually useful.

[zalo]

Sweeps, Decomposited Minkowski Sums, “Elegant” Offsets, and non-rounded offsets should all benefit from this, but I haven’t quite come back from holidays yet enough to profile all this 😅

I have noticed that any Convex Decomposition based operation I do has unnecessarily tessellated topology, and it would be great to have a mechanism to both speed that up, and reduce the topological complexity.

There’s a chance that it will generally speed up the Minkowski Sums significantly 🤔

[zalo]

I'm not noticing any speed differences in the NonConvex/NonConvex test ☹️
The topology is simpler, but the results appear to be identical to just calling .asOriginal() afterward...

Ah, the test gets faster, but the equivalent Python call does not...

Perhaps my testing setup is bad? I'm just setting ManifoldParams().alwaysAsOriginal = true; at the top of the Minkowski function...

Also, it doesn't seem like overriding my hulls' originalIDs with the below makes any difference:

int hullID = (int)ReserveIDs(1);

// Then later, when making hulls
const_cast<Impl&>(*curHull.GetCsgLeafNode().GetImpl()).meshRelation_.originalID = hullID;

[elalish]

Okay, it's as I suspected then - I got excited about one particular edge case when the method isn't actually general. Thanks for confirming.

[zalo]

Ahhh, I hadn't realized the NonConvex/NonConvex test changed... yeah, the reason it sped up is this:

Before:

After:

I didn't realize what you meant by coplanar triangles (I assumed it was Manifolds that were touching each other, not the 0-volume manifolds that are created from the minkowski sum of two coplanar triangles).

My New Naive Minkowski refactor has a check for them, but it's not clear to me that it's working yet... My output topology is still just as messy 😅

[pca006132]

Thinking about it, we can have a default mesh id 0 and are available for simplification, and other mesh ids are unique. I.e. make not as original an opt-in option, as a lot of users are probably not aware of this.

[elalish]

Thinking about it, we can have a default mesh id 0 and are available for simplification, and other mesh ids are unique. I.e. make not as original an opt-in option, as a lot of users are probably not aware of this.

Oh, that's an interesting thought. Care to expand on that a bit? What API do you envision?

[pca006132]

sorry I am still sick today, will expand on it later when I feel better

[pca006132]

we should probably open a discussion or issue about this though, the thread is getting long.