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

BatchedMesh: add support for frustum culling per batched geometry #27120

Merged
merged 12 commits into from
Nov 8, 2023

Conversation

gkjohnson
Copy link
Collaborator

@gkjohnson gkjohnson commented Nov 5, 2023

Related issue: #22376, #27111

Description

Add support for frustum culling to BatchedMesh by storing the box and sphere bounds per geometry.

Upcoming PRs

  • Raycasting, sorting support
  • Object-level bounds
  • Add support to ObjectLoader
  • Move to core
  • Documentation

@gkjohnson gkjohnson added this to the r159 milestone Nov 5, 2023
@gkjohnson gkjohnson requested a review from Mugen87 November 5, 2023 12:51
@Mugen87
Copy link
Collaborator

Mugen87 commented Nov 6, 2023

So the PR adds a frustum culling check for each geometry of the batched mesh in onBeforeRender().

This is a different strategy to what is implemented in other classes. Normally, I would have expected bounding volumes on batched mesh level (similar to InstancedMesh or SkinedMesh). The computation of these bounding volumes happens automatically by the renderer (or or other components like in context of raycasting) when the access happens for the first time (meaning when their values are still null). Whenever the bounds require an updated, computeBoundingSphere() or computeBoundingBox() needs to be called on app level since the app updates the geometry data.

I think it would be more consistent to implement the bounding volume computation according to this policy for batched mesh as well.

@gkjohnson
Copy link
Collaborator Author

To be clear you're suggesting that we only generate bounding volumes on an as-needed basis for BatchedMesh, correct? I can adjust that if that's correct.

Whenever the bounds require an updated, computeBoundingSphere() or computeBoundingBox() needs to be called on app level since the app updates the geometry data.

This case is a bit different, though, since we're talking about a geometry that could have hundreds or thousands of bounding boxes - only one of them could be invalidated. I'm thinking adding an optional argument to "setGeometryAt" that indicates that the bounds should be invalidated is a good option.

# Conflicts:
#	examples/jsm/objects/BatchedMesh.js
@Mugen87
Copy link
Collaborator

Mugen87 commented Nov 6, 2023

on an as-needed basis for BatchedMesh, correct?

Yes.

This case is a bit different, though, since we're talking about a geometry that could have hundreds or thousands of bounding boxes - only one of them could be invalidated. I'm thinking adding an optional argument to "setGeometryAt" that indicates that the bounds should be invalidated is a good option.

BatchedMesh is indeed special compared to other 3D objects but I think it would be good to have bounds on object level so projectObject() in WebGL renderer can sort out the entire batched mesh if possible.

If you think it's important to have bounding volumes for each geometry of the batched mesh then I'm fine with that. But it would be good to add BachtedMesh.boundingSphere and BachtedMesh.boundingBox which enclose the entire batched mesh. Both volumes should be computed with BachtedMesh.computeBoundingSphere() and BachtedMesh.computeBoundingBox().

Ideally, frustum culling and raycasting test with the object level bounding volumes first, and then with the ones on geometry level.

@gkjohnson
Copy link
Collaborator Author

gkjohnson commented Nov 6, 2023

BatchedMesh is indeed special compared to other 3D objects but I think it would be good to have bounds on object level so projectObject() in WebGL renderer can sort out the entire batched mesh if possible.

I don't know if I agree that this is as useful - but I can support keeping the ability. I'd want the ability to disable that feature, though, since keeping the full bounding box up to date with many dynamic objects spread throughout the scene is not super useful. In this case how do you feel about a different field like "perObjectFrustumCulled" or "batchFrustumCulling" that can be set to toggle the the "onBeforeRender" frustum culling feature?

Also I just updated the PR to generate the boxes and spheres as-needed. When a geometry is replaced the existing bounding box is used or it's marked as needing to be regenerated.

@Mugen87
Copy link
Collaborator

Mugen87 commented Nov 6, 2023

In this case how do you feel about a different field like "perObjectFrustumCulled" or "batchFrustumCulling" that can be set to toggle the the "onBeforeRender" frustum culling feature?

That sounds good to me! However, in order to keep the class consistent I would be glad to see the object level features mentioned in #27120 (comment).

It's right that bounding volumes which enclose the entire batched mesh might not be useful for stuff like tiles rendering. But other scenarios might benefit from sorting out the entire batched mesh when rendering/raycasting. By implementing this approach, we keep the class consistent with InstancedMesh but can also provide more fine granular control over frustum culling with an additional property. perObjectFrustumCulled sounds good to me!

@gkjohnson
Copy link
Collaborator Author

Okay! I've just added "perObjectFrustumCulled" flag.

That sounds good to me! However, in order to keep the class consistent I would be glad to see the object level features mentioned in #27120 (comment).

I'll add this in another PR, then.

I guess one concern I have for things like raycasting or frustum culling when elements are dynamic (meaning the bounding volumes have to be regenerated every frame) is that the cost of regenerating the bounding box and / or sphere is more expensive than just checking the ray / frustum intersection with the child bounding boxes. Instancing would have the same issue, though I guess this would have to be tested.

@gkjohnson
Copy link
Collaborator Author

@Mugen87 I just updated the copy and toJSON functions to add support for bounds and perObjectFrustumCulled flag. This should be ready!

Copy link

github-actions bot commented Nov 7, 2023

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize dev Filesize PR Diff
655.5 kB (162.3 kB) 655.7 kB (162.4 kB) +281 B

🌳 Bundle size after tree-shaking

Minimal build including a renderer, camera, empty scene, and dependencies.

Filesize dev Filesize PR Diff
448.1 kB (108.3 kB) 448.4 kB (108.4 kB) +281 B

@gkjohnson gkjohnson merged commit 7863b73 into mrdoob:dev Nov 8, 2023
19 checks passed
@gkjohnson gkjohnson deleted the batched-frustum-culling branch November 8, 2023 09:42
.multiply( this.matrixWorld );
_frustum.setFromProjectionMatrix(
_projScreenMatrix,
_renderer.isWebGPURenderer ? WebGPUCoordinateSystem : WebGLCoordinateSystem
Copy link
Contributor

Choose a reason for hiding this comment

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

I think something like _renderer.backend !== undefined ? _renderer.backend.coordinateSystem : WebGLCoordinateSystem would be more correct. /ping @gkjohnson

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not familiar enough with WebGPURenderer to know

Copy link
Contributor

Choose a reason for hiding this comment

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

@sunag What do you think?

Copy link
Collaborator

@sunag sunag Jan 12, 2024

Choose a reason for hiding this comment

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

I think we can use _renderer.coordinateSystem, it is compatible with both renderers.

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.

4 participants