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 #596

Closed
wants to merge 9 commits into from
Closed

Implement extension KHR_animation_pointer #596

wants to merge 9 commits into from

Conversation

mikeskydev
Copy link

@mikeskydev mikeskydev commented Apr 14, 2023

Implements KHR_animation_pointer, Closes #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

If preferred I can break out extension animations into their own PRs for greater focus. For example, KHR_texture_transform 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.

  • KHR_texture_transform
    • baseColorTexture/scale
    • baseColorTexture/offset
    • baseColorTexture/rotation
    • normalTexture/scale
    • normalTexture/offset
    • normalTexture/rotation
    • other texture targets?
  • KHR_lights_punctual
  • KHR_materials_transmission
    • transmissionFactor

This PR is sponsored by Envoke.

commit 78c674f
Author: Mike Nisbet <[email protected]>
Date:   Thu Apr 13 23:31:59 2023 +0100

    Enumerate lookups behind a check

commit 1a78ee4
Author: Mike Nisbet <[email protected]>
Date:   Thu Apr 13 19:31:15 2023 +0100

    Pointer utility class

commit 07c8da2
Author: Mike Nisbet <[email protected]>
Date:   Thu Apr 13 12:11:24 2023 +0100

    KHR_animation_pointer materials

    commit 05d4fd4ce661dff670de209b7f8fe00e6d03369d
    Author: Mike Nisbet <[email protected]>
    Date:   Thu Apr 13 12:09:56 2023 +0100

        More refactoring, materials 'complete' per spec

    commit a22bb9aa572a025d2f237b38fcb00884548ae4f7
    Author: Mike Nisbet <[email protected]>
    Date:   Thu Apr 13 11:15:20 2023 +0100

        remaining supported material values

        in example, but many dupes still missing

    commit 585aa46
    Author: Mike Nisbet <[email protected]>
    Date:   Thu Apr 13 10:14:08 2023 +0100

        refactor to make more generic

    commit 8391db8
    Author: Mike Nisbet <[email protected]>
    Date:   Wed Apr 12 18:01:31 2023 +0100

        Tidy up, roughness

    commit 585f322
    Author: Mike Nisbet <[email protected]>
    Date:   Wed Apr 12 17:14:59 2023 +0100

        formatting tidy up

    commit 4469656
    Author: Mike Nisbet <[email protected]>
    Date:   Wed Apr 12 17:11:24 2023 +0100

        Cache material lookups

    commit 7ec045d
    Author: Mike Nisbet <[email protected]>
    Date:   Wed Apr 12 16:17:25 2023 +0100

        Texture Offset, Scale

    commit 07c6e53
    Author: Mike Nisbet <[email protected]>
    Date:   Thu Apr 6 12:04:50 2023 +0100

        tidy up

    commit aaa1329
    Author: Mike Nisbet <[email protected]>
    Date:   Thu Apr 6 11:47:02 2023 +0100

        Generic lookup

    commit ac6a9de
    Author: Mike Nisbet <[email protected]>
    Date:   Wed Apr 5 15:33:24 2023 +0100

        Hardcoded color animation

    commit b288278
    Author: Mike Nisbet <[email protected]>
    Date:   Fri Mar 31 15:38:15 2023 +0100

        Read pointer path
@mikeskydev mikeskydev marked this pull request as ready for review April 20, 2023 15:36
@lyuma
Copy link

lyuma commented Apr 20, 2023

I noticed this is implementing material animations as MaterialPropertyBlock animation, which apply to the whole Renderer.

data.TargetType = TargetType.Material;
data.AnimationClipType = typeof(Renderer);
...
template = "material.baseColorTexture_ST.";
data.PropertyNames = new[] {$"{template}x", $"{template}y"};

where the nodelist is every node which uses that material, such as:

m_MaterialUsages[materialId].AddRange(m_NodeUsages[meshId]);

The consequence of this implementation choice is any glTF document which uses multiple primitives on a single mesh node will cause all materials on that primitive to be animated, which is not correct.

