BufferAttribute: Add support for multiple update ranges #27103

gkjohnson · 2023-11-02T09:13:20Z

Description

Adds support for updating multiple ranges in an attribute buffer. It's the users responsibility to make sure there are no overlapping or duplicate ranges uploaded.

Deprecates BufferAttribute.updateRange.
adds BufferAttribute.updateRanges and associated clear and add functions.
Update tests.
Update two example files.
Update WebGLAttributes to support updateRanges.
Update WebGPUAttributeUtils to support updateRanges.

I will update other docs once this is merged.

github-actions · 2023-11-02T09:16:17Z

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize `dev`	Filesize PR	Diff
655.7 kB (162.4 kB)	656.4 kB (162.6 kB)	+672 B

🌳 Bundle size after tree-shaking

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

Filesize `dev`	Filesize PR	Diff
448.4 kB (108.4 kB)	448.9 kB (108.6 kB)	+484 B

gkjohnson · 2023-11-02T13:45:39Z

src/core/BufferAttribute.js

@@ -360,7 +380,8 @@ class BufferAttribute {

 		if ( this.name !== '' ) data.name = this.name;
 		if ( this.usage !== StaticDrawUsage ) data.usage = this.usage;
-		if ( this.updateRange.offset !== 0 || this.updateRange.count !== - 1 ) data.updateRange = this.updateRange;
+		if ( this._updateRange.offset !== 0 || this._updateRange.count !== - 1 ) data.updateRange = this._updateRange;


Is this needed if we're aiming for backwards compatibility for 10 releases?

Yes, let's keep this until updateRange is completely removed.

Can't think of a use case when this is needed in the json though...
Maybe we can skip adding this.updateRanges this time?

Good point. Update rangers are resetted after the render has processed the frame so it's not a typical data that you serialize for a scene graph structure.

Great - I removed toJSON and BufferGeometryLoader support for both the old updateRange and updateRanges

examples/jsm/renderers/webgpu/utils/WebGPUAttributeUtils.js

src/renderers/webgl/WebGLAttributes.js

src/core/BufferAttribute.js

Mugen87

Looks good to me!

@mrdoob Are you okay with changing the API like proposed in this PR? This new feature will be useful for BatchedMesh since it enables more efficient updates without rearranging buffer data (which would be too expensive anyway, see #27080 (comment)).

gkjohnson · 2023-11-03T09:20:45Z

This new feature will be useful for BatchedMesh since it enables more efficient updates without rearranging buffer data

It's also useful for large dynamic geometries even without BatchedMesh. For example this another case where I was updating geometry in a ring-buffer style fashion but was unable to upload all the changes in one frame when reaching the end of the array:

https://twitter.com/garrettkjohnson/status/1642890723531055104

aardgoose · 2023-11-03T09:23:02Z

Do you want to add support in the WebGPURenderer WebGL fallback?

examples/jsm/renderers/webgl/utils/WebGPUAttributeUtils.js

gkjohnson · 2023-11-03T10:38:51Z

Do you want to add support in the WebGPURenderer WebGL fallback?

examples/jsm/renderers/webgl/utils/WebGPUAttributeUtils.js

I'll add support in another PR since updateRange wasn't used, yet, and keep this PR focused on migrating existing usage.

aardgoose · 2023-11-03T13:47:21Z

Incidentally is there a typical size of update where the overhead of a number of small updates exceeds any gains from reduction of bytes copied?

Mugen87 · 2023-11-06T09:00:20Z

Not that I am aware of. Concrete numbers will also vary from device to device. I think projects need to find out by their own whether multiple update ranges makes sense for their use case or not.

gkjohnson · 2023-11-08T02:59:29Z

cc @mrdoob is this API okay to merge?

Before

only one update range is allowed

attr.updateRange.offset = 100;
attr.updateRange.count = 100;

After

multiple update ranges are allowed

attr.addUpdateRange( 100, 100 );
attr.addUpdateRange( 300, 100 );

attr.updateRanges.length // 2

mrdoob · 2023-11-08T08:28:01Z

src/core/BufferAttribute.js

+	addUpdateRange( offset, count ) {
+
+		this.updateRanges.push( { offset, count } );
+
+	}


Can we name it start instead of offset this time?

addUpdateRange( start, count ) { this.updateRanges.push( { start, count } ); }

mrdoob · 2023-11-08T08:41:55Z

I've added a couple of comments, but the new API looks good to me 👍

gkjohnson · 2023-11-08T10:20:34Z

Made the changes - merging!

LeviPesin · 2024-01-11T12:03:46Z

src/core/BufferAttribute.js

@@ -42,6 +43,13 @@ class BufferAttribute {

 	}

+	get updateRange() {


@gkjohnson Please add a // @deprecated, r159 comment here.

Feel free to make a PR. This one is merged

Note that since this only defines a getter, setting updateRange is in fact broken in r159.

This broke our project and we were able to work around it easily, but I think it would be nice if the release notes for r159 were updated to mention the sudden breaking change for setting the value. As of right now, they only mention a new feature without mentioning any breaking changes:

Add support for multiple update ranges. #27103, #27148, #27149 (@gkjohnson)

https://github.com/mrdoob/three.js/releases/tag/r159

(Or you could introduce a setter with a deprecation message.)

Breaking changes are separately listed in the migration guide along with the migration tasks: https://github.com/mrdoob/three.js/wiki/Migration-Guide#r158--r159

Aha! Unfortunately, that updateRange does not appear on this page when I search for it. I had to spend a few minutes trying to figure out why setting updateRange wasn't working, because nothing in the release notes actually mentioned the removal of the setter. While I understand the appeal of a condensed migration reference, it feels like the release notes themselves are sort of pretending that certain changes don't exist — when in fact those are the most impactful changes for existing codebases.

Would it be reasonable to inline a copy of the migration changes into the release notes, or to include the breaking changes directly as bullets in the list of changes? I think this would make the release notes a lot more intuitive, since other projects include breaking changes in the release notes themselves.

LeviPesin · 2024-01-11T12:04:09Z

src/core/InterleavedBuffer.js

@@ -28,6 +29,13 @@ class InterleavedBuffer {

 	}

+	get updateRange() {


LeviPesin · 2024-01-11T12:05:39Z

src/renderers/webgl/WebGLAttributes.js

+
+		}
+
+		// deprecated


gkjohnson added 3 commits November 2, 2023 17:45

Replace "updateRange" with "updateRanges"

7f2e1db

Fix upload logic

70b49f0

Add updateRanges support to BufferGeometryLoader

34d36dc

gkjohnson changed the title ~~BufferAttribute: Update ranges~~ BufferAttribute: Add support for multiple update ranges Nov 2, 2023

Clear attributes after upload, fix update condition

736b362

gkjohnson added this to the r159 milestone Nov 2, 2023

gkjohnson added 7 commits November 2, 2023 18:31

Update warnings, update InterleavedBuffer

623733e

Update tests

7a10fff

Update examples

72da889

Fix example

1c42fce

Fix upload logic

3c11a14

Update webgpu support

5b72ae7

Clear update ranges

a2d5c3d

gkjohnson commented Nov 2, 2023

View reviewed changes

Update comment

df4113f

gkjohnson marked this pull request as ready for review November 2, 2023 13:49

gkjohnson requested a review from Mugen87 November 2, 2023 13:49

Mugen87 reviewed Nov 2, 2023

View reviewed changes

src/renderers/webgl/WebGLAttributes.js Show resolved Hide resolved

Mugen87 reviewed Nov 2, 2023

View reviewed changes

src/core/BufferAttribute.js Outdated Show resolved Hide resolved

Update logs

6a977e9

Mugen87 approved these changes Nov 3, 2023

View reviewed changes

gkjohnson requested a review from mrdoob November 4, 2023 00:15

mrdoob reviewed Nov 8, 2023

View reviewed changes

gkjohnson added 6 commits November 8, 2023 18:44

Merge remote-tracking branch 'mrdoob/dev' into update-ranges

b468b24

Switch "offset" to "start"

3f283ca

Remove toJSON values

c2e93ca

Fix interleavedBuffer and WebGPUAttributeUtils

38aa457

Remove use of updateRange from tests, BufferGeometryLoader

aa1547f

Remove update ranges from loader test

d6e86d9

gkjohnson merged commit 4f6ecee into mrdoob:dev Nov 8, 2023
19 checks passed

gkjohnson deleted the update-ranges branch November 8, 2023 10:20

This was referenced Nov 8, 2023

Docs: Add docs for updateRanges #27148

Merged

WebGPURenderer: Add "updateRanges" support for WebGLAttributeUtils #27149

Merged

This was referenced Nov 9, 2023

BufferAttribute: Add support for multiple update ranges three-types/three-ts-types#676

Merged

r159 three-types/three-ts-types#661

Closed

Mugen87 mentioned this pull request Dec 10, 2023

Proposal: InstancedMesh.updateRanges -> array of update ranges #26562

Closed

LeviPesin reviewed Jan 11, 2024

View reviewed changes

src/core/InterleavedBuffer.js

@@ -28,6 +29,13 @@ class InterleavedBuffer {

}

get updateRange() {

Copy link

Contributor

LeviPesin Jan 11, 2024

Choose a reason for hiding this comment

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

Same here.

LeviPesin reviewed Jan 11, 2024

View reviewed changes

src/renderers/webgl/WebGLAttributes.js

}

// deprecated

Copy link

Contributor

LeviPesin Jan 11, 2024

Choose a reason for hiding this comment

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

Same here.

LeviPesin mentioned this pull request Jan 12, 2024

WebGLAttributes: Add .updateRange deprecation comment #27550

Merged

aardgoose mentioned this pull request Sep 2, 2024

WebGLRenderer: Remove deprecated BufferAttribute.updateRange() and InterleavedBuffer.updateRange(). #29298

Merged

RenaudRohlinger mentioned this pull request Nov 26, 2024

WebGPUAttributeUtils: fix updateAttribute() when using range (#29966) #29967

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BufferAttribute: Add support for multiple update ranges #27103

BufferAttribute: Add support for multiple update ranges #27103

gkjohnson commented Nov 2, 2023 •

edited

Loading

github-actions bot commented Nov 2, 2023 •

edited

Loading

gkjohnson Nov 2, 2023

Mugen87 Nov 2, 2023

mrdoob Nov 8, 2023

Mugen87 Nov 8, 2023 •

edited

Loading

gkjohnson Nov 8, 2023

Mugen87 left a comment

gkjohnson commented Nov 3, 2023

aardgoose commented Nov 3, 2023

gkjohnson commented Nov 3, 2023 •

edited

Loading

aardgoose commented Nov 3, 2023

Mugen87 commented Nov 6, 2023

gkjohnson commented Nov 8, 2023 •

edited by mrdoob

Loading

mrdoob Nov 8, 2023

gkjohnson Nov 8, 2023

mrdoob commented Nov 8, 2023

gkjohnson commented Nov 8, 2023 •

edited

Loading

LeviPesin Jan 11, 2024

gkjohnson Jan 11, 2024

lgarron Jan 16, 2024 •

edited

Loading

Mugen87 Jan 16, 2024

lgarron Jan 16, 2024

LeviPesin Jan 11, 2024

LeviPesin Jan 11, 2024

		@@ -42,6 +43,13 @@ class BufferAttribute {

		}

		get updateRange() {

		@@ -28,6 +29,13 @@ class InterleavedBuffer {

		}

		get updateRange() {

BufferAttribute: Add support for multiple update ranges #27103

BufferAttribute: Add support for multiple update ranges #27103

Conversation

gkjohnson commented Nov 2, 2023 • edited Loading

github-actions bot commented Nov 2, 2023 • edited Loading

📦 Bundle size

🌳 Bundle size after tree-shaking

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mugen87 Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mugen87 left a comment

Choose a reason for hiding this comment

gkjohnson commented Nov 3, 2023

aardgoose commented Nov 3, 2023

gkjohnson commented Nov 3, 2023 • edited Loading

aardgoose commented Nov 3, 2023

Mugen87 commented Nov 6, 2023

gkjohnson commented Nov 8, 2023 • edited by mrdoob Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrdoob commented Nov 8, 2023

gkjohnson commented Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgarron Jan 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkjohnson commented Nov 2, 2023 •

edited

Loading

github-actions bot commented Nov 2, 2023 •

edited

Loading

Mugen87 Nov 8, 2023 •

edited

Loading

gkjohnson commented Nov 3, 2023 •

edited

Loading

gkjohnson commented Nov 8, 2023 •

edited by mrdoob

Loading

gkjohnson commented Nov 8, 2023 •

edited

Loading

lgarron Jan 16, 2024 •

edited

Loading