GLTFLoader: Support morph targets w/ interleaved attributes #12800

donmccurdy · 2017-12-04T21:07:53Z

Alternatively, I'd be glad to add this implementation as InterleavedBufferAttribute.prototype.clone(), which currently does not exist.

/cc @takahirox

mrdoob · 2017-12-05T08:29:46Z

Alternatively, I'd be glad to add this implementation as InterleavedBufferAttribute.prototype.clone(), which currently does not exist.

I think that'd be a better solution yeah.

donmccurdy · 2017-12-06T01:03:56Z

Ok, done. 👍

WestLangley · 2017-12-06T02:17:16Z

src/core/InterleavedBufferAttribute.js

+
+		}
+
+		return new BufferAttribute( array, itemSize, this.normalized );


I would expect an InterleavedBufferAttribute to be returned from the clone() method.

That seems more intuitive, yes, but I can't think of a reason you'd want data the attribute is interleaved with to also be copied... do we return a new InterleavedBufferAttribute with stride === itemSize? Or rename this as toBufferAttribute()?

Only guessing here, but maybe

BufferAttribute.fromInterleavedBufferAttribute()

is the method you need to create.

Do you mean a static method, or instance like geometry.fromBufferGeometry(bufferGeometry)? Feels non-ideal making an empty array just to instantiate an empty buffer attribute then overwrite it. What about .toNonInterleaved() similar to .toNonIndexed()?

I'm not sure if the problem you are trying to solve warrants a special method... How often do uses want to de-interleave their geometry?

How often do uses want to de-interleave their geometry?

Not sure, I only need this within GLTFLoader. But the idea that .clone() should exist for what most users will assume is a subclass of BufferAttribute seems nice on least-surprise principle. The original version of this PR put the method inside of GLTFLoader, and I changed that based on #12800 (comment).

I think your suggestion makes sense.

var bufferAttribute = interleavedBufferAttribute.toNonInterleaved(); var geometry = new THREE.BufferGeometry(); geometry.addAttribute( 'position', bufferAttribute );

I'm just not sure if such a method is needed outside your loader... I'll leave that up to you.

@mrdoob with the context above, any preference on moving this back into a private cloneBufferAttribute(attr) helper, vs renaming clone() as toNonInterleaved() here?

I don't expect users outside GLTFLoader to call .toNonInterleaved() directly, but we have multiple open issues about .clone() not working consistently, and this is one of those edge cases — having any interleaved buffer attributes will cause bufferGeometry.clone() to throw a TypeError.

Agreed. I think this is an accurate statement of the issues:

@donmccurdy wants to de-interleave his buffer geometry. I think that should be implemented via a private helper for now.

BufferGeometry.clone() does not support interleaved attributes. That should be fixed.

I've looked into this and created a little code snippet that should enable the support of interleaved attributes in BufferGeometry.clone(). The most important thing first: Because interleaved attributes of a geometry share a common instance of ArrayBuffer, it does not make sense to perform an isolated clone of a single interleaved attribute. A clone operation is only valid on geometry level in order to ensure continuous, interleaved memory. The following code performs this task. The assumption of the implementation is that all interleaved attributes of a geometry share the same array buffer (which might not be true in all cases).

// attributes var attributes = source.attributes; var arrayBuffer; for ( name in attributes ) { var attribute = attributes[ name ]; if ( attribute.isInterleavedBufferAttribute ) { if ( arrayBuffer === undefined ) { // clone the array buffer once and use it for subsequent typed arrays arrayBuffer = attribute.array.buffer.slice( 0 ); } // now perform the actual clone of the attribute and its internal data var typedArray = new attribute.array.constructor( arrayBuffer ); var interleavedBuffer = new attribute.data.constructor( typedArray, attribute.data.stride ); var interleavedBufferAttribute = new attribute.constructor( interleavedBuffer, attribute.itemSize, attribute.offset, attribute.normalized ); this.addAttribute( name, interleavedBufferAttribute ); } else { this.addAttribute( name, attribute.clone() ); } }

Besides, it's maybe a good idea to merge InterleavedBuffer with InterleavedBufferAttribute. Properties like dynamic and updateRange should be part of the attribute. Are there any reasons for a separation of InterleavedBuffer and InterleavedBufferAttribute?

donmccurdy · 2017-12-30T01:10:56Z

