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.

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

GLTFLoader: Morph targets only work for matching accessor types #17608

Closed
zeux opened this issue Sep 29, 2019 · 10 comments · Fixed by #17649
Closed

GLTFLoader: Morph targets only work for matching accessor types #17608

zeux opened this issue Sep 29, 2019 · 10 comments · Fixed by #17649
Assignees

Comments

@zeux
Copy link
Contributor

zeux commented Sep 29, 2019

glTF files assume that morph target data is specified as relative adjustments, and three.js assumes that the data in morph target buffers is absolute.

This leads to a few obvious memory/performance caveats - the morph target data needs to be duplicated and modified during import - but also leads to inability to use a very narrow type for position deltas.

For example, when data is quantized with gltfpack (which makes the glTF files technically invalid right now but this will change soon: KhronosGroup/glTF#1673), the positions typically use UInt16 and morph target deltas use Int16. However, very frequently the morph target delta range is small enough to fit in Int8 - because three.js attempts to reconstruct the absolute position however, attempting to encode Int8 deltas leads to decoding issues on three.js side. Babylon.JS handles files like this perfectly.

I'd like to fix this - the obvious tactical fix is changing the morph target attribute cloning to assume the type of the base attribute, not morph target. However, I'd like to check - what are the thoughts of fixing this by adding support to three.js for relative morph target data?

In theory, the formulas are compatible to not require any shader code duplication - specifically, this equation that three.js currently uses:

basePosition
+ weight0 * ( morphPosition0 - basePosition )
+ weight1 * ( morphPosition1 - basePosition )

and this equation that glTF assumes:

basePosition
+ weight0 * glTFmorphPosition0
+ weight1 * glTFmorphPosition1

can be reformulated as:

weightBase * basePosition
+ weight0 * morphPosition0
+ weight1 * morphPosition1

After which for glTF style files the shader needs [1, weight0, weight1, ...] as an input, whereas normally you'd pass [1 - weight0 - weight1 - ..., weight0, weight1, ...] as an input. So by adding some sort of flag to three.js mesh (e.g. morphTargetsAreDeltas) and a bit of JS code to compute the weights correctly, we could support both with minimal cost.

@zeux
Copy link
Contributor Author

zeux commented Sep 29, 2019

Here's an example mesh with this problem that works perfectly fine in Babylon.JS but doesn't work in three.js.

morphbytedeltas.glb.zip

@donmccurdy
Copy link
Collaborator

because three.js attempts to reconstruct the absolute position however, attempting to encode Int8 deltas leads to decoding issues on three.js side...

What decoding issues do you see? At a glance it looks like we'll have an issue with normalized target positions, but I don't see why the accessor types would need to match to decode correctly.

For the other parts:

Quantized Attributes: I'm not sure if we've discussed this feature before here. I think it would be implemented for "position" and "normal" attributes using NodeMaterial, but I wouldn't know how to implement it for other attributes without changes to three.js core, like a quantized option for BufferAttribute. Seems worth looking into after the proposed extension progresses further.

Relative Morph Targets: Yeah, this is the major case where THREE.GLTFLoader has to actually process an attribute accessor. It'd be nice to eliminate that parsing time. Note that morph normals are also relative in glTF, and morph tangents are not currently implemented in three.js. A couple ideas for the API:

// o = Material, BufferGeometry, or Mesh?
o.morphTargetsRelative = true;
o.morphTargetsMode = THREE.MorphTargetsRelative;

^ Currently the Material, BufferGeometry, and Mesh types all have morphtarget-related properties, so I have no strong preference on which would get the property, but Mesh would be my vote. I'm assuming the setting would affect all morph target attributes.

@mrdoob what do you think about adding a property for this? It is convenient that the GLSL can be the same for both modes. @looeee it looks like this would apply to FBXLoader as well?

@zeux
Copy link
Contributor Author

zeux commented Sep 30, 2019

I don't see why the accessor types would need to match to decode correctly.

If base position attribute is Int16 and morph position attribute is Int8, the code will clone Int8 attribute and attempt to synthesize the absolute position by adding Int16 and Int8 values and storing the result in Int8, which will result in the value getting truncated to the shorter range. The code assumes that the type for the morph target delta is adequate to store the absolute value, which isn't true (it's a fair assumption for the current glTF spec that only allows FLOAT values though).

