Skip to content

Conversation

@takahirox
Copy link
Collaborator

This PR updates morph handling of GLTFLoader.

In the current implementation, addMorphTargets() always set both morph position and morph normal even if just either one of them is defined in glTF. This PR lets addMorphTargets() not set unnecessary morph.

Plus, I moved material.morphTargets/Normals = true to loadMesh() because most of other property, like .skinning are set in that method.

}
// Copying the original position not to affect the final position.
// See the formula above.
positionAttribute = cloneBufferAttribute( geometry.attributes.position );
Copy link
Collaborator

@donmccurdy donmccurdy Apr 5, 2018

Choose a reason for hiding this comment

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

Trying to remember why this case is here... is the problem that because morphAttributes takes the form:

morphAttributes = {
  position: [ position1, position2 ],
  normal: [ normal1, normal2 ]
}

... then if any morph target includes normal, every other target must also do so to avoid throwing off the array indices? And if so, what would "affect the final position" so that we need to make a copy of the BufferAttribute?

Copy link
Collaborator Author

@takahirox takahirox Apr 8, 2018

Choose a reason for hiding this comment

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

Three.js morphPosition/Normal values are absolute. The formula is

finalPosition/Normal =
    basePosition/Normal
        + weight0 * ( morphPosition/Normal0 - basePosition/Normal )
        + weight1 * ( morphPosition/Normal1 - basePosition/Normal )
        ...

while the glTF ones are relative

finalPosition/Normal =
    basePosition/Normal
        + weight0 * glTFmorphPosition/Normal0
        + weight1 * glTFmorphPosition/Normal1
        ...

If morphPosition/Normal values in Three.js are same as the base ones, they don't affect finalPosition/Normal.

Imagine the following targets in glTF.

"targets": [
    {
        "NORMAL": 0
    },
    {
        "POSITION": 1,
        "NORMAL": 2
    }
]

targets[0] doesn't have "POSITION" so mesh.morphTargetInfluences[0] shouldn't affect finalPosition. Then I need to copy base position (geometry.attributes.position) to geometry.morphAttributes.position[0].

As you mentioned (and if I'm right), morphAttributes forms

morphAttributes = {
    position: [ position1, position2 ],
    normal: [ normal1, normal2 ]
}

morphAttributes.position[0] should exist even if targets[0] doesn't have "POSIITION".

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok yes, that all makes sense except one thing — do we need to clone the BufferAttribute if we're not going to modify it? Why not just reuse the existing BufferAttribute? In this example, I think it would be OK to have geometry.attributes.position === geometry.morphAttributes.position[0]. If there's a reason it must be cloned let's just add a comment.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I had just never thought and tried to assign the same array to two or more attributes in shader. I tried and it seems to work. So updated. Thanks.


// unnecessary to set morph if base attributes don't exist
if ( geometry.attributes.position === undefined ) hasMorphPosition = false;
if ( geometry.attributes.normal === undefined ) hasMorphNormal = false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

From the spec:

A Morph Target is a morphable Mesh where primitives' attributes are obtained by adding the original attributes to a weighted sum of targets attributes.

My reading of that would be, base attributes MUST be set for any attributes used with morph targets. I don't think we need to ignore morph targets gracefully in this case, it would be fine to have a runtime error and enforce the spec strictly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed with that. Updated, thanks.

BTW, even if base attribute isn't set but morph attribute is set, Three.js doesn't seem to show any errors/warnings. Anyways, users could be aware of something wrong in glTF because of unexpected rendering.

@mrdoob mrdoob added this to the r92 milestone Apr 12, 2018
@mrdoob mrdoob merged commit 1b6688b into mrdoob:dev Apr 12, 2018
@mrdoob
Copy link
Owner

mrdoob commented Apr 12, 2018

Thanks!

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 this pull request may close these issues.

3 participants