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: Batched cleanup 2 #29695

Merged
merged 4 commits into from
Oct 19, 2024
Merged

Conversation

gkjohnson
Copy link
Collaborator

Related issue: #29687

Description

Re-adds the remaining changes from #29687:

  • Rename drawInfo to instanceInfo for clarity
  • Rename local count variable to multiDrawCount in onBeforeRender for clarity
  • Remove unneeded global variables
  • Add unusedVertexCount, unusedIndexCount, and instanceCount variables so applications can have insight into whether a geometry can be added without errors being thrown.
  • Fix copy-paste error.
  • If a geometry has not been initialized then don't mark it as initialized in setGeometrySize.

With this PR the class publicly exposes everything needed in order to accommodate rendering 3d tiles with BatchedMesh, which requires quickly adding and removing tiles from the batched mesh. Specifically the approach requires:

  • Checking if there are any unused ids that have enough space to fit the new tile (uses getGeometryRangeAt).
  • If one does not exist then we just add the tile geometry if there's enough space (checking the unused*Count members).
  • If there's not enough space then optimize the geometry.
  • If there's still not enough space then expand the geometry.

Overall it works pretty well aside from some ArrayTexture hiccups which are in-progress.

@gkjohnson gkjohnson requested a review from Mugen87 October 19, 2024 03:27
Copy link

📦 Bundle size

Full ESM build, minified and gzipped.

Before After Diff
WebGL 690.9
171.07
691.23
171.12
+331 B
+53 B
WebGPU 816.55
220.03
816.88
220.05
+331 B
+21 B
WebGPU Nodes 816.06
219.9
816.39
219.92
+331 B
+21 B

🌳 Bundle size after tree-shaking

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

Before After Diff
WebGL 463.45
111.9
463.45
111.9
+0 B
+0 B
WebGPU 538.64
145.42
538.64
145.42
+0 B
+0 B
WebGPU Nodes 494.75
135.28
494.75
135.28
+0 B
+0 B

@Makio64
Copy link
Contributor

Makio64 commented Oct 19, 2024

Thanks for cleaning !

Just there is something I don’t really understand in the tiles logic you re describing. In the games and editor I’m using batchmesh I have a pool of instance and I checkout/checkin instance, update the visibility/ set the correct geometryID and done.
If the pool is empty I extend the pool and the batchedmesh.

This approach keep the code minimalist and easy and I though tiles system architecture would be the same?

I was wondering if this approach could make this class much more simple than dealing with range unused etc manually as you’re describing ?

@gkjohnson
Copy link
Collaborator Author

3D Tiles streams geometry data in and out of memory as the camera moves since the whole set of data cannot be loaded at once. Inserting the newly loaded geometry and textures must be as fast as possible to keep the frame rate up. You also can't really know ahead of time how large all the tiles are in a general case, though we try to allocate a minimum amount of space ahead of time per tile assuming it will be easier to fit another tile there later, which is faster. Every tile geometry is also considered unique and no geometry is being rendered with multiple instances.

So when a new tile geometry is loaded we try to add it to the BatchedMesh with the quickest method possible. So that results in the fallback approach listed above - trying to find space if it exists, and then trying some more expensive approaches to make space (optimizing, expanding geometry in-place).

I have a pool of instance and I checkout/checkin instance, update the visibility/ set the correct geometryID and done.

If the pool is empty I extend the pool and the batchedmesh.

It's not clear from your description whether your delete geometry data from the batched mesh. If you're not then you're avoiding the hard part of the problem I'm describing. If you delete geometries (or are left with unused, undeleted geometry) there will be dead space left in the middle of the geometry buffers that BatchedMesh won't try to automatically use. If you don't optimize / repack those buffers or at least manually try to fit other geometry in that space then it's memory.

@Makio64
Copy link
Contributor

Makio64 commented Oct 19, 2024

Thanks for taking the time to explain your problematic ! Now your use case is much more clear and this PR totally makes sense!

Also you re right in my use case I just load all geometries once and then just play with the instances as the games have finite number of geometries

@gkjohnson gkjohnson merged commit 6e161d5 into mrdoob:dev Oct 19, 2024
12 checks passed
@gkjohnson gkjohnson deleted the batched-cleanup-2 branch October 19, 2024 08:51
@mrdoob mrdoob added this to the r170 milestone Oct 21, 2024
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