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.

Sign up for GitHub

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

BufferAttribute: Add support for multiple update ranges #27103

Merged
merged 19 commits into from
Nov 8, 2023
Merged

BufferAttribute: Add support for multiple update ranges #27103

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
7f2e1db
Replace "updateRange" with "updateRanges"
gkjohnson Nov 2, 2023
70b49f0
Fix upload logic
gkjohnson Nov 2, 2023
34d36dc
Add updateRanges support to BufferGeometryLoader
gkjohnson Nov 2, 2023
736b362
Clear attributes after upload, fix update condition
gkjohnson Nov 2, 2023
623733e
Update warnings, update InterleavedBuffer
gkjohnson Nov 2, 2023
7a10fff
Update tests
gkjohnson Nov 2, 2023
72da889
Update examples
gkjohnson Nov 2, 2023
1c42fce
Fix example
gkjohnson Nov 2, 2023
3c11a14
Fix upload logic
gkjohnson Nov 2, 2023
5b72ae7
Update webgpu support
gkjohnson Nov 2, 2023
a2d5c3d
Clear update ranges
gkjohnson Nov 2, 2023
df4113f
Update comment
gkjohnson Nov 2, 2023
6a977e9
Update logs
gkjohnson Nov 3, 2023
b468b24
Merge remote-tracking branch 'mrdoob/dev' into update-ranges
gkjohnson Nov 8, 2023
3f283ca
Switch "offset" to "start"
gkjohnson Nov 8, 2023
c2e93ca
Remove toJSON values
gkjohnson Nov 8, 2023
38aa457
Fix interleavedBuffer and WebGPUAttributeUtils
gkjohnson Nov 8, 2023
aa1547f
Remove use of updateRange from tests, BufferGeometryLoader
gkjohnson Nov 8, 2023
d6e86d9
Remove update ranges from loader test
gkjohnson Nov 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 3 additions & 6 deletions examples/jsm/misc/TubePainter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,16 +176,13 @@ function TubePainter() {

if ( start === end ) return;

positions.updateRange.offset = start * 3;
positions.updateRange.count = ( end - start ) * 3;
positions.addUpdateRange( start * 3, ( end - start ) * 3 );
positions.needsUpdate = true;

normals.updateRange.offset = start * 3;
normals.updateRange.count = ( end - start ) * 3;
normals.addUpdateRange( start * 3, ( end - start ) * 3 );
normals.needsUpdate = true;

colors.updateRange.offset = start * 3;
colors.updateRange.count = ( end - start ) * 3;
colors.addUpdateRange( start * 3, ( end - start ) * 3 );
colors.needsUpdate = true;

count = geometry.drawRange.count;
Expand Down
25 changes: 15 additions & 10 deletions examples/jsm/renderers/webgpu/utils/WebGPUAttributeUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,9 +72,9 @@ class WebGPUAttributeUtils {
const buffer = backend.get( bufferAttribute ).buffer;

const array = bufferAttribute.array;
const updateRange = bufferAttribute.updateRange;
const updateRanges = bufferAttribute.updateRanges;

Mugen87 marked this conversation as resolved.
Show resolved Hide resolved
if ( updateRange.count === - 1 ) {
if ( updateRanges.length === 0 ) {

// Not using update ranges

Expand All @@ -87,15 +87,20 @@ class WebGPUAttributeUtils {

} else {

device.queue.writeBuffer(
buffer,
0,
array,
updateRange.offset * array.BYTES_PER_ELEMENT,
updateRange.count * array.BYTES_PER_ELEMENT
);
for ( let i = 0, l = updateRanges.length; i < l; i ++ ) {

const range = updateRanges[ i ];
device.queue.writeBuffer(
buffer,
0,
array,
range.start * array.BYTES_PER_ELEMENT,
range.count * array.BYTES_PER_ELEMENT
);

}

updateRange.count = - 1; // reset range
bufferAttribute.clearUpdateRanges();

}

Expand Down
3 changes: 1 addition & 2 deletions examples/webgl_simple_gi.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,8 +128,7 @@

}

attributes.color.updateRange.offset = startVertex * 3;
attributes.color.updateRange.count = ( currentVertex - startVertex ) * 3;
attributes.color.addUpdateRange( startVertex * 3, ( currentVertex - startVertex ) * 3 );
attributes.color.needsUpdate = true;

if ( currentVertex >= totalVertex ) {
Expand Down
23 changes: 21 additions & 2 deletions src/core/BufferAttribute.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,8 @@ class BufferAttribute {
this.normalized = normalized;

this.usage = StaticDrawUsage;
this.updateRange = { offset: 0, count: - 1 };
this._updateRange = { offset: 0, count: - 1 };
this.updateRanges = [];
this.gpuType = FloatType;

this.version = 0;
Expand All @@ -42,6 +43,13 @@ class BufferAttribute {

}

get updateRange() {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Feel free to make a PR. This one is merged

Copy link
Contributor

@lgarron lgarron Jan 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.


console.warn( 'THREE.BufferAttribute: "updateRange" is deprecated and removed in r169. Use "addUpdateRange()" instead.' );
return this._updateRange;

}

setUsage( value ) {

this.usage = value;
Expand All @@ -50,6 +58,18 @@ class BufferAttribute {

}

addUpdateRange( start, count ) {

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

}

clearUpdateRanges() {

this.updateRanges.length = 0;

}

copy( source ) {

this.name = source.name;
Expand Down Expand Up @@ -360,7 +380,6 @@ 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;

return data;

Expand Down
22 changes: 21 additions & 1 deletion src/core/InterleavedBuffer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,8 @@ class InterleavedBuffer {
this.count = array !== undefined ? array.length / stride : 0;

this.usage = StaticDrawUsage;
this.updateRange = { offset: 0, count: - 1 };
this._updateRange = { offset: 0, count: - 1 };
this.updateRanges = [];

this.version = 0;

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

}

get updateRange() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.


console.warn( 'THREE.InterleavedBuffer: "updateRange" is deprecated and removed in r169. Use "addUpdateRange()" instead.' );
return this._updateRange;

}

setUsage( value ) {

this.usage = value;
Expand All @@ -36,6 +44,18 @@ class InterleavedBuffer {

}

addUpdateRange( start, count ) {

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

}

clearUpdateRanges() {

this.updateRanges.length = 0;

}

copy( source ) {

this.array = new source.array.constructor( source.array );
Expand Down
7 changes: 0 additions & 7 deletions src/loaders/BufferGeometryLoader.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,13 +125,6 @@ class BufferGeometryLoader extends Loader {
if ( attribute.name !== undefined ) bufferAttribute.name = attribute.name;
if ( attribute.usage !== undefined ) bufferAttribute.setUsage( attribute.usage );

if ( attribute.updateRange !== undefined ) {

bufferAttribute.updateRange.offset = attribute.updateRange.offset;
bufferAttribute.updateRange.count = attribute.updateRange.count;

}

geometry.setAttribute( key, bufferAttribute );

}
Expand Down
34 changes: 30 additions & 4 deletions src/renderers/webgl/WebGLAttributes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,17 +84,43 @@ function WebGLAttributes( gl, capabilities ) {
function updateBuffer( buffer, attribute, bufferType ) {

const array = attribute.array;
const updateRange = attribute.updateRange;
const updateRange = attribute._updateRange; // deprecated
const updateRanges = attribute.updateRanges;

gl.bindBuffer( bufferType, buffer );

if ( updateRange.count === - 1 ) {
if ( updateRange.count === - 1 && updateRanges.length === 0 ) {

// Not using update ranges

gl.bufferSubData( bufferType, 0, array );

} else {
Mugen87 marked this conversation as resolved.
Show resolved Hide resolved
}

if ( updateRanges.length !== 0 ) {

for ( let i = 0, l = updateRanges.length; i < l; i ++ ) {

const range = updateRanges[ i ];
if ( isWebGL2 ) {

gl.bufferSubData( bufferType, range.start * array.BYTES_PER_ELEMENT,
array, range.start, range.count );

} else {

gl.bufferSubData( bufferType, range.start * array.BYTES_PER_ELEMENT,
array.subarray( range.start, range.start + range.count ) );

}

}

attribute.clearUpdateRanges();

}

// deprecated
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.

if ( updateRange.count !== - 1 ) {

if ( isWebGL2 ) {

Expand Down
6 changes: 2 additions & 4 deletions test/unit/src/core/BufferAttribute.tests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ export default QUnit.module( 'Core', () => {

} );

QUnit.todo( 'updateRange', ( assert ) => {
QUnit.todo( 'updateRanges', ( assert ) => {

assert.ok( false, 'everything\'s gonna be alright' );

Expand Down Expand Up @@ -296,16 +296,14 @@ export default QUnit.module( 'Core', () => {
const attr2 = new BufferAttribute( new Float32Array( [ 1, 2, 3, 4, 5, 6 ] ), 3, true );
attr2.name = 'attributeName';
attr2.setUsage( DynamicDrawUsage );
attr2.updateRange.offset = 1;
attr2.updateRange.count = 2;
attr2.addUpdateRange( 1, 2 );
assert.deepEqual( attr2.toJSON(), {
itemSize: 3,
type: 'Float32Array',
array: [ 1, 2, 3, 4, 5, 6 ],
normalized: true,
name: 'attributeName',
usage: DynamicDrawUsage,
updateRange: { offset: 1, count: 2 }
}, 'Serialized to JSON as expected with non-default values' );

} );
Expand Down
2 changes: 1 addition & 1 deletion test/unit/src/core/InterleavedBuffer.tests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ export default QUnit.module( 'Core', () => {

} );

QUnit.todo( 'updateRange', ( assert ) => {
QUnit.todo( 'updateRanges', ( assert ) => {

assert.ok( false, 'everything\'s gonna be alright' );

Expand Down
2 changes: 0 additions & 2 deletions test/unit/src/loaders/BufferGeometryLoader.tests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,6 @@ export default QUnit.module( 'Loaders', () => {
const attr = new BufferAttribute( new Float32Array( [ 7, 8, 9, 10, 11, 12 ] ), 2, true );
attr.name = 'attribute';
attr.setUsage( DynamicDrawUsage );
attr.updateRange.offset = 1;
attr.updateRange.count = 2;

geometry.setAttribute( 'attr', attr );

Expand Down