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

Import: import UV scrolling animations using EXT_property_animation #776

Closed
wants to merge 1 commit into from
Closed

Import: import UV scrolling animations using EXT_property_animation #776

wants to merge 1 commit into from

Conversation

scurest
Copy link
Contributor

@scurest scurest commented Nov 14, 2019

This implements importing animations that scroll UVs by animating a KHR_texture_transform/offset property with the EXT_property_animaton extension.

out

UVScrollExample.zip

Only animations that ONLY target the offset property are handled; if the rotation or scale is also animated, it gets ignored. The reason is that the curves will need to be resampled at a higher rate to handle the change in UV space between glTF and Blender (for example, a constant offset and simple linear ramp for rotation in glTF would require a sinusoidal offset in Blender). Even if the more general case were implemented, it's still desirable to have a separate codepath for handling only-offset animations because you can do it without resampling.

Also UV scrolling animations are much more common than animating the scale or rotation.

Outline of the implementation

  1. call BlenderMaterialAnim.anim for each animation in the file
  2. the data in EXT_property_animation gets rearranged so we can lookup a sampler from what it targets
  3. one action is created per glTF material as necessary (if there are two Blender materials for a glTF material (to handle vertex color) they will use the same action)
  4. any KHR_texture_transform that has only its offset property animated gets processed
  5. the KHR_texture_transform object needs to be looked up because we need the constant rotation and scale values to do the glTF<->Blender UV change
  6. the Mapping node is targeted by its name (eg. baseColorTexture.mapping) which is set in the make_texture_block function from Import: Factor texture node creation out into its own function #696
  7. at the end, the first animation in the glTF file is restored from the NLA tracks, just like for regular animations

Uses EXT_property_animation to import animations that control a
KHR_texture_transform/offset. If the rotation or scale is also
animated, it will be ignored -- doing this requires resampling
the animation curves which is more complicated.
@julienduroure
Copy link
Collaborator

Not sure about the status of this extension.
Is it this: KhronosGroup/glTF#1301 ?

Not sure if we have to implement non official extension. @donmccurdy ?
Maybe #732 can be used for importing too?

@scurest
Copy link
Contributor Author

scurest commented Nov 16, 2019

Yes, that. KhronosGroup/glTF#1301 (comment) indicated an interest in implementing it here.

This case is unintrusive and could probably be done in a importer user-extension with the right hooks, but animating other properties can't be so easily split from the importer. For material stuff, whether a property is animated needs to inform material creation (ex. even if a material doesn't have a baseColorFactor, if it's animated the node for it needs to be created) and then material creation needs to inform action creation (ex. you need to know where in the node tree baseColorFactor was inserted so you can target it for animation; it might have ended up directly in the PBR node, in an emission node (for unlit materials), in a multiply node (if there was also a baseColorTexture), etc). Also there are already places in the importer where it needs to know if something is animated (for Y-Up removal, for meshes with morph targets (#290)). So it'd be difficult to split it off like that.

@donmccurdy
Copy link
Contributor

Not sure if we have to implement non official extension. @donmccurdy ?

We certainly don't have to, but we can decide. My preference would be:

  • Try to support all KHR_ extensions unless there's some reason not to.
  • Decide on a case-by-case basis whether to support EXT_ extensions.
  • Do not implement vendor extensions (all other prefixes), leave to User defined extensions #732.

My only concern about this PR is that KhronosGroup/glTF#1301 is still a draft, and most working group effort is on other extensions right now. The specification could change if/when 1301 is picked back up again, although certainly having implementations (even unmerged) helps us to make progress. So I think it's better to (a) keep this PR open for a while, (b) implement via #732, or (c) put the setting behind some kind of "experimental" option. My vote is probably (a).

@julienduroure julienduroure added Animation importer This involves or affects the import process labels Dec 6, 2019
@scurest scurest closed this Dec 27, 2019
@donmccurdy donmccurdy mentioned this pull request Feb 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Animation importer This involves or affects the import process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants