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

Spec very ambiguous about multiple animation support. #1052

Closed
reduz opened this Issue Aug 3, 2017 · 32 comments

Comments

Projects
None yet
8 participants
@reduz
Copy link
Contributor

reduz commented Aug 3, 2017

I'm very disappointed, I implemented 100% of the spec in Godot. Spent 4 days on it. At the time I'm done with it, I realize that, unlike what it proposes through the whole spec, glTF does not support multiple animations.

Guys, It's 2017 and all DCCs and game engines support multiple animation clips, so it should be perfectly possible to export all character animations in a single file without relying on cheap tricks like segmenting a timeline or having to export one animation per file (with the whole scene cloned on it). Collada and FBX support this perfectly.

In the spec, the term animations, plural, is used through the whole thing. It totally makes sense, any animation works on a number of channels. By reading the spec it becomes obvious that multiple animations can be stored in the format.

Yet, when I go try the demos.. every demo I find uses an animation for a different node. For example on this demo:

https://github.com/KhronosGroup/glTF-Sample-Models/tree/master/2.0/BoxAnimated

To get the animation to look like in the gif, you have to play all animations together.
In this other demo:

https://github.com/KhronosGroup/glTF-Sample-Models/tree/master/2.0/CesiumMan

Each bone has an animation of it's own. you must play all animations together in order to see the guy walking.

So, please change the spec to make it very clear to implementers that, in 2017, a new format does not support multiple animations for real, or that there is a strict requirement for them to be played at the same time for everything to look properly. Please also urgently add an official extension to support REAL multiple animations, so this format can be used for 21st century workflows.

I suggest: KHR_actual_multiple_animations or similar.

@darksylinc

This comment has been minimized.

Copy link

darksylinc commented Aug 3, 2017

+1 to this.

Juan is trying to convince me to support glTF in Ogre as well, and this is a deal breaker.

If popular exporters default to exporting animations as one node per track which is further encouraged by the official samples, the format is useless to us.

The specs should be very strict on that animations should be done by having multiple nodes being animated in one track; so that each animation is independent of each other.

@donmccurdy

This comment has been minimized.

Copy link
Member

donmccurdy commented Aug 3, 2017

