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

deduplicate models bounding box computation #318

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sdumetz
Copy link
Contributor

@sdumetz sdumetz commented Oct 23, 2024

Specifically BufferGeometry.computeBoundingBox, which sets BufferGeometry.boundingBox, looks like it is not used anywhere.

The code uses computeLocalBoundingBox, using more or less the same algorithm but with additional tree traversal and matrix composition logic.

fwiw, each of those can take up to 100ms to compute on large-ish models so it can add up quickly. Ideally i'd totally skip this if the box can be retrived from the svx document or at least compute only the Thumb model's box and roll with it, but that'd be a more substantial change for another time.

@sdumetz sdumetz changed the title don't compute models bounding box deduplicate models bounding box computation Oct 23, 2024
@sdumetz sdumetz force-pushed the no_boundingbox branch 2 times, most recently from f142b47 to 4815932 Compare October 24, 2024 07:10
@gjcope
Copy link
Collaborator

gjcope commented Nov 4, 2024

Is this really saving anything or just shifting the overhead?

When the scene bounds get recomputed in CVScene

box.expandByObject(model.object3D, true);

expandByObject is used. Looking at that source, if the bounding box is not set in the Three object/geometry, it's going to get called anyway for each object.

https://github.com/mrdoob/three.js/blob/dev/src/math/Box3.js#L192

@sdumetz
Copy link
Contributor Author

sdumetz commented Nov 5, 2024

I should test this more.

It looked like the computation was removed but I didn't go through the whole flame graph so it might have just been moved.

I thought we were in this case https://github.com/mrdoob/three.js/blob/dev/src/math/Box3.js#L171 with precise == true, where the object's bounding box wasn't used but on closer inspection we might not be.

I'll test again to check if I missed something.

@gjcope
Copy link
Collaborator

gjcope commented Nov 5, 2024

Ah, yeah sorry I accidentally linked a proof-of-concept branch above where the precise bounds were needed. That could be another option, but obviously the optimization value would be different.

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.

2 participants