Ok, I've reverted the .clone() implementation on InterleavedBufferAttribute and just used a private helper method in GLTFLoader.

Besides, it's maybe a good idea to merge InterleavedBuffer with InterleavedBufferAttribute. Properties like dynamic and updateRange should be part of the attribute. Are there any reasons for a separation of InterleavedBuffer and InterleavedBufferAttribute?

The separation of InterleavedBuffer and InterleavedBufferAttribute is a bit confusing to me as well...

WestLangley · 2017-12-30T02:39:36Z

I think this PR needs a new title, now...

WestLangley · 2017-12-30T02:40:05Z

Besides, it's maybe a good idea to merge InterleavedBuffer with InterleavedBufferAttribute. Properties like dynamic and updateRange should be part of the attribute. Are there any reasons for a separation of InterleavedBuffer and InterleavedBufferAttribute?

/ping @benaadams for opinion

This reverts commit 8f78c13.

benaadams · 2017-12-30T02:56:05Z

Not sure how the buffer and attribute can be the same thing; else its not interleaved?

Multiple attributes share the same buffer when interleaved

So the attributes are next to each other; so the single (InterleavedBuffer) Float32Array that's laid out as follows:

position0, uv0, normal0; position1, uv1, normal1; position2, uv2, normal2; ... positionM, uvN, normalN

And you have 3 InterleavedBufferAttributes, position,uv,normal for the attributes vec3 position, vec2 uv, vec3 normal

donmccurdy · 2017-12-30T03:10:05Z

@benaadams for me the confusing bit is why InterleavedBuffer should not simply be a Float32Array, and have stride/dynamic/updateRange be handled within the InterleavedBufferAttribute class. Or if that's not possible, why the InterleavedBuffer can't be passed into a normal BufferAttribute instance. The API requirement of creating two unique classes for interleaved attributes isn't quite natural-feeling, but maybe there are reasons I've missed.

Mugen87 · 2017-12-30T14:59:41Z

for me the confusing bit is why InterleavedBuffer should not simply be a Float32Array,

Correct. The only difference of BufferAttribute and InterleavedBufferAttribute is the way typed arrays are created. In the case of BufferAttribute the internal arrays are independent of each other. When creating InterleavedBufferAttributes one could use this pattern.

var arrayBuffer = new ArrayBuffer( 72 ); // a generic buffer that holds 72 bytes of data

// share the buffer across multiple typed arrays (this makes the vertex information interleaved!)
var positions = new Float32Array( arrayBuffer );
var normals = new Float32Array( arrayBuffer );

// create attributes without InterleavedBuffer via new THREE.InterleavedBufferAttribute( array, itemSize, stride, offset, normalized );
var interleavedPositionAttribute = new THREE.InterleavedBufferAttribute( positions, 3, 6, 0, false );
var interleavedNormalAttribute = new THREE.InterleavedBufferAttribute( normals, 3, 6, 3, false );

// fill in some data with e.g. interleavedPositionAttribute.setXYZ();

We can't use BufferAttribute because we need to specify the stride and offset value for each interleaved attribute. In this case:

stride: A single unit of vertex information (position + normal vector): 6 floats (24 bytes)
offset: The offset inside this unit of vertex information that marks the start of each attribute. In the case of the normal attribute: 3 floats (12 bytes)

So i think we do not necessarily need InterleavedBuffer. InterleavedBufferAttribute could hold array and stride as new properties and also the other stuff of InterleavedBuffer (things like dynamic or updateRange).

WestLangley · 2017-12-30T17:15:06Z

Do you understand that interleaved attribute data can contain a mixture of types? For example:

var geometry = new THREE.BufferGeometry();

var stride32 = 7; // x/y/z + nx/ny/nz + color = 7 4-byte floats
var stride8 = 28; // x/y/z + nx/ny/nz + color = 28 1-byte uints

var float32Array = new Float32Array( numTriangles * 3 * stride32 );
var uint8Array = new Uint8Array( float32Array.buffer ); // same buffer, interpreted differently

// populate data ...

var ib32 = new THREE.InterleavedBuffer( float32Array, stride32 );
var ib8 = new THREE.InterleavedBuffer( uint8Array, stride8 );

