Incorrect attribute length after calling `BufferGeometry.computeVertexNormals` with changed positions attribute

[SebastianStamm]

Description

BufferGeometry.computeVertexNormals checks if the normal attribute exists and initializes it with the size of the position attribute. If it already exists, it is reused and reset.

three.js/src/core/BufferGeometry.js

Lines 994 to 1011 in d3b629c

	let normalAttribute = this.getAttribute( 'normal' );
	if ( normalAttribute === undefined ) {
	normalAttribute = new BufferAttribute( new Float32Array( positionAttribute.count * 3 ), 3 );
	this.setAttribute( 'normal', normalAttribute );
	} else {
	// reset existing normals to zero
	for ( let i = 0, il = normalAttribute.count; i < il; i ++ ) {
	normalAttribute.setXYZ( i, 0, 0, 0 );
	}
	}

If you call computeVertexNormals, then change the size of the position attribute (e.g. to reflect changes to the model), and call computeVertexNormals again, the attribute retains its original size, either missing entries (if the position attribute has more entries), or having too many values (if the position attribute is now smaller).

I would expect that after calling computeVertexNormals, the normal attribute has the same size as the position attribute.

Workaround

Check if the position attribute has a different count than the normal attribute and delete the normal attribute before calling computeVertexNormals

Other affected methods

I assume BufferGeometry.computeTangents is also affected.

Reproduction steps

1. Create a Buffer Geometry and set the positions attribute
2. Call computeVertexNormals
3. Overwrite the positions attribute with a new BufferAttribute of a different size
4. Call computeVertexNormals again

Expected:
The normal attribute has the same size as the new position attribute.

Observed:
The normal attribute has the size of the old position attribute.

Code

import * as THREE from 'three';

const geometry = new THREE.BufferGeometry();

// create a simple triangle (3 vertices)
geometry.setAttribute( 'position', new THREE.BufferAttribute( new Float32Array( [
	-1.0, -1.0,  1.0, // v0
	 1.0, -1.0,  1.0, // v1
	 1.0,  1.0,  1.0, // v2
] ), 3 ) );

// Creates the `normal` attribute
geometry.computeVertexNormals();

let normalAttr = geometry.getAttribute( 'normal' );
console.log(normalAttr.count); // correctly displays count of 3

// overwrite the position attribute with new values (a square, 6 vertices)
geometry.setAttribute( 'position', new THREE.BufferAttribute( new Float32Array( [
	-1.0, -1.0,  1.0, // v0
	 1.0, -1.0,  1.0, // v1
	 1.0,  1.0,  1.0, // v2
	 1.0,  1.0,  1.0, // v3
	-1.0,  1.0,  1.0, // v4
	-1.0, -1.0,  1.0  // v5
] ), 3 ) );

// Reuses the `normal` attribute created in the first call
geometry.computeVertexNormals();

// should be the same, but just to be safe, retrieve the current normal attribute again
normalAttr = geometry.getAttribute( 'normal' );

console.log(normalAttr.count); // !!! still 3 - but should be 6 !!!

Live example

https://stackblitz.com/edit/typescript-1ofnthap?file=index.ts

Screenshots

No response

Version

0.184.0

Device

Desktop

Browser

Chrome

OS

Windows

[Mugen87]

Um, this is some sort of edge case I am not sure we need to support. In general, geometries are fixed sized after the first render. So the only valid use case is to change the length of a position attribute before the first render which feels quite unlikely to me. If it would be common, this issue would have raised up way earlier in three's history.

Would it be sufficient to just add a note to the docs that this method does not support resized position attributes?

[SebastianStamm]

Maybe I am doing it wrong, but I am able to update the attributes of a BufferGeometry with differently sized attributes after the first render. Of course you cannot resize the existing attributes, but I am able to attach new BufferAttributes with the required size to an existing BufferGeometry, even after it was rendered.

Here is a sandbox where a BufferGeometry with 3 vertices is created and rendered, then the positions attribute of the geometry is changed and rendered again: https://stackblitz.com/edit/typescript-w2hw6mcw?file=index.ts

Of course replacing attributes is kinda slow, but probably still faster than recreating the whole geometry, especially for more complex geometries with many attributes when you can reuse some of them.

If replacing or changing the size of attributes is some kind of anti-pattern, I would rather have a note in the docs for the BufferGeometry in general, rather than just for this method.

[Mugen87]

Right, I forgot attribute replacement is supported. The problem is the attribute replacement kind of "hides" the overhead since the VAO must still be dumped and a new one created from scratch. Besides, the replaced and now unused buffer attribute hangs in memory so you need to wait for the GC to clean things up.

It's always best to keep your geometries static. If you need more space in attributes, allocate them with larger size right from the beginning and use the draw range to decide what should be rendered.

But I agree it's better if BufferGeometry supports your use case more gracefully. I'll file a PR.

[WestLangley]

Right, I forgot attribute replacement is supported

When did that take effect? And after the first render?

[Mugen87]

I don't know anymore but I believe WebGLBindingsState was implemented with this use case in mind right from the beginning. It's this check:

three.js/src/renderers/webgl/WebGLBindingStates.js

Line 176 in 831b0c1

if ( cachedAttribute.attribute !== geometryAttribute ) return true;

In the meanwhile, users rely on this behavior and we had to support it in WebGPURenderer as well.

[WestLangley]

OK. So no dispose is required at the app level?

[Mugen87]

No. One reason is BufferAttribute.dispose() does not exist. It's an open issue how to implement attribute-level disposal (especially the VAO invalidation bit) but sooner or later we need it in WebGPURenderer for storage attributes, see #33468.