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

PropertyBinding: Use .morphTargetDictionary, not attribute.name... #19362

Conversation

donmccurdy
Copy link
Collaborator

@donmccurdy donmccurdy commented May 15, 2020

… for morph target bindings.

Also removes the Geometry support, since support for Mesh.updateMorphTargets() was already dropped.

Fixes #19357.

@donmccurdy donmccurdy force-pushed the bug-propertybinding-without-morph-attribute-names branch 3 times, most recently from dfdb20a to 8b53f3c Compare May 15, 2020 03:52
@donmccurdy
Copy link
Collaborator Author

Well the morph target examples still work, but none of them actually hit this code. I guess they are targeted by index instead.

@Bug-Reaper do you have a way of testing if this fix also works for your issue, #19357?

@donmccurdy donmccurdy force-pushed the bug-propertybinding-without-morph-attribute-names branch from 8b53f3c to 27f3129 Compare May 15, 2020 03:56
@mrdoob mrdoob added this to the r117 milestone May 15, 2020
@Bug-Reaper
Copy link
Contributor

Bug-Reaper commented May 15, 2020

Tested and confirmed to work! Things are animating and the buffer attribute names are free to be whatever they want to be :)
morphAttributes

@donmccurdy donmccurdy marked this pull request as ready for review May 15, 2020 16:49
@donmccurdy
Copy link
Collaborator Author

The one remaining use of attribute.name for morph targets that I know of is, calling mesh.updateMorphTargets() will erase the morph target dictionary and overwrite it with attribute names. I think the only reason to use that method is if you're manually constructing a mesh with morph targets, so it shouldn't be an issue when loading a GLB, but might be worth revisiting in the future.

@Bug-Reaper
Copy link
Contributor

Bug-Reaper commented May 15, 2020

Yea, I agree it'd probably be helpful to have mesh.updateMorphTargets() have behavior consistent with the change we're making here to decouple attribute names from the morphTargetDictionary names. (@donmccurdy Can you link me to where that function is declared?)

This would probably be best accompanied with an example for manually constructing a mesh with morph targets.

I'm also curious about the differences between bufferGeometries and geometries here.

Docs seem to indicate that geometries use .morphTargets and .morphNormals.
https://threejs.org/docs/index.html#api/en/core/Geometry.morphTargets

While bufferGeometries use .morphAttributes and .morphTargetsRelative
https://threejs.org/docs/index.html#api/en/core/BufferGeometry.morphAttributes

Are there any implications in the differences there that might be affected by this update? I believe I've only tested this PR with bufferGeometries.

@donmccurdy
Copy link
Collaborator Author

Support for Geometry (as opposed to BufferGeometry) is a non-goal: we're trying to deprecate Geometry, and have already removed skinning support from Geometry. I would like to see .updateMorphTargets become independent of attribute.name, but I do not plan to make that change here. For the current implementation:

updateMorphTargets: function () {
var geometry = this.geometry;
var m, ml, name;
if ( geometry.isBufferGeometry ) {

@Bug-Reaper
Copy link
Contributor

Alright cool cool. Didn't know we were deprecating geometry, sounds spicy. I'll see what I can do about a separate PR for wrangling in the updateMorphTargets function. Though looking at it, it seems like we might want to adapt it to have more useful functionality. A full fledged .addMorphTarget() function might be a good route?

I'd probably want to go through the process of manually adding morphTargets to see what would be most useful. Thanks again @donmccurdy :)

@mrdoob mrdoob merged commit 5d6f198 into mrdoob:dev May 16, 2020
@mrdoob
Copy link
Owner

mrdoob commented May 16, 2020

Thanks!

@donmccurdy donmccurdy deleted the bug-propertybinding-without-morph-attribute-names branch May 16, 2020 01:07
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.

GLTFLoader geometry.morphAttributes normal/position .name not being set
3 participants