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

WebGLRenderer: Add uploadGeometry function #27080

Closed
wants to merge 1 commit into from

Conversation

gkjohnson
Copy link
Collaborator

@gkjohnson gkjohnson commented Oct 29, 2023

Related issue: #27078

Description

Adds WebGLRenderer.uploadGeometry which is for geometry as compile is for materials and initTexture is for textures. It uploads any necessary buffer attributes, respecting any "updateRange" values if needed.

I've run into this issue a number of times but it's particularly noticeable with frequent BatchedMesh geometry updates (see demo in #27078, also mentioned here) since any change in loaded geometry means reuploading the entire large attribute buffer which can be unnecessarily slow and ultimately undermine any benefits from the batched mesh in the first place. Instead if you only update 4 out of 800 geometries in a BatchedMesh you should only upload the changed ones immediately, which this enables:

const attr = mesh.geometry.attributes.position;
attr.updateRange.start = 100;
attr.updateRange.count = 100;
renderer.updateGeometry( mesh );

In the tiles renderer demo I'm doing this to force an update of the geometry on change since you can't specify multiple update ranges:

// Force an upload of the given range
const visible = mesh.visible;
mesh.visible = true;
mesh.geometry.setDrawRange( 0, 3 );
renderer.render( mesh, _camera );
mesh.geometry.setDrawRange( 0, Infinity );
mesh.visible = visible;

I can add documentation if this is merged.

@gkjohnson gkjohnson added this to the r159 milestone Oct 29, 2023
@github-actions
Copy link

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize dev Filesize PR Diff
652.4 kB (161.7 kB) 652.5 kB (161.7 kB) +92 B

🌳 Bundle size after tree-shaking

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

Filesize dev Filesize PR Diff
445 kB (107.7 kB) 445.1 kB (107.7 kB) +92 B

@Mugen87
Copy link
Collaborator

Mugen87 commented Oct 29, 2023

I'm not sure about introducing this method on renderer level. Especially since we need to carry it over to WebGPURenderer.

When I understand the issue correctly, the limited factor right now is that updateRange only accepts one definition. Is a new method on renderer level still required if update range would accept multiple entries?

Besides, could BatchedMesh internally sort buffer data before rendering? So that the data which are actually rendered are placed first in the buffers? Or is this too expensive?

@gkjohnson
Copy link
Collaborator Author

gkjohnson commented Oct 29, 2023

When I understand the issue correctly, the limited factor right now is that updateRange only accepts one definition. Is a new method on renderer level still required if update range would accept multiple entries?

I'd be happy with supporting multiple updateRanges per geometry, as well, and would allow this to happen more transparently.

Besides, could BatchedMesh internally sort buffer data before rendering? So that the data which are actually rendered are placed first in the buffers? Or is this too expensive?

It is definitely too expensive. See #27078 (comment). If it wasn't then batching with something like multidraw arrays would be useless. You'd just merge and then rearrange geometry, instead.

@gkjohnson
Copy link
Collaborator Author

gkjohnson commented Oct 31, 2023

I'd be happy with supporting multiple updateRanges per geometry, as well, and would allow this to happen more transparently.

@Mugen87 is this something you can support? This is what I'm imagining:

  • A new API called addUpdateRange and clearUpdateRanges and (maybe) optimizeUpdateRanges in addition to an .updateRanges field.
  • WebGLRenderer runs all update ranges on render
  • Make it backward compatible by making the .updateRange field add and modify the first element in the updateRanges list.

@Mugen87
Copy link
Collaborator

Mugen87 commented Nov 1, 2023

The methods sounds good to me. However, I would leave out optimizeUpdateRanges for now.

If we add updateRanges, we should definitely deprecated updateRange. The engine doesn't need both.

@gkjohnson
Copy link
Collaborator Author

If we add updateRanges, we should definitely deprecated updateRange. The engine doesn't need both.

Agreed - I figured we would have backwards compatibility support for the typical 10 releases, though.

I'll make a PR when I get the chance.

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