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: Allow map as target object. #24537

Merged
merged 1 commit into from
Aug 25, 2022
Merged

Conversation

Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented Aug 24, 2022

Related issue: -

Description

It should be possible to animate the texture properties offset or rotation like in this fiddle: https://jsfiddle.net/37mfsugc/2/

However, since map is no part of the supported object names in PropertyBinding, it does not work if the animation is played in context of a 3D object.

The workaround would be to change the root object of the animation mixer to the texture which is problematic since animations are usually registered for 3D objects.

This PR has no effect on existing animations.

@donmccurdy
Copy link
Collaborator

In the JSFiddle the keyframe track name is material.map.offset, though looking at the code here I think I would have expected it to be something more like map.offset or .map.offset, is that intended? All else being equal I think <node-name>.material.map.offset would be ideal, but I understand that's one level deeper than we currently parse and might present a problem.

+1 for supporting textures as property binding targets here, thanks!

/cc @hybridherbst relevant to #24108

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Aug 24, 2022

In the JSFiddle the keyframe track name is material.map.offset, though looking at the code here I think I would have expected it to be something more like map.offset or .map.offset, is that intended?

Yes, the keyframe track name has to start relative from the 3D object.

@Mugen87 Mugen87 added this to the r144 milestone Aug 24, 2022
@Mugen87 Mugen87 merged commit 946c493 into mrdoob:dev Aug 25, 2022
@mrdoob
Copy link
Owner

mrdoob commented Aug 25, 2022

Thanks!

@arpu
Copy link

arpu commented Aug 26, 2022

@Mugen87 would be nice to have this for normalMap too?

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Aug 30, 2022

I'm trying to think of a solution that scales well for more map types.

@hybridherbst
Copy link
Contributor

hybridherbst commented Aug 30, 2022

@Mugen87 in case you want to take a look at all that would be needed for KHR_animation_pointer (effectively allowing all glTF properties to be animated), here's what I had to change for that:
https://github.com/mrdoob/three.js/pull/24108/files#diff-868e3d9b3219d163d7d53939fc3f7a6e65b7664ad5e3da03cd31fa23ea1a6df6R829-R917

(not saying that's a great implementation, but it covers all required cases as far as I'm aware)

hybridherbst added a commit to needle-tools/three.js that referenced this pull request Sep 6, 2022
hybridherbst added a commit to needle-tools/three.js that referenced this pull request Sep 7, 2022
hybridherbst added a commit to needle-tools/three.js that referenced this pull request Sep 7, 2022
abernier pushed a commit to abernier/three.js that referenced this pull request Sep 16, 2022
abernier pushed a commit to abernier/three.js that referenced this pull request Sep 16, 2022
snagy pushed a commit to snagy/three.js-1 that referenced this pull request Sep 21, 2022
snagy pushed a commit to snagy/three.js-1 that referenced this pull request Sep 21, 2022
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.

6 participants