fixed Animation Keyframe IO

[ShynRou]

fix(blender-exporter): Animation._parse_pose_action now returns only existing keyframes, Animation._find_channels returns all channels

fix(animation): AnimationClip uses id instead index

[mrdoob]

@bhouston @tschw looks good?

[mrdoob]

@bhouston do the changes in the exporter look good to you?

[bhouston]

@godlzr, (who works with us at Exocortex) could you have a look at the Blender exporter this in PR and see if it works well with animations? I know the previous exporter has some issues. If you could also report some of the other issues we found with the master-branch Three.JS Blender exporter that has prevented us from upgrading to the current master one, i am sure that would be useful as well. I think that animation isn't the only issue.

[mrdoob]

I'll merge this for now and we can tweak later if necessary.

[mrdoob]

Thanks!

[mrdoob]

I think this PR broke these examples:
http://rawgit.com/mrdoob/three.js/dev/examples/webgl_animation_skinning_blending
http://rawgit.com/mrdoob/three.js/dev/examples/webgl_animation_skinning_morph.html

[ShynRou]

found this: models/skinned/marine/marine_anims_core.json

"parent":-1,

the bone index should never be -1

I fixed the issue in the exporter already:

old:

hierarchy.append({
constants.PARENT: bone_index - 1,
constants.KEYS: keys[bone_index]
})

new:

hierarchy.append({
constants.PARENT: bone_index,
constants.KEYS: keys[bone_index],
})

There are propably now more examples which json files are corrupted.

[bhouston]

I am unsure if you should consider all the existing *.json files as corrupted. Basically you are saying that everyone's pre-existing exported *.json files need to be re-exported. I am a little worried that this is not a good approach, it may be that you are modifying the meaning of the JSON files. IF that is the case all the exporters may need to be updated -- but again you may be invalidating everyone's existing JSON files.

[bhouston]

I'm pretty sure the change on this line is changing the meaning of what is in the JSON files and thus breaking all previously exported JSON files:

https://github.com/mrdoob/three.js/pull/9192/files#diff-b5362d762a2d0a4fb092a647e9165e9fL327

I highly recommend not breaking backwards compatibility as you will frustrate quite a few people. Changes have to be made such that they are backwards compatible to all pre-existing JSON files.

[mrdoob]

Yep. We should add a check there that keeps old json files working. Log a warning if re-exporting is really encouraged.

[ShynRou]

in the exporters/blender/addons/io_three/exporter/api/animation.py _parse_rest_action()
the index got previously already correctly set. I dont know about the other exporters.
For a backwards compatibility a version tag in the files would have been great.
A check if the the files animations are wrongly indexed, would be a crude way of fixing it.
But yes outherwise it propably will cause some problems.

[takahirox]

Quick question.

hierarchy.append({
constants.PARENT: bone_index,
constants.KEYS: keys[bone_index],
})

Why should parent property have bone's index, not parent bone's index?
The name is very confusing to me...

[ShynRou]

I suppose because it's the parent of the AnimationClip, since the parents bone index is useless to us and you'll certainly don't get it by decrementing.

Usually you would also not save all transformation channels in one clip per bone. But rather per bone each channel with it's keyframes individually, like fbx or collada does.

maybe a complete workover is necessary, but that would mean a lot of refactoring.

[bhouston]

I suppose because it's the parent of the AnimationClip, since the parents bone index is useless to us and you'll certainly don't get it by decrementing.

The bone_index is just the bone_index with a zero base. If the bone doesn't have a parent it is just -1. I am concern your change is confused and wrong.