For example, imagine a stoplight as a mesh with 4 primitives/materials: the base chassis of the stoplight, the green bulb, yellow bulb and red bulb. A hypothetical animation document may animate red emission to 0 and animate green emission to vec4(0,1,0,1) to simulate a slight changing to green. In the Unity implementation, this will animate all baseColor on the mesh node to the same color, which will even include the base material defining the stoplight chassis itself.

As for implementing this properly, there are a few possibilities, but none will be easy, and some may have runtime performance implications.

  1. One approach is to split mesh primitives into separate nodes when individual materials are targeted with animation. This allows typeof(Renderer) to be used for MaterialPropertyBlock animations with few other changes, but it may add complexity to other parts of the import process.
  2. Another approach is to use a runtime component with animatable properties, such as how AnimationCameraGameObject is implemented. A AnimationMaterialGameObject with the hardcoded set of known material properties, or an array of properties could perhaps work similar to typeof(Renderer), and then can forward the animated material properties. However, runtime performance could be bad. To avoid the overhead of void Update(), could property setters be used?
  3. Like 2 but with possibly less overhead: Create a set of game objects for each animated material, with no MeshFilter and a MeshRenderer with one material. Every frame, copy the properties in the MaterialPropertyBlock to the corresponding sharedMaterial - this will create some dummy game objects, but it will give something nice for animations to target that avoids splitting meshes.
  4. Finally, different shader graphs can be used for each material to ensure properties are named by their material ID when there is a conflict (for example, material.baseColorTexture3_ST for the green bulb. This means a lot of shader compiles, and it break shader compatibility, so probably the worst option.

Some questions I have with AnimationCameraGameObject:

  1. in AnimationCameraGameObject, void Update() runs before animator update, not after. See here: https://docs.unity3d.com/Manual/ExecutionOrder.html This code should be moved to LateUpdate()
  2. is there a way to avoid ticking the camera scripts every frame if nothing is animated? Would property setters be better or worse given it will run for every property.

@mikeskydev
Copy link
Author

Hey @lyuma, thanks for the review!

On materials, I was aware of the multiple material targeting problems that might arise (great example by the way), however I think glTFast is already assigning one gameobject/renderer per primitive right now for separate reasons? @atteneder feel free to correct me here!

On camera animations:

  1. Thanks so much for pointing that out! Will change.
  2. This was a concern of mine too, I wanted to avoid using any type of update! However it seems Unity's legacy animations only works on serializable fields, not properties, which means we can't do any of the setter logic needed to massage the raw glTF properties into Unity ones. I also figured as some properties like Ortho matrix calculation depends on multiple properties, maybe the tick loop wouldn't be too bad?

@ptc-rgrasset
Copy link

ptc-rgrasset commented May 2, 2023

@atteneder : any plan on merging this PR ? We will be interested.

(there seems to be a renewed activity on the PR for the extension from the interactivity group, I'm hoping the extension get merged/ratified at some point in the near future)

@CLAassistant
Copy link

CLAassistant commented Jul 10, 2023

CLA assistant check
All committers have signed the CLA.

@mikeskydev
Copy link
Author

mikeskydev commented Jul 10, 2023

@atteneder is this CLA bot legit? Nevermind, I was wary as it asked for write access to gists, but after a reload it seems fine with the bare minimum permissions.

@atteneder
Copy link
Owner

@ptc-rgrasset I'm most definitely interested in this work! I just haven't had the time to go into review, sorry about that.

@mikeskydev Yes, the CLA assistant is legit. Thanks to all who signed already ❤️

@mikeskydev
Copy link
Author

mikeskydev commented Nov 6, 2023

Replaced by Unity-Technologies#7 in the Unity fork, and updated to v6. (Is this the right move?)

@atteneder
Copy link
Owner

@mikeskydev great move! :)

@mikeskydev mikeskydev closed this Nov 6, 2023
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
5 participants