8000 Make OriginalID tracking optional and opt-in by elalish · Pull Request #1252 · elalish/manifold · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

elalish
Copy link
Owner
@elalish elalish commented May 6, 2025

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 calling OriginalID() 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.

@elalish elalish self-assigned this May 6, 2025
@elalish elalish requested a review from pca006132 May 6, 2025 10:40
@elalish
Copy link
Owner Author
elalish commented May 6, 2025

@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 MarkCoplanar after each Boolean op may be taking more time that is gained from having fewer faces to operate on. I'll be in favor of this if it actually aids performance, but otherwise the slight increase in API complexity may not be justified.

@jirihon
Copy link
Contributor
jirihon commented May 6, 2025

What about the case where I have a MeshGL storing the output of some previous boolean operation? Should I also call AsOriginal() after converting the MeshGL back into Manifold to enable properties? But that would assign single mesh ID to all the triangles and break the properties, wouldn't it?

@pca006132
Copy link
Collaborator

What about the case where I have a MeshGL storing the output of some previous boolean operation? Should I also call AsOriginal() after converting the MeshGL back into Manifold to enable properties? But that would assign single mesh ID to all the triangles and break the properties, wouldn't it?

I don't think you need to call AsOriginal in that case. We are just opting out of OriginID tracking for primitives.

@jirihon
Copy link
Contributor
jirihon commented May 6, 2025

I don't think you need to call AsOriginal in that case. We are just opting out of OriginID tracking for primitives.

It seems ok for primitives only. Thanks for clarification.

@pca006132
Copy link
Collaborator

@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 MarkCoplanar after each Boolean op may be taking more time that is gained from having fewer faces to operate on. I'll be in favor of this if it actually aids performance, but otherwise the slight increase in API complexity may not be justified.

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 MarkCoplanar after each Boolean op is probably an issue, especially when this function is not well optimized and sequential. This will probably make things slower instead. I'm thinking if it is possible to just mark the faces as different, e.g., add an offset to faces from Q. Ideally this already allows simplification of small features, and maybe we can let the CSG tree decide when to compute coplanar surfaces for optimization, based on the size of the tree and the number of operations etc.

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?

Copy link
codecov bot commented May 6, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.17%. Comparing base (e3ff2a0) to head (46e3c53).
Report is 2 commits behind head on master.

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@elalish
Copy link
Owner Author
elalish commented May 7, 2025

Yeah, sounds reasonable.

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

Successfully merging this pull request may close these issues.

opt-in mesh id for primitives
3 participants
0