Hi @reduz, this is an issue with our sample models, and (related) the Blender Exporter is missing support for exporting actions separately (KhronosGroup/glTF-Blender-Exporter#39). However, the glTF spec itself very much does support what you're asking for here, there is no extension necessary.

We will certainly work on updating the sample models to be more in line with expectations here.

Thanks!

@reduz

This comment has been minimized.

Copy link
Contributor

reduz commented Aug 3, 2017

@donmccurdy Oh I'm very glad this is a misunderstanding then. My fear is that this behavior of exporting every track in a separate animation becomes the "de facto" standard, even if not mentioned in the spec. This kind of ambiguity is, in my opinion, what doomed Collada because every implemetor did whatever they wanted with it.

gltF2.0 is pretty much the best exchange format i've ever seen and worth pushing it to be supported by every DCC and game engine. As an industry we need this, given the high reliance on libfbx and it's very restrictive usage license.

At Godot we maintain a small Blender exporter that supports the full Collada specification, including multiple animations (because the built-in one from Blender sucks). If you need it as reference please let me know.

I would really appreciate if the test models could be updated at some point in the near future, so I can call my implementation complete and start pushing the format as much as we can from our side.

@javagl

This comment has been minimized.

Copy link
Contributor

javagl commented Aug 4, 2017

Part of this was originally written in response to KhronosGroup/glTF-Sample-Models#108 , but applies here nevertheless

It could help if you described more clearly what you refer to. There may be multiple animation objects. Each animation object may have multiple animation.channel objects. This leaves some degrees of freedom for "mix and match", i.e. how you structure the data for your particular use case.

(A side note: Remember that glTF is supposed to be a transfer format. A format that is only consumed by clients. To put it that way: The actual structure of the animation data in the file "does not matter", as long as it is capable of representing all sorts of animations that might be created with any authoring tool)

So, for example, regarding the particular example of the "BoxAnimated" that you referred to elsewhere: This was created from a COLLADA file that someone created as an example. And although I don't know for sure, I could imagine that he intentionally created two separate animations, to make the sample less trivial, and to show that multiple separate animations can be created.

(Another side note: The spec is indeed a bit quiet about playing the animations - particularly, about the animation loop behavior - I opened #573 a while ago, but there had been different priorities in the meantime).

As mentioned in KhronosGroup/glTF-Sample-Models#108 (comment) and KhronosGroup/glTF-Sample-Models#107 : There should indeed be more test models for different animation configurations (and personally, I'd once more prefer "simple" ones that only show the different ways of structuring animations in the JSON file, conceptually).

If you think that an extension is necessary, it would be good if you could roughly sketch a few lines of JSON that could encode the type of animations that cannot be represented in glTF right now, but as already mentioned by donmccurdy : This is probably not necessary.

@reduz

This comment has been minimized.

Copy link
Contributor

reduz commented Aug 4, 2017

@javagl All samples have animations exported like this, I think probably due to how Collada2GLTF works. Still, it's great that @donmccurdy said they will be fixed.

Specification and demos need to be as strict and unambiguous as possible, otherwise we end up with the same problems Collada had.

@donmccurdy

This comment has been minimized.

Copy link
Member

donmccurdy commented Aug 4, 2017

@reduz @javagl Definitely there are some changes and additions that would be beneficial for the sample models. I've written up my suggestions in KhronosGroup/glTF-Sample-Models#108.

For the spec itself, I think it is fine that the spec is quiet about playback/looping behavior; no need to change that. Although the spec does not need a technical change, I do suggest we add an implementation note along these lines in the Animations section:

Implementation Note: While content authors may use animations as they please, common practice is to group channels into a single animation if they are intended to be played back as a unit. For example, "Walk" and "Run" animations might each contain multiple channels targeting the model's bones. The viewer or engine may choose how and when to toggle or crossfade between the two animations.

@javagl

This comment has been minimized.

Copy link
Contributor

javagl commented Aug 4, 2017

According to the discussion at KhronosGroup/glTF-Sample-Models#108 (comment) , the main point seems to be that there could be several ways of structuring animations, and the outcome is not covered by the spec:

  • Individual animation objects that have to be played simulatneously to achieve the desired effect. (This could be covered and be made unambiguous by converting them into one animation with multiple animation.channel objects)
  • Individual animations that may (at the discretion of the user) be played simultaneously or separately
  • Individual animations that must not be played simultaneously. The obvious problematic case here is when two different animations refer to the same animation.channel.target. From a quick glance at the spec, this does not seem to be covered there at all. I cannot imagine that this was not yet discussed, and I'll probably do some issue-archeology to check this, but maybe one of the spec-masters ( @lexaknyazev ) knows more here.
@donmccurdy

This comment has been minimized.

Copy link
Member

donmccurdy commented Aug 4, 2017

Individual animations that must not be played simultaneously.

I don't think this last case is necessary; and it would be hard to define. For example, "dance" and "walk" animations are distinct, but even so the viewer might choose to play them both for a brief period of time while crossfading from one to the other.

@darksylinc

This comment has been minimized.

Copy link

darksylinc commented Aug 4, 2017

Of course, "anything goes". But too much lenience makes the format useless if the exporter only supports the most useless method of exporting animations.

In most game engines, animations are structured as independent single chunks of animations. For example:

  • Run
  • Walk
  • Jump
  • Idle
  • etc...

Now, some more advanced games may chose to follow its own convention and define more specific set of animations:

  • Run_Torso
  • Run_Legs
  • Walk_Torso
  • Walk_Legs
  • Idle_Torso
  • Idle_Legs
  • etc

In this case the game will have to run both Run_Torso & Run_Legs at the same time. "_Torso" will animated everything that belongs to the "upper side" of the body from the wrist, while "_Legs" animate the lower side. But gamedevs may also want to mix Idle_Torso with Walk_Legs and do more interesting animation combinations.

Other games may define even more thorough conventions for splitting up animations (rather than splitting everything just in 2).
This is normal.

However this is not what's being criticized. What we're criticizing is that right now all the models in the sample repository are doing the following:

  • Walk_left_hand_finger0
  • Walk_left_hand_finger1
  • Walk_left_hand_finger2
  • Walk_left_hand
  • Walk_left_elbow
  • Walk_left_shoulder
  • Walk_torso0
  • Walk_torso1
  • Walk_torso2
  • Walk_torso3
  • Walk_waist
  • Walk_left_quadriceps
  • etc...
  • [repeat with right side of body]

That is, one animation track per node. That's what reduz & myself are complaining about.

This is completely useless, and only puts a much bigger burden on us importers, who must implement code to convert all of these animations separated back into one; and also ask the user which animations go into each set.

Why is it useless? Because if I'd wanted to animate with the granularity of a single bone, I'd disable the keyframes for that particular bone, rather than having +20 different tracks and no way of knowing what goes with each.

Not to mention that all major content creator tools (e.g. Maya, Blender, 3DS Max, etc) group animations in sets. So it is to be expected an animation exported from these packages to be grouped in the same hierarchy that it was defined by the artist.

And if somebody actually wants to do this (exporting one track per node/bone), he or she can do that. But it should definitely not be the default behavior.

And because the default samples are encouraging this behavior, some implementors may even become convinced glTF only supports one track per node, causing a nightmare of compatibility issues.

@javagl

This comment has been minimized.

Copy link
Contributor

javagl commented Aug 5, 2017

I don't think this last case is necessary; and it would be hard to define.

This may go beyond the core of this issue (the animation "grouping", which is for itself a valid point), but I think that the spec should at least mention the case of two animations that use the same animation.channel.target. (I don't even say that it has to dictate a certain behavior for this case. It could also say that the behavior of playing these animations simultaneously is unspecified. But I think it is important to at least create awareness that such a case could happen...)

@alugarius

This comment has been minimized.

Copy link

alugarius commented Aug 5, 2017

+1
This is very important

@reduz

This comment has been minimized.

Copy link
Contributor

reduz commented Aug 5, 2017

@javagl

the main point seems to be that there could be several ways of structuring animations, and the outcome is not covered by the spec

Yes, this is actually the problem, the spec is very ambiguous on something it should not be. There should be one way to make the animations, so we don't have to add stupid hacks to all the importers to make up for the ambiguity. This was terrible in Collada (a format I have a ton of experience with, writing exporters and parsers) and should be avoided at all costs in glTF. In fact, this is even worse because Collada, at least, gets this right.

As such, I hope @pjcozzi, @donmccurdy and the others clarify in the specification that animations are not intended to be played simultaneously as a single animation, or for exporting each bone in one of them. Afterwads, I hope the samples are fixed. This would make glTF2.0 ambiguity free and avoid future problems.

@pjcozzi

This comment has been minimized.

Copy link
Member

pjcozzi commented Aug 5, 2017

@reduz if you have suggested spec language and a tad of bandwidth, feel free to open a pull request for the spec change (and also add yourself to the acknowledgments section). We'll try to get someone to review soon.

@reduz

This comment has been minimized.

Copy link
Contributor

reduz commented Aug 5, 2017

@pjcozzi done, thanks!

@javagl

This comment has been minimized.

Copy link
Contributor

javagl commented Aug 5, 2017

Another crosslink: The issue about multiple animations having the same target was already discussed around #704 (comment) . The resulting PR brought the statement

Different channels of the same animation can't have equal targets

into the schema ( https://github.com/KhronosGroup/glTF/blob/master/specification/2.0/schema/animation.schema.json#L10 ). But the case of multiple different animations is not yet covered in detail

BTW: Now I think that this is even more strongly related to #573 . One important point there was that "one animation" does not necessarily cover the time range that the animations have to span in order to be played back properly. This will also be solved when the animations are combined into one with multiple channels, because then, the sampler input for each animation will automatically define the duration of the animation as a whole.

@alugarius

This comment has been minimized.

Copy link

alugarius commented Aug 5, 2017

@reduz you are awesome

@emackey

This comment has been minimized.

Copy link
Member

emackey commented Aug 7, 2017

@reduz Did you create a pull request for this? Once this is created, please link the issue here. Thanks!

Also, thanks @donmccurdy for filing KhronosGroup/COLLADA2GLTF#66, I think that's the actual root of all these animation problems.

@reduz

This comment has been minimized.

Copy link
Contributor

reduz commented Aug 7, 2017

@emackey I think i put a suggest edition thing in there, no idea how to see it now..

@emackey

This comment has been minimized.

Copy link
Member

emackey commented Aug 7, 2017

@reduz I mean I think you created a branch but forgot to click "Create pull request" on your new branch. If you follow this link, GitHub shows the create PR button for your branch in the upper-left.

@pjcozzi pjcozzi added the bug label Aug 9, 2017

@ghost

This comment has been minimized.

Copy link

ghost commented Aug 10, 2017

I modified the BoxAnimated sample to have a single animation with 2 channels and 2 samplers as discussed above and it works great in the gltf viewer. I made a fork and hastily threw it up over here: https://github.com/PlutoVR/glTF-Sample-Models/tree/single-animation-box/2.0/BoxAnimatedSingleAnimation

@reduz

This comment has been minimized.

Copy link
Contributor

reduz commented Aug 10, 2017

@emackey oh, apologies, forgot to create the PR

@emackey

This comment has been minimized.

Copy link
Member

emackey commented Aug 10, 2017

I modified the BoxAnimated sample to have a single animation [...]

@willswannackpluto Thanks, but this model is part of a set that are auto-generated from COLLADA2GLTF. So the fix will not be to modify them all by hand, the fix will need to be a code change for KhronosGroup/COLLADA2GLTF#66.

@javagl

This comment has been minimized.

Copy link
Contributor

javagl commented Aug 10, 2017

As discussed during the last call: In fact, the culprit here may not be COLLADA2GLTF. There are already two animations in the COLLADA input files, as can be seen at https://github.com/KhronosGroup/glTF-Sample-Models/blob/master/sourceModels/BoxAnimated/BoxAnimated.dae#L66 (I assume that this is similar for the other sample model inputs, although I didn't check them all). So it may be that COLLADA2GLTF is doing the right thing, as good as it can based on the input...

@donmccurdy

This comment has been minimized.

Copy link
Member

donmccurdy commented Aug 10, 2017

Hmm that is a point... Apparently COLLADA groups animations using the <animation_clip/> extension, and none of these COLLADA source models use that extension as far as I can tell. It doesn't look like COLLADA2GLTF has implemented that extension yet, presumably because the samples didn't need it.

This raises the question, were glTF animation entries intended to be treated as clips? Or are they analogous to COLLADA's <animation/>, in which case they should be grouped into some higher-order type not yet in glTF?

My own opinion would be that a runtime format has no use for a layer of abstraction grouping channels by target node, so we should treat glTF animations as analogous to COLLADA's <animation_clip/>, not <animation>. But I'm very new to COLLADA, so input from others would be great...

If that is agreed, then all COLLADA animations should be written by COLLADA2GLTF as channels of a single glTF animation (in the absence of <animation_clip/> groups).


UPDATE: Actually, the glTF spec is not silent on this as I thought. It includes language in the animation section, "Animate two nodes with different samplers", which I would interpret to mean that glTF animations are in fact analogous to COLLADA's <animation_clip/>.

@reduz

This comment has been minimized.

Copy link
Contributor

reduz commented Aug 10, 2017

@donmccurdy In Collada, unless animation_clip is used, everything exported is assumed to be a single animation. Clearly the problem here is that COLLADA2GLTF is not supporting this properly.

@Nehon

This comment has been minimized.

Copy link
Contributor

Nehon commented Aug 12, 2017

I'm writing a gltf loader for jMonkeyEngine and stumble on this issue.
It's very cumbersome to have separate animations for different nodes, especially when it comes to bone animation.
However, I found here a good example of a single animation for multiple nodes.
https://gltf.sketchfab.com/models/busterDrone.zip
Source model: https://sketchfab.com/models/294e79652f494130ad2ab00a13fdbafd
This is not a bone animation, only spatial animation, but it, imho, depicts how animations should be serialized by exporters..

@pjcozzi

This comment has been minimized.

Copy link
Member

pjcozzi commented Aug 12, 2017

Great, thanks for sharing @Nehon! There's also a pull request to update the spec, #1069, and we plan to update the sample models.

Keep us posted on your progress in JME!

@javagl

This comment has been minimized.

Copy link
Contributor

javagl commented Aug 12, 2017

(Off-Topic: @Nehon For possibly obvious reasons, I'd be curious to see what you've done to bring glTF into JME. There doesn't seem to be a public repo yet, is there? If so, JME could also be added to #867 )

@Nehon

This comment has been minimized.

Copy link
Contributor

Nehon commented Aug 12, 2017

It's in a branch of the engine for now.
It's a very early WIP, though.
If you are willing to discuss it feel free to come by our forum as it's a bit off topic of this issue :)

@emackey

This comment has been minimized.

Copy link
Member

emackey commented Aug 15, 2017

Can this be closed now that #1069 is merged?

@reduz

This comment has been minimized.

Copy link
Contributor

reduz commented Aug 15, 2017

I guess, but I would leave one of the reports open until the demos are fixed

@donmccurdy

This comment has been minimized.

Copy link
Member

donmccurdy commented Aug 15, 2017

All remaining actions are related to the sample models or tools that generate them, so I will close this issue.

For anyone interested in tracking them, the open items are:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment