Skip to content

Texture: Add updateRanges. #30998

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

Open
wants to merge 8 commits into
base: dev
Choose a base branch
from

Conversation

CodyJasonBennett
Copy link
Contributor

@CodyJasonBennett CodyJasonBennett commented Apr 25, 2025

Related issue: #30184

Description

Adds an update ranges API to textures, mirroring BufferAttribute. Note that the current implementation assumes update ranges refer to contiguous blocks or rows of memory; non-contiguous updates should be split into multiple update ranges. Adjacent update ranges are merged similarly to #29189 with additional checks according to the texture image layout.

A good application of partial updates to textures would be BatchedMesh, where updating a single object's matrix or visibility flushes the entire texture, which is very slow and can cause a memory management event (crash!). It should serve as a good testbed for usability/performance with the webgl_mesh_batch example.

@CodyJasonBennett CodyJasonBennett marked this pull request as draft April 25, 2025 17:02
@CodyJasonBennett
Copy link
Contributor Author

I am sure the indirection texture in BatchedMesh can be updated in a smarter way to leverage this API.

Copy link

github-actions bot commented Apr 25, 2025

📦 Bundle size

Full ESM build, minified and gzipped.

Before After Diff
WebGL 336.29
78.32
337.26
78.64
+970 B
+320 B
WebGPU 548.37
152.03
548.37
152.03
+0 B
+0 B
WebGPU Nodes 547.72
151.88
547.72
151.88
+0 B
+0 B

🌳 Bundle size after tree-shaking

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

Before After Diff
WebGL 466.23
112.58
467.33
112.95
+1.11 kB
+364 B
WebGPU 623.15
168.73
623.28
168.77
+130 B
+40 B
WebGPU Nodes 578.02
158.02
578.15
158.06
+130 B
+41 B

@agargaro
Copy link
Contributor

agargaro commented Apr 29, 2025

Great work, just a couple of thoughts:

I am afraid that calling addUpdateRange every time a color/matrix is changed, could slow down a lot when changing many/all instances.

In my case, I would like to freely manage the selection of update ranges.

The best choice would be to have the user call addUpdateRange directly, although I understand that it is less user friendly.
We could also consider a flag but would add complexity.

IndirectTexture is updated every frame except if ! this._visibilityChanged && ! this.perObjectFrustumCulled && ! this.sortObjects, recalculating all the indices of the instances to be rendered.

So being updated sequentially, we can calculate the two update ranges:

  • The first is all rows prior to the last one.
  • The second is the last partial row.

To avoid trouble with the garbage collector we might also consider reusing the same objects like BatchedMesh does with the MultiDrawRenderList, avoiding creating a new object for each update range.

It would also be nice to test on Firefox these changes, I had several problems when the update calls were many.

@agargaro
Copy link
Contributor

agargaro commented Apr 29, 2025

In the issue I opened I had suggested this signature:

.addUpdateRange ( region : Box2 )

but you implemented the same as BufferAttribute

.addUpdateRange ( start : number, count : number )

I prefer your approach because it makes things easier, but a user might also need to update a region. In that case it would be sufficent to create a separate method that creates multiple update ranges based on the number of rows, although it's probably not the best solution because it would make more update calls than necessary, but I think it's fine.

What do you think about using this syntax?

texSubImage2D(target, level, xoffset, yoffset, width, height, format, type, pixels, srcOffset);

// example how to update full consecutive rows
gl.texSubImage2D(gl.TEXTURE_2D, 0, 0, row, width, count, glFormat, glType, data, row * width * channels);

adding the offest as the last parameter, avoiding the overhead due to pixelStorei on firefox.

_gl.pixelStorei( _gl.UNPACK_SKIP_PIXELS, x );
_gl.pixelStorei( _gl.UNPACK_SKIP_ROWS, y );

However, I would have to test this and see if these methods are equivalent

@CodyJasonBennett
Copy link
Contributor Author

CodyJasonBennett commented Apr 30, 2025

I am afraid that calling addUpdateRange every time a color/matrix is changed, could slow down a lot when changing many/all instances.

This needs an experiment to inform and is why this PR is in draft status so we do not risk regression. It's possible we restrict to only a few ranges at most (or a decent heuristic) and greedily merge within BatchedMesh to orphan the least amount of memory. That is a fair amount of complexity I would prefer to hide, and data is piped very differently from buffer attributes, which go through a ring buffer. I expect optimizations to diverge as a result. It's notable that any method that can reallocate or change size is required to zero the target buffer. I prefer to use sub* update methods at runtime for this reason, whether WebGL or WebGPU.

In the issue I opened I had suggested this signature:

.addUpdateRange ( region : Box2 )

but you implemented the same as BufferAttribute

.addUpdateRange ( start : number, count : number )

I prefer your approach because it makes things easier, but a user might also need to update a region. In that case it would be sufficent to create a separate method that creates multiple update ranges based on the number of rows, although it's probably not the best solution because it would make more update calls than necessary, but I think it's fine.

The current implementation assumes that update ranges refer to contiguous rows of memory, but it can be modified to split non-contiguous rows of memory into separate calls (before the optimization pass). This should be an implementation detail. If overloads are okay, I would want to break this into another PR and mirror the API with BufferAttribute.

@CodyJasonBennett
Copy link
Contributor Author

CodyJasonBennett commented Apr 30, 2025

I am removing changes to BatchedMesh with cac5d42 so that it can be completed later with experiments. #30184 (comment)

It would help to have a minimal example showcasing a partial update to Texture, also for quality control purposes.

@CodyJasonBennett CodyJasonBennett marked this pull request as ready for review April 30, 2025 20:46
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