geometry.addAttribute( 'position', new THREE.InterleavedBufferAttribute( ib32, 3, 0, false ) );
geometry.addAttribute( 'normal', new THREE.InterleavedBufferAttribute( ib32, 3, 3, false ) );
geometry.addAttribute( 'color', new THREE.InterleavedBufferAttribute( ib8, 3, 24, true ) );

donmccurdy · 2017-12-30T18:30:43Z

Sure, but conceptually I think of the typed arrays as having a shared interleaved (array)buffer, not being encapsulated by an interleaved buffer. So where we have this structure:

InterleavedBufferAttribute > InterleavedBuffer > TypedArray > ArrayBuffer

Why not have this:

InterleavedBufferAttribute > TypedArray > ArrayBuffer

This matches a normal BufferAttribute more closely, and the example code becomes:

var geometry = new THREE.BufferGeometry();

var stride32 = 7; // x/y/z + nx/ny/nz + color = 7 4-byte floats
var stride8 = 28; // x/y/z + nx/ny/nz + color = 28 1-byte uints

var float32Array = new Float32Array( numTriangles * 3 * stride32 );
var uint8Array = new Uint8Array( float32Array.buffer ); // same buffer, interpreted differently

// populate data ...

geometry.addAttribute( 'position', new THREE.InterleavedBufferAttribute( float32Array, stride32, 3, 0, false ) );
geometry.addAttribute( 'normal', new THREE.InterleavedBufferAttribute( float32Array, stride32, 3, 3, false ) );
geometry.addAttribute( 'color', new THREE.InterleavedBufferAttribute( uint8Array, stride8, 3, 24, true ) );

The constructor arguments are different from BufferAttribute of course, but that's already the case today.

Mugen87 · 2017-12-30T19:12:33Z

If we would remove InterleavedBuffer, i think we have to ensure that the renderer creates only a single buffer via gl.createBuffer() for all InterleavedBufferAttributes that previously belonged to an InterleavedBuffer. This will cause some refactoring, especially in WebGLAttributes.

Mugen87 · 2017-12-30T19:36:25Z

When i think about it, it should be also possible to have a single buffer created via gl.createBuffer() for all interleaved attributes of a geometry. Maybe we should try this out 😁

benaadams · 2017-12-30T20:13:57Z

Another scenario; its not just distinct attributes that are interleaved; it can be the same attribute.

var geometry = new THREE.BufferGeometry();

var stride = 3; // x/y/z 4-byte floats
var float32Array = new Float32Array( (numPoints + 4) * 3);

// populate position data ...

var ib = new THREE.InterleavedBuffer( float32Array, stride );

// Same position data, different offsets
geometry.addAttribute( 'last', new THREE.InterleavedBufferAttribute( ib, 3, 0, false ) );
geometry.addAttribute( 'current', new THREE.InterleavedBufferAttribute( ib, 3, 2 * stride, false ) );
geometry.addAttribute( 'next', new THREE.InterleavedBufferAttribute( ib, 3, 4 * stride, false ) );

var mesh = new THREE.Mesh( geometry, material );
mesh.drawMode = THREE.TriangleStripDrawMode;

You want to call gl.createBuffer() a single time for all attributes; but also gl.buffer{Sub}Data(...) a single time for all attributes.

Removing InterleavedBuffer will mean its extra info; which is still needed, will need to be pushed on to the next shared location; mutating the ArrayBuffer instance and adding extra properties to it like buffer etc; and mutating the primitive types with extra properties isn't great (and can't be detected with instanceof).

As for items like updateRange; that could be a pass through item; so its available on the InterleavedBufferAttribute; but when you change it it updates the InterleavedBuffer multiplying by stride and adding offset etc

Or you'd need an aside dict of buffers types with dirty flags keyed on the attribute; but the extra lookup wouldn't be good for perf.

Mugen87 · 2017-12-31T10:28:34Z

Um, I see what you mean. InterleavedBuffer definitely has a purpose 😇.

@benaadams Can you make a suggestions on how to support interleaved attributes in BufferGeometry .clone()? I've posted some code here but it does not handle InterleavedBuffer correctly. It seems that the detection of shared resources (things like InterleavedBuffer or ArrayBuffer) and the respective clone is not trivial to implement.

donmccurdy · 2018-01-07T01:48:04Z

