Fix discrepency between ids and names in AnimationCollection

[lilleyse]

Bug brought up on the forum: https://groups.google.com/forum/#!topic/cesium-dev/XDhrq2ejwR4

ModelAnimationCollection.add expects options.name to be an animation name, but with the switch to glTF 2 the function only worked if options.name was the array index. The changes here add back support for adding animations by name and not index. Part of this was cleaning up other areas of the animation code that were still stuck in the glTF 1.0 mindset.

[cesium-concierge]

@lilleyse, thanks for the pull request! Maintainers, we have a signed CLA from @lilleyse, so you can review this at any time.

I noticed that CHANGES.md has not been updated. If this change updates the public API in any way, fixes a bug, or makes any non-trivial update, please add a bullet point to CHANGES.md and comment on this pull request so we know it was updated. For more info, see the Pull Request Guidelines.

I am a bot who helps you make Cesium awesome! Thanks again.

[lilleyse]

In the long term in would be better if the add function took the index instead of the name, but that would require a breaking change. Hard to tell if it's worth it.

[pjcozzi]

Code looks OK at quick glance.

@ggetz could you please review and test?

See #927 - I think id vs. name issues in Cesium go back several years before 2.0.

[ggetz]

@lilleyse What's a good way to test this? Is the animated aircraft model sufficient?

[lilleyse]

Sure, any animated model will work.

[ggetz]

Looks good, thanks @lilleyse !

[emackey]

This appears to be a breaking change, as specifying the index no longer works. The name is required now.

[emackey]

Also note that the name is optional in glTF 2, so we need a way to specify by index.

[emackey]

For backwards compatibility, name must accept either an index or a name. Previously, this field only accepted an index, in spite of its name. Some models, such as "BrainStem", don't name their animations, as names are optional.

[lilleyse]

Specifying the index only worked by accident, which was more of a bug from the 1.0-2.0 transition. For glTF 1.0 it expected the id, which updateVersion now moves to the name property. I don't think this change breaks any existing models.

I also thought about overloading name, but it seemed possibly confusing.

[emackey]

This change does break code that @hpinkos recently merged into another project.

Agreed that it was only working "by accident" and probably only for 2 releases or so.

But to be clear, with this change there is no way to individually toggle the animations in the BrainStem model (which incidentally shouldn't be using individual animations, but that's beside the point, the point is that animations don't require names anymore in glTF 2.0, only indicies).

[lilleyse]

I guess what I mean is apps built before 1.36 shouldn't be broken, but anything after that passes in index instead of the name to get around the slightly weird behavior here would be broken.

[EDIT: saw your updated comment, I think we agree]

I'm alright with the overloaded behavior, I'll make that change in a bit.

[emackey]

I'm OK if you don't want to overload, and make the index a separate parameter. It doesn't break any old code, only new code. But some sort of option for specifying the index is required, because glTF models no longer require names.

[lilleyse]

@emackey @ggetz @hpinkos updated. I went with the options.index approach.

[pjcozzi]

@emackey please merge when ready.

[emackey]

Awesome, thanks. Tests pass, and I confirmed that both new options work as intended. I tried each option on Cesium Air (glTF 1.0), BrainStem (glTF 2.0 without names), and AnimatedMorphSphere (glTF 2.0 with names).

Only wrinkle is, Cesium allows you to specify both an index and a name in the same call. When this is done, the index "wins" over the name, but it works fine. Is this behavior OK? I'm OK with everything else. Thanks!

[cesium-concierge]

Congratulations on closing the issue! I found these Cesium forum links in the comments above:

https://groups.google.com/forum/#!topic/cesium-dev/XDhrq2ejwR4

If this issue affects any of these threads, please post a comment like the following:

The issue at #5815 has just been closed and may resolve your issue. Look for the change in the next stable release of Cesium or get it now in the master branch on GitHub https://github.com/AnalyticalGraphicsInc/cesium.

I am a bot who helps you make Cesium awesome! Thanks again.