-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
EXT_property_animation extension proposal #1301
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for opening this!
I appreciate that your approach is infinitely flexible, but I'm worried that comes at the risk that there will be no complete implementation. E.g. there are some things we cannot animate in three.js without re-compiling a shader, and if we can't animate at 60-90fps we're probably not going to ship it.
Personally I'd feel better about including an explicit list of properties that may be animated, and adding things to that list after determining that they make sense and can practically be animated. This would make it vastly easier to test conformance. I realize we haven't established whether extensions should use versioning, which might be important here, but let's not have a logistical hurdle get in the way of designing this robustly.
Would you help me out by describing what types of properties would need recompilation? I've attempted to avoid this particular issue by limiting the data types and not allowing index object references. I'm not apposed to having this extension enumerate all the properties that can animate including those in the current accepted and draft extensions. Going forward extensions could reference this spec and indicate which properties are allowed. If there is a way to avoid this level of detail I think we'd benefit from it. |
A few likely-to-be-problematic examples that would seem to be allowed as written:
This feels like enough examples that new extensions are commonly going to run into special cases. What about a whitelist of properties in the core spec, and a provision that current and future extensions may "opt in" with their own whitelist? It does not necessarily need to be an exhaustively maintained whitelist. For example, |
original JSON; default or implicit properties can be animated. `target` must | ||
reference a leaf property on an object that fits the data types supported by | ||
accessor elements. These types are `MAT4`, `MAT3`, `MAT2`, `VEC4`, `VEC3`, | ||
`VEC2`, and `SCALAR`. Properties that don't fit these data types cannot be |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any use of matrix interpolation?
accessor elements. These types are `MAT4`, `MAT3`, `MAT2`, `VEC4`, `VEC3`, | ||
`VEC2`, and `SCALAR`. Properties that don't fit these data types cannot be | ||
animated. Animation of boolean or enum values is prevented as well as any | ||
indexed object reference within the glTF JSON (i.e. a property animation can't |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the current state of glTF 2.0 core spec, we could say that animation of all integer
properties must be disallowed. This would cover the list above.
```json | ||
"animations" : [ | ||
{ | ||
"channels" : [], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Empty lists are forbidden by the current schema. Why not just re-use the existing path
property (since the schema allows arbitrary strings there)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the sample such that it doesn't fall into this problem. We can't use the path
as it exists because its defined as an enum value with only the four existing supported values.
There may be assets that wish only to animate some property values but not TRS or weights. Suggestions welcome for schema change or a best practice note for an identity animation that would go in core spec channels
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The schema allows any string in addition to pre-defined four values.
glTF/specification/2.0/schema/animation.channel.target.schema.json
Lines 27 to 29 in 09a404d
{ | |
"type": "string" | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does but the validator and more than one engine makes this a strict enum value. Plus this schema requires a node reference which isn't required with the JSON Pointer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Node reference is not required
"required": [ "path" ] |
The latest validator revision (wip) no longer treats unknown enum values as errors.
The documentation about extensions explicitly says that they can extend allowed enums.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see how it may work although this distinction complicates promotion a bit (so schemas must be remade for minor version updates).
Looking again at the animation example above, it seems that target
isn't referenced by any other property but inlined into channel
, so it's neither case, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
schemas must be remade for minor version updates
I'm not sure what you mean. What schema changes are you talking about?
Looking again at the animation example above, it seems that
target
isn't referenced by any other property but inlined intochannel
, so it's neither case, right?
target
is being referenced through animations[n].channels[m].target. Basically, any enum that will be parsed and interpreted by an implementation loading the core spec falls into case 2.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What schema changes are you talking about?
Updated properties reside in the extension object for now. If/when the same functionality goes into 2.x
core version, the schema will be different. That would complicate implementations willing to support all combinations of core versions / extensions.
target
is being referenced through animations[n].channels[m].target.
I'd say that target
is contained within channel
object, not referenced by it. So an engine may transitively treat the whole channel
object as "unknown" as well as animation
. Given that there're no external (indexed) references to any of animation-related objects, I don't see much difference with top-level image
with updated mimeType
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see much difference with top-level
image
with updatedmimeType
.
I suppose it depends on implementation. If an implementation is a top-down loader, then I see a difference. Images referenced only by an extension will never be loaded if the extension is never read and thus the types in the image can be extended without fear of existing implementations falling over.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose it depends on implementation.
Exactly. Given that the spec allows an asset to have only, e.g., images
collection, we can't assume that an arbitrary implementation won't try to load them in advance unless we want to incorporate "top-down" approach into spec's compatibility strategy (which may be a good idea after all).
The existing schema prevents us from extending animation.channel.target
with an extension object because animation.channel.target.path
is required. With versioning concerns in mind, this means that the safest way of designing the extension schema is to use a new top-level collection.
samplers list and the `sampler` reference indexes into this list for the | ||
enclosing animation. | ||
|
||
The sampler's output accessor type must match the targeted property's type. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd propose to provide a full list of allowed accessor types for each animatable core spec property.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Must all keyframe values comply with limitations specific to the animated property (e.g. min
/max
for material factors)?
Engines already have to solve that; e.g. a material may be reused by a skinned and non-skinned mesh, meaning you need different shaders and so must collect and clone the materials (at least this is how it works in three.js). I'm a bit worried about this... now if a top-level material is targeted we need to track down the nodes that use it, and possibly clones of the material created e.g. because of skinning. The three.js animation system generally targets nodes and their subproperties, and is based closely on Unity's so I'm guessing the situation is similar there. Reusing materials is also important for an application, and having the authoring tool split identical materials in order to target them with different animations isn't ideal. On the other hand, if materials are reused it is possible to modify uniforms on a per-object basis while rendering. (This may be more complex for the client, but is achievable and likely to be worthwhile.) I don't think easier implementation is a sufficient reason for targeting top-level objects with animation. It feels less natural conceptually, and the fact that it loses information about where materials could otherwise be reused worries me.
Potentially across multiple scenes, also? |
This was a previous iteration description having node/scene based targeting. I think both approaches have their downsides; clarity of effect and difficulty of implementation in one place or another. It'd be best to have a target description that is explicit where we can describe "everything with this material" or "just this instance of this material" but I've not encountered an animation system that is this descriptive. In three.js' animation system when targeting The three.js proof of concept I did not attempt to collect up objects that would need to have clones based on an animation. Would the algorithm be simply if material is part of any animation assign a unique clone it unless it was previously cloned? It seems like it would be better to group them based on the key data and top level target (i.e. three blinking lights on my jet model, should be one animation on one material). |
Perhaps it's worth advising what a producer should do for an animation that doesn't have any regular glTF channels since it's not immediately obvious. Current syntax requires a non-empty "channels" : [
{
"sampler" : 0,
"target" : {
"path" : "scale"
}
}
] because the spec says that channels without edit: ah, I see this was discussed above. |
Does this extend to animating KHR_texture_transform properties on a texture info object that doesn't already use KHR_texture_transform? |
Yes I think the language of the extension allows that. Though supplying a requires or uses extension should be necessary as those properties are undefined without the target extension. |
@najadojo @bghgary @donmccurdy Could you please elaborate a bit on the positioning and the life-cycle of this extension? I understand that this extension has I think that at some point, we'll want to have something very similar to this extension in glTF core. Moreover, I'm still concerned about (from comments above):
and
since such behavior doesn't follow the schema. |
This extension was a proposal for something we were thinking about internally, unfortunately we are no longer perusing those plans. I started with EXT because I was able to prototype functionality in multiple engines and tools. There has been pretty good engagement in the discussion for this PR but I don’t believe enough to solidify the proposal. It may make the most sense to archive this PR until future additions to glTF core are underway. |
Added this as a suggestion for KhronosGroup/glTF-Asset-Generator#414.
I think it would be useful to have a general syntax for extensions to use when adding animatable properties (the syntax in this PR seems reasonable, also see #1346) and if that requires changes to glTF core at some point we should consider it. But I do still have reservations about making things animateable-by-default, and would prefer that we consider allowing animation property-by-property as part of any new extension. For example, |
That's something I'd avoid, because it will likely lead to unmanageable fragmentation. |
@lexaknyazev could you explain further? In the long term, my opinion is that when we add any extension where animation is likely to be needed, that extension should define which of its properties can be animated, using syntax that is (a) part of glTF core, or (b) not in core but at least consistent across extensions. I only suggest Are you concerned about having extensions define animation, having inconsistent channel target syntax, or something else? |
/Bump. I think it would be helpful to have a general plan for (eventually) supporting additional animated properties. Example:
In retrospect, it would have been useful to have an animation syntax for arbitrary properties in place from the beginning, so that extensions like |
A few more things I'd like to track here:
|
No, I'm not suggesting an extension. Just wondering what the options are. There are other animations that would ideally be supported that aren't suitable to JSON pointers (e.g. animate one morph target at a time). Perhaps we can have animation shorthand for them without adding new JSON schema for the features.
It seems like hack to support zero-scale, degenerate matrices throughout an engine just to disable a node tree, when engines already have visibility as a feature. 😕 Another option is to move the node (far) out of the probable camera frustum. |
Why aren't JSON pointers suitable for animating one morph target? As you said above, it'd just be |
Let's not do this. Some viewers will adapt the default view to include the entire scene, including distant nodes. And some renderers, including Cesium, will show much of the inner Solar System, making it unclear how far one would have to translate a node to get out of the way. Scaling to zero isn't elegant, but it works even without hacky optimizations, and even on under-powered late-1990's era hardware (ehh... probably best not to ask me how I know this... 😅 ) |
Oops, yes that might be fine.
I wouldn't want to prescribe this as the official way to toggle visibility in glTF, but yeah it might be good enough. We could also introduce one (last?) pre-defined path, like the glTF 2.0 ones...
... and use JSON pointers for everything else. I don't know what I prefer. |
Would visibility be a boolean or a float value? |
Accessor componentType does not allow boolean. |
I don't mean the actual type on disk. Another way to ask, is visibility expected to be (0 and 1) or (0 to 1)? |
I think 0 or 1. |
I updated the comment above to indicate that Babylon.js has an implementation of this extension from @najadojo. It is a bit out-of-date as it was implemented a while back. |
Here is a POC in Babylon.js I did to animate KHR_texture_transform properties using EXT_Property_animation proposal https://github.com/Jaswant99/Babylon.js/tree/ext_prop_anim_POC . |
Hi, I'm from Asobo Studio, I'm thinking to implement this in FlightSimulator
what do you think? |
@elpie89 this extension is meant to allow animation of material properties, yes. However, it is just a proposal at this stage. |
would be very helpful for UV scrolling/offset for gif like animations |
Hello, |
This is a very high-level comment on an idea with a lot of moving parts, but I wonder if a some mechanism for procedural textures (see #1889) might not be a better alternative to the (difficult to fully scope, define, and implement?) approach we've been working on here. Procedural textures with vertex displacement potentially allow a much broader range of use cases (see Houdini VAT: https://github.com/keijiro/HdrpVatExample) than JSON pointers to specific glTF properties, and are easier to normatively define — see the open OSL implementations of MaterialX Standard Nodes. One case this does not cover would be animation of lights, but that's much easier to define in a simple standalone extension. |
Merged this into #2033 |
This is fully superseded by the glTF Object Model and KHR_animation_pointer, so this PR should be closed. As for the discussion of node visibility, this is covered by KHR_node_visibility, which is close to complete. |
No description provided.