-
Notifications
You must be signed in to change notification settings - Fork 129
Make OriginalID tracking optional and opt-in #1252
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 8000 privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
@pca006132 I'm curious if we can create a test that actually runs faster with this change - I've yet to find one. I think that having to run |
What about the case where I have a |
I don't think you need to call |
It seems ok for primitives only. Thanks for clarification. |
I think the benefit will probably come from things like Minkowski or Minecraft blocks. I will try to play with this, maybe next week when I have more time. The extra I think there will definitely be performance issues here and this is not something we can easily fix now. Maybe we can delay this to another release? |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1252 +/- ##
==========================================
- Coverage 92.17% 92.17% -0.01%
==========================================
Files 31 31
Lines 5895 5901 +6
==========================================
+ Hits 5434 5439 +5
- Misses 461 462 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Yeah, sounds reasonable. |
Fixes #1217
This could be construed as a breaking change, though I think it's minor enough I'm going to let it slide in a minor release. The idea is that mesh ID tracking can slow things down for complicated trees of Booleans with lots of coplanar faces, and we shouldn't bother by default so folks who are not using properties or faceIDs can skip all of that. However, IDs are still necessary to make properties and such work, since we need to retain edges separating input meshes to avoid interpolating between inconsistent properties.
With this change, if you're tracking IDs / materials / properties, you'll need to call
AsOriginal()
on each Manifold, but it's natural to do so since on construction you already have to be callingOriginalID()
to record the value in whatever material map you use.AsOriginal()
also now takes an optional id parameter - with none given or negative it has the old behavior of making a new unique ID. But now you can set zero to opt back out of mesh ID tracking and allow more simplification. Likewise you can set multiple manifolds to have the same ID. This would not be a good idea if using vertex properties, but could be helpful if, e.g. your materials consist entirely of uniform values, like a color.