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

InstancedMesh: Add bounding volumes. #21507

Closed
wants to merge 1 commit into from
Closed

Conversation

Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented Mar 24, 2021

Related issue: Fixed #18334.

Description

This PR adds a boundingBox and boundingSphere property as well as two new methods for bounding volume computation to InstancedMesh.

Since bounding volumes on geometry level are not sufficient for certain use cases like skinned and instanced meshes, the idea is to provide bounding volumes on object3D level for better results. I've tried to find alternative approaches but could not come up with a better solution for implementing proper bounding volumes for InstancedMesh and SkinnedMesh. The following policy is currently implemented:

  • Bounding volumes on object3D level supersede bounding volumes on geometry level.

  • When a geometry is created, bounding volumes are null. If the renderer needs bounding volumes e.g. for view frustum culling, it computes them once based on the current data. This is also true for bounding volumes on object3D level.

  • When application or engine logic changes geometry data, bounding volumes have to be recomputed, too. This is also true for bounding volumes at object3D level when respective data of InstancedMesh are changed (e.g. instanceMatrix). The examples are changed to reflect this pattern.

  • With this PR, InstancedMesh.frustumCulled is not set to false anymore. User who dynamically change instanced meshes now have to recompute at least the bounding sphere for proper view frustum culling.

A subsequent PR could try to enhance InstancedMesh.raycast() so bounding volumes are used.

Updated the build files for testing purposes.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Mar 24, 2021

@donmccurdy
Copy link
Collaborator

I like the approach of adding these properties and methods. A good solution for InstancedMesh, and seems likely to work well for SkinnedMesh too.

When a geometry is created, bounding volumes are null. If the renderer needs bounding volumes e.g. for view frustum culling, it computes them once based on the current data. This is also true for bounding volumes on object3D level.

In this line you appear to be saying the renderer will call mesh.computeBoundingSphere(), but throughout the examples it is now called manually — is this intentional?

Bounding volumes on object3D level supersede bounding volumes on geometry level.

To clarify — where a bounding volume on a Mesh exists, the bounding volume on its geometry will be completely ignored by raycaster and frustum checks. Is that correct? Just wanted to be sure "supersede" does not mean "expands" here, or something else.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Mar 28, 2021

In this line you appear to be saying the renderer will call mesh.computeBoundingSphere(), but throughout the examples it is now called manually — is this intentional?

If a bounding volume is null, the renderer triggers a computation.

The renderer does not compute them when they are already set. The problem is they might be outdated (because the user changed the transformation of instances). In this case, app-level code has to trigger the re-computation.

To clarify — where a bounding volume on a Mesh exists, the bounding volume on its geometry will be completely ignored by raycaster and frustum checks. Is that correct?

Yes! 😊

@arpu
Copy link

arpu commented May 8, 2022

anything needs to be done to get this in?

@lexengineer
Copy link

lexengineer commented May 25, 2022

Wondering if we plan to merge this any soon or if something still has to be done?

Seems to be useful and would be nice to have that

@mvilledieu
Copy link
Contributor

This is great and exactly what I need, are we still planning on merging this? Is there anything I can do to help getting this merged, happy to update the PR if needed?

Copy link
Contributor

@OndrejSpanel OndrejSpanel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks good to me, quite straightforward and functionality is really a necessary one.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Feb 23, 2023

@mrdoob If you are happy with the design, I can rebase the PR.

This change would also allow us to optimize InstancedMesh.raycast() by checking the bounding volumes first before testing all instances.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Mar 1, 2023

Couldn't properly rebase the PR in this branch so I created a new one. Closing in favor of #25591.

@Mugen87 Mugen87 closed this Mar 1, 2023
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.

InstancedMesh: Incorrect bounding volumes.
7 participants