Removing InterleavedBuffer will mean its extra info; which is still needed, will need to be pushed on to the next shared location; mutating the ArrayBuffer instance and adding extra properties to it like buffer etc...

Ok, yes this makes sense and I agree that mutating the primitive array should be avoided.

If the InterleavedBuffer's UUID is used to avoid calling createBuffer() multiple times, is the example @WestLangley posted earlier (#12800 (comment)), using two InterleavedBuffer instances around the same ArrayBuffer instance, correct?

var geometry = new THREE.BufferGeometry();

var stride32 = 7; // x/y/z + nx/ny/nz + color = 7 4-byte floats
var stride8 = 28; // x/y/z + nx/ny/nz + color = 28 1-byte uints

var float32Array = new Float32Array( numTriangles * 3 * stride32 );
var uint8Array = new Uint8Array( float32Array.buffer ); // same buffer, interpreted differently

// populate data ...

var ib32 = new THREE.InterleavedBuffer( float32Array, stride32 );
var ib8 = new THREE.InterleavedBuffer( uint8Array, stride8 );

geometry.addAttribute( 'position', new THREE.InterleavedBufferAttribute( ib32, 3, 0, false ) );
geometry.addAttribute( 'normal', new THREE.InterleavedBufferAttribute( ib32, 3, 3, false ) );
geometry.addAttribute( 'color', new THREE.InterleavedBufferAttribute( ib8, 3, 24, true ) );

Or does this result in multiple createBuffer calls?

donmccurdy · 2018-01-07T01:50:05Z

Aside, I think the PR itself is OK to merge at this point and orthogonal to discussion above.

mrdoob · 2018-01-08T23:32:23Z

Thanks!

Mugen87 · 2018-12-11T11:54:11Z

examples/js/loaders/GLTFLoader.js

+				array[ i ] = attribute.getX( i );
+				if ( itemSize >= 2 ) array[ i + 1 ] = attribute.getY( i );
+				if ( itemSize >= 3 ) array[ i + 2 ] = attribute.getZ( i );
+				if ( itemSize >= 4 ) array[ i + 3 ] = attribute.getW( i );


@donmccurdy Um, I'm not sure this section is correct. If you use i in order to refer to the current array position, you are going to overwrite values after array[ i ] = attribute.getX( i ); since you raise the variable in the for loop. I think it should look like this:

var j = 0; for ( var i = 0; i < count; ++ i ) { array[ j ++ ] = attribute.getX( i ); if ( itemSize >= 2 ) array[ j ++ ] = attribute.getY( i ); if ( itemSize >= 3 ) array[ j ++ ] = attribute.getZ( i ); if ( itemSize >= 4 ) array[ j ++ ] = attribute.getW( i ); }

Oops, yes. There's a sample file that uses interleaved morph targets in donmccurdy/three-gltf-viewer#56, but it looks like I never got an answer back from the OP about whether the result was what they expected.

Do you think we should fix this directly, or change something in a base class per #15395?

I would change it directly. I've actually added this code section in #15413. I'm not sure how much effort it takes not enable copy/clone and serialization/deserialization of interleaved buffer attributes. For now, I vote to create "normal" buffer attributes if we have to clone or serialization them.

donmccurdy changed the title ~~GLTFLoader: Support morph targets with interleaved accessors.~~ InterleavedBufferAttribute: Implement .clone() Dec 6, 2017

WestLangley reviewed Dec 6, 2017

View reviewed changes

donmccurdy changed the title ~~InterleavedBufferAttribute: Implement .clone()~~ GLTFLoader: Support morph targets w/ interleaved attributes Dec 30, 2017

donmccurdy and others added 3 commits December 29, 2017 21:54

GLTFLoader: Support interleaved morph target accessors.

db7aa46

InterleavedBufferAttribute: Implement .clone().

1733ddb

Revert "InterleavedBufferAttribute: Implement .clone()."

81f9082

This reverts commit 8f78c13.

donmccurdy force-pushed the feat-gltfloader-interleaved-morphtargets branch from fc96e29 to 81f9082 Compare December 30, 2017 02:54

mrdoob merged commit ecd4515 into mrdoob:dev Jan 8, 2018

mrdoob added this to the r90 milestone Jan 17, 2018

donmccurdy mentioned this pull request Jan 18, 2018

Error message when importing file with morph targets donmccurdy/three-gltf-viewer#56

Closed

Mugen87 mentioned this pull request Dec 8, 2018

InterleavedBufferAttribute has no clone method #15395

Closed

13 tasks

Mugen87 reviewed Dec 11, 2018

View reviewed changes

donmccurdy deleted the feat-gltfloader-interleaved-morphtargets branch December 11, 2018 15:23

donmccurdy mentioned this pull request Dec 16, 2018

GLTFLoader: Fix bug with certain interleaved attributes. #15438

Merged

Mugen87 mentioned this pull request Jun 19, 2019

InterleavedBufferAttribute misses toJSON method #16835

Closed

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GLTFLoader: Support morph targets w/ interleaved attributes #12800

GLTFLoader: Support morph targets w/ interleaved attributes #12800

donmccurdy commented Dec 4, 2017

mrdoob commented Dec 5, 2017

donmccurdy commented Dec 6, 2017

WestLangley Dec 6, 2017

donmccurdy Dec 6, 2017

WestLangley Dec 6, 2017 •

edited

Loading

donmccurdy Dec 21, 2017

WestLangley Dec 21, 2017

donmccurdy Dec 22, 2017

WestLangley Dec 22, 2017

donmccurdy Dec 23, 2017

WestLangley Dec 23, 2017

Mugen87 Dec 29, 2017 •

edited

Loading

donmccurdy commented Dec 30, 2017

WestLangley commented Dec 30, 2017

WestLangley commented Dec 30, 2017

benaadams commented Dec 30, 2017

donmccurdy commented Dec 30, 2017 •

edited

Loading

Mugen87 commented Dec 30, 2017 •

edited

Loading

WestLangley commented Dec 30, 2017

donmccurdy commented Dec 30, 2017

Mugen87 commented Dec 30, 2017 •

edited

Loading

Mugen87 commented Dec 30, 2017 •

edited

Loading

benaadams commented Dec 30, 2017 •

edited

Loading

Mugen87 commented Dec 31, 2017 •

edited

Loading

donmccurdy commented Jan 7, 2018

donmccurdy commented Jan 7, 2018

mrdoob commented Jan 8, 2018

Mugen87 Dec 11, 2018 •

edited

Loading

donmccurdy Dec 11, 2018

Mugen87 Dec 11, 2018 •

edited

Loading


		}

		return new BufferAttribute( array, itemSize, this.normalized );

GLTFLoader: Support morph targets w/ interleaved attributes #12800

GLTFLoader: Support morph targets w/ interleaved attributes #12800

Conversation

donmccurdy commented Dec 4, 2017

mrdoob commented Dec 5, 2017

donmccurdy commented Dec 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WestLangley Dec 6, 2017 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mugen87 Dec 29, 2017 • edited Loading

Choose a reason for hiding this comment

donmccurdy commented Dec 30, 2017

WestLangley commented Dec 30, 2017

WestLangley commented Dec 30, 2017

benaadams commented Dec 30, 2017

donmccurdy commented Dec 30, 2017 • edited Loading

Mugen87 commented Dec 30, 2017 • edited Loading

WestLangley commented Dec 30, 2017

donmccurdy commented Dec 30, 2017

Mugen87 commented Dec 30, 2017 • edited Loading

Mugen87 commented Dec 30, 2017 • edited Loading

benaadams commented Dec 30, 2017 • edited Loading

Mugen87 commented Dec 31, 2017 • edited Loading

donmccurdy commented Jan 7, 2018

donmccurdy commented Jan 7, 2018

mrdoob commented Jan 8, 2018

Mugen87 Dec 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mugen87 Dec 11, 2018 • edited Loading

Choose a reason for hiding this comment

WestLangley Dec 6, 2017 •

edited

Loading

Mugen87 Dec 29, 2017 •

edited

Loading

donmccurdy commented Dec 30, 2017 •

edited

Loading

Mugen87 commented Dec 30, 2017 •

edited

Loading

Mugen87 commented Dec 30, 2017 •

edited

Loading

Mugen87 commented Dec 30, 2017 •

edited

Loading

benaadams commented Dec 30, 2017 •

edited

Loading

Mugen87 commented Dec 31, 2017 •

edited

Loading

Mugen87 Dec 11, 2018 •

edited

Loading

Mugen87 Dec 11, 2018 •

edited

Loading