If you look at the knight.js file it looks like this:

   "bones" : [
            {
                "parent" : -1,
                "name"   : "Armature.DEF_cog",
                "pos"    : [0,6.59559,-0.100367],
                "scl"    : [1,1,1],
                "rot"    : [-90,-180,-0],
                "rotq"   : [0,0.707107,0.707107,0]
            },

            {
                "parent" : 0,
                "name"   : "Armature.DEF_spine_base",
                "pos"    : [0,0,0],
                "scl"    : [1,1,1],
                "rot"    : [90.8287,-6.42209e-07,-9.42902e-06],
                "rotq"   : [0.712202,-6.25367e-08,-5.37697e-08,0.701975]
            },

            {
                "parent" : 1,
                "name"   : "Armature.DEF_spine",
                "pos"    : [0,0.582953,0],
                "scl"    : [1,1,1],
                "rot"    : [0.637364,1.78871e-05,-4.99159e-07],
                "rotq"   : [0.00556202,1.56068e-07,-5.22412e-09,0.999985]
            },

I believe you change with regards to bone_index is not correct both in the loader and in the exporter. I strongly believe you should not be changing the meaning of bone.parent.

[ShynRou]

Then please explain to me why you need the index of the parent bone in the AnimationClip?

The bone needs to know its parent bone to proper calculate its transformation matrix,
but the Animation only needs to know the bone it belongs to and that got to be the "parent" value.
I think it's just named confusingly.

[ShynRou]

To clear up this mess we should consider writing a format specification.

[bhouston]

ShynRou wrote:

Then please explain to me why you need the index of the parent bone in the AnimationClip?

Can you link to me the place in AnimationClip.js that references the parent bone? AnimationClip just has to know the bone names so that it can change their transform:

https://github.com/mrdoob/three.js/blob/master/src/animation/AnimationClip.js#L329

It is not aware of the hierarchy of the Skeleton/Bones in any way. It just sees a lot of named tracks and then it applies those tracks to the Bones/Nodes that have those names. That is it.

ShynRou wrote:

The bone needs to know its parent bone to proper calculate its transformation matrix,
but the Animation only needs to know the bone it belongs to and that got to be the "parent" value.
I think it's just named confusingly.

When Bones are parsed here:
https://github.com/mrdoob/three.js/blob/master/src/objects/SkinnedMesh.js#L24

You can see that they are created as Bone Nodes with the right name and the right parents (from the parent index). This is where the hierarchy is remembered from.

AnimationClip does not care about the bone hierarchy, it just cares about the bone names.

[ShynRou]

Can you link to me the place in AnimationClip.js that references the parent bone? AnimationClip just has to know the bone names so that it can change their transform:

Yeah and to get that information from the .json you need to have either the bones index per AnimationClip or the name.

The old code only worked because the exporter falsely exported every frame all transformations for every bone, if they have an animations or not. Which resulted in giant file sizes and no support for proper animation blending.

[titansoftime]

This has broken all of my animations. Re-exporting over 300 models is not something I really want to do =[ (if I have to sigh... so be it)

Is there another way?

[ShynRou]

Only proper way I see would be to start a new format with specifications and everything and leave the old code in there.

[bhouston]

@ShynRou I believe that if you are smart you can make this work without breaking everyone's existing scenes. I think we have to un-merge this and we have to find the specific case you want it to work in and we patch is so that it can work in those situations.

Also it is an evolving animation format, we can add new functionality while keeping backwards compatibility. We've done this a few times already.

[ShynRou]

@bhouston alright!
I would suggest to add a format-version tag to the file so future changes to the format could also be parsed properly while keeping backwards compatibility. Sounds good?

[bhouston]

@ShynRou, there already is a format tag at the beginning of all ThreeJS json files made in the last couple years: https://raw.githubusercontent.com/mrdoob/three.js/master/examples/models/skinned/knight.js

[ShynRou]

@bhouston oh, thanks! I'll make it work then in the next few days.

[bhouston]

@ShynRou, can you submit a PR in the mean time that reverts your changes? Basically just check out dev, revert the collapsed change set and submit that as a PR. This will unbreak dev for all the people using it like @titansoftime.

[ShynRou]

@bhouston I'll do it tomorrow morning.

[takahirox]

Just IMO,

1 parent bone index is useless for AnimationClip

Agree

2 we should be able to pass bone index

Agree because we'll be able to pass only existing keyframes

3 we should have bone index on hierarchy.parent property instead of parent bone index

Disagree because it'll break backwards compatibility and the name "parent" is confusing.

So how about passing bone index on the new property like hierarchy.index.
The code will be

var boneName = '.bones[' + bones[ hierarchyTracks[ h ].index !== undefined ? hierarchyTracks[ h ].index : h ].name + ']';

This can keep compatibility.
(And probably we need to put warning if hierarchyTracks[h].index is undefined)

[ShynRou]

@bhouston revert PR submitted

@takahirox I like that idea. Would clear up confusion and allow backwards compatibilty!

@mrdoob Should we start a proper File Format Specefication under docs?

[mrdoob]

@mrdoob Should we start a proper File Format Specefication under docs?

Sounds good! We should do it in the wiki though. https://github.com/mrdoob/three.js/wiki

[bhouston]

Just to be clear, there is already a compact named animation format -- the track set format that is used for all other animations, see the gear animation scene example -- it supports named tracks, sparse keys, and a lot of flexibility. I didn't introduce it for the skeletal part yet but it can just be used there. There is no need for a third animation format. We can just adopt the new track set animation format in more places. Just write the JSON so this can parse it:

https://github.com/mrdoob/three.js/blob/master/src/animation/AnimationClip.js#L83

But we should maintain full backwards compatibility.

[mrdoob]

@bhouston Agreed. But it would be nice if we could create a page in the wiki that shows how the json structure looks like.

[bhouston]

@mrdoob wrote:

@bhouston Agreed. But it would be nice if we could create a page in the wiki that shows how the json structure looks like.

I completely agree. :)

[ShynRou]

@mrdoob just looked at the AnimationTrack parser. It would definatly be wise to switch the bone animations to that system. My concern is that the overhead with this is quite big.

"keys": [{
"time": 0,
"value": [-2e-06,-6.71175,3.01972]
},{
"time": 5,
"value": [-2e-06,-6.71175,3.01972]
},{
"time": 70,
"value": [-2e-06,-6.52962,-38.1906]
},{
"time": 80,
"value": [-2e-06,-6.52962,-38.1906]
},{
"time": 145,
"value": [-2e-06,-6.71175,3.01972]
}],

I would pack it into one big array like the verteces/normals/... are:

keys{
"time":[ ... ],
"value":[...,....,...]
}

would save us about 30-40% of memory used for the keyframes.

[bhouston]

@ShynRou wrote:

I would pack it into one big array like the verteces/normals/... are:

Actually it is already converted into array buffers here in memory:
https://github.com/mrdoob/three.js/blob/master/src/animation/KeyframeTrack.js#L23

We could expose that same in-memory format into the JSON and pack it as Base64 (floats as strings suck big-time) or we could just create an binary file format that stores this more directly, sort of like how BinaryGeometry can be saved/loaded as binary files.

[ShynRou]

@bhouston
Great. Yap ASCII formats tend to suck at performance, but for consistencies sake I'd say changing it to an float array since thats the way it's done in the rest of the format at the moment (vertices, normals, uvs ...).

[bhouston]

@ShynRou wrote:

Yap ASCII formats tend to suck at performance, but for consistencies sake I'd say changing it to an float array since thats the way it's done in the rest of the format at the moment (vertices, normals, uvs ...).

Well, remember that the Blender exporter already supports the nested key/time/value format that you complained about above. I would suggest not replacing it with another text format, because now we have two text formats that represent the same thing and are basically both nearly equivalent in terms of inefficiency.

If you were to replace it, I would replace it with a Base64 or binary format. In the end we will hhave a binary animation format, like we have with BufferGeometry, so you could just jump straight there and implement that. Making a text format that is only a little bit more efficient isn't a great use of time and it also makes it even hard to maintain the Blender pipeline.

If you are going to change the animation format, you need to update the other parts of the Blender exporter that export in the nested key/value/time format -- you will need to also ensure that we can still load the old format so that we do not break backwards compatibility. I recommend against yet another text animation format in favor of a binary animation format, but I can see the argument, even though I do not think it is a good investment of time, for introducing an animation format that uses arrays in text.

[bhouston]

I will recuse myself from this conversation for the time being. Just please make sure that your PR is fully compatible with all existing animation examples.

[ShynRou]

@bhouston I'll do. Sorry for the trouble.