quantized option for BufferAttribute

This isn't necessary - three.js already supports everything that is needed for this extension wrt GPU rendering. The extension is structured such that the data is compatible with WebGL style vertex attribute setup (which three.js already performs correctly).

The only issues that come to mind are that BufferAttribute.normalized setting is ignored by some JS code that accesses the buffer values - this is because getX/Y/Z/W accessors ignore the .normalized attribute as well. This can probably be fixed case by case.

@zeux
Copy link
Contributor Author

zeux commented Sep 30, 2019

// o = Material, BufferGeometry, or Mesh?

In terms of interface and implementation, it seems easiest and probably cleaner to put this setting on the Mesh - that's where the morph target influences live atm. If there's consensus on this path being interesting to pursue I'd be happy to draft a change for this, it seems pretty simple to do.

edit although I can see the attraction of putting this onto BufferGeometry (that way the geometry data is self-contained) as well. I don't think Material is a good route unless we need to change the shader code, which I'm hoping to be able to avoid completely.

@donmccurdy
Copy link
Collaborator

the code will clone Int8 attribute and attempt to synthesize the absolute position by adding Int16 and Int8 values and storing the result in Int8...

Ah, got it. 😕

three.js already supports everything that is needed for this extension wrt GPU rendering

Oh, nice – I'd missed that your proposal used existing mechanisms to simplify dequantization.

Curious to hear others' thoughts on adding a relative morph target mode. If necessary I can do some performance profiling to see how much processing it saves. Note that the bug itself can be fixed without a relative mode — that would purely be to improve parsing performance of loaders with relative morph targets.

@zeux
Copy link
Contributor Author

zeux commented Sep 30, 2019

Note that the bug itself can be fixed without a relative mode — that would purely be to improve parsing performance of loaders with relative morph targets.

Yeah - definitely. This is why I mentioned the tactical fix as a possibility. However, with both GLTF and FBX storing deltas and having to reconstruct absolute positions at load time it seems like the core change may be more appropriate. This will also lead to being able to - eventually, conditional on something close to #17089 getting done at some point - directly load the buffer data for geometry buffers into large ArrayBuffers with no need for extra processing which is nice.

@zeux
Copy link
Contributor Author

zeux commented Sep 30, 2019

Just to give a basic idea, here's how first-class relative morph target support may look like: dev...zeux:morph-relative

@looeee
Copy link
Collaborator

looeee commented Oct 3, 2019

@looeee it looks like this would apply to FBXLoader as well?

Yes, I think blend shapes are always relative in FBX format.

However the blend shapes array is sparse, zero elements are omitted and there's an additional index to connect them to vertices, so there will still be a certain amount of processing to be done. I'm not sure without testing how much speed or simplicity we'd gain from this, but certainly at least some.

EDIT: Goofed this comment, the blend shapes array is sparse.

@takahirox
Copy link
Collaborator

takahirox commented Oct 3, 2019

Maybe we don't need to decide now but out of curiosity, if we start to support relative morph will we keep supporting both absolute and relative morph? Or will we deprecate the absolute one at some point?

Of course we don't want to break the compatibility, but the only one type support may sound simpler in core and to users. And the relative one may have advantage, data type can be shorter and major formats glTF and FBX use the relative one. Do other major 3D engines support the both types?

@zeux
Copy link
Contributor Author

zeux commented Oct 4, 2019

@takahirox Yeah I think this can be decided separately. As far as I know, all production uses of blend shapes use relative morphs for one reason or another; however, three.js currently uses morph targets to implement keyframe animations (Quake 3 style; I think MD2 loader does this?), so I'm sure there are some people who would like to keep the absolute morphs.

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 a pull request may close this issue.

4 participants