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

Implement extension KHR_animation_pointer #7

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

mikeskydev
Copy link

Implements KHR_animation_pointer, Closes atteneder#592.

2023-04-20_16-39-04.mp4

Main pointers:

  • meshes
    • weights
  • nodes
    • rotation
    • scale
    • translation
    • weights (using duplicated version of standard code. needs refactor.)
  • cameras
    • orthographic/xmag
    • orthographic/ymag
    • orthographic/zfar
    • orthographic/znear
    • perspective/yfov
    • perspective/zfar
    • perspective/znear
  • materials
    • pbrMetallicRoughness/baseColorFactor
    • pbrMetallicRoughness/metallicFactor
    • pbrMetallicRoughness/roughnessFactor
    • alphaCutoff
    • emissiveFactor
    • normalTexture/scale
    • occlusionTexture/strength

Camera animations have been implemented with a helper GameObject due to the required recalculations needed from the raw animation values.

Extensions

In my original PR, I had implemented KHR_texture_transform, however I will submit this as an additional PR when suitable as it necessitates moving some code from material initialisation in C#, to the shaders. I have done this for the shadergraph shaders, but not the built in one. To save changing all shadergraphs, I packed the previous vec2 rotation property with both the new scalar rotation and the bool flipY that's required for KTX textures.

I will also implement KHR_lights_punctual as a separate PR.

  • KHR_materials_transmission
    • transmissionFactor

This PR is sponsored by Envoke.

@unity-cla-assistant
Copy link

unity-cla-assistant commented Nov 6, 2023

CLA assistant check
All committers have signed the CLA.

@mikeskydev
Copy link
Author

KHR_animation_pointer is now at release candidate stage.

@mikeskydev
Copy link
Author

mikeskydev commented Mar 5, 2024

And merged! @atteneder if this is still something glTFast is interested in, should I look at refactoring the pointers system to be more generic and not specific to animation, now that Khronos plans to use them for the Object Model? https://github.com/KhronosGroup/glTF/blob/main/specification/2.0/ObjectModel.adoc

@mikeskydev
Copy link
Author

Animation pointer has now been ratified 🎉

@atteneder
Copy link
Collaborator

Thanks a lot for this PR and getting the work started.

I agree, we need a non-animation-specific Object Model implementation. That's become even clearer after seeing KHR_interactivity getting to the public review stage using it as well.

This involves quite some string operations (which is natural, given pointers are string). I'd like to carefully review minimizing memory garbage created before merging.

The package still supports 2020 LTS. You've used range indexes instead of Substring. Unfortunately those don't work in 2020 LTS, so we either have to postpone it to the next release (that drops 2020 LTS) or change it to use Substrings.

I noticed that the new AnimationData is used, even on non-animation-pointer glTFs. Again, I need to confirm that there are no (performance) downsides whatsoever.

I've started to implement some of that feedback. I'll give an update next week.

@mikeskydev
Copy link
Author

mikeskydev commented Jun 20, 2024

Thanks, appreciate the review.

Object Model implementation

Absolutely. is KHR_interactivity something you'd be looking to implement in the main package too?

minimizing memory garbage

Good point, it's not something I'm an expert at, hopefully it's not terrible.

Substrings

Happy to leave that up to you! My target projects were 2021 so I didn't notice this.

AnimationData is used, even on non-animation-pointer glTFs

Yep totally understand, my thought was there ideally shouldn't be two places that animation creation happens, so I chose to pipe "legacy" animation into a new general system.

@atteneder
Copy link
Collaborator

Object Model implementation

Absolutely. is KHR_interactivity something you'd be looking to implement in the main package too?

No immediate plan, but I'm keeping an eye on it.

minimizing memory garbage

Good point, it's not something I'm an expert at, hopefully it's not terrible.

Nah, I'm pretty nit-picky to be honest.

Substrings

Happy to leave that up to you! My target projects were 2021 so I didn't notice this.

On it.

AnimationData is used, even on non-animation-pointer glTFs

Yep totally understand, my thought was there ideally shouldn't be two places that animation creation happens, so I chose to pipe "legacy" animation into a new general system.

You might be right after all. I need to learn more about the tradeoffs first.

@atteneder
Copy link
Collaborator

Further observations/questions

If animation channel targets are used multiple times (e.g. a camera, light or material is used on multiple nodes):

  1. Curves get created redundantly (not great, not terrible; pointer for optimization)
  2. IIRC the material is a shared one anyways. Isn't it sufficient to animate the first occurrence? A potential problem then becomes if you have multiple scene instances using the same material. An animation on instance A should not affect the material of instance B. Needs more testing and likely fixing/optimization.
  3. You can animate both a mesh's weights and a node's weights.
  • Are curves for mesh weight animation properly duplicated/re-used for multiple nodes?
  • What happens if there's a collision/both? Is this even covered in the spec?

API challenge

The PR requires to add an extensions field to AnimationChannelTarget. Unfortunately that conflicts with a generic one that already exists for the Newtonsoft JSON parsing schema classes. The fix requires a change that strictly speaking is an API breaking change. Even though I hardly expect an actual problem for users, policy requires raising the major version for that.

This is obviously none of your fault, but we have to address this and find a solution before moving on.

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.

Extension KHR_animation_pointer
3 participants