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 Animation Libraries #59980

Merged
merged 1 commit into from
Apr 11, 2022
Merged

Conversation

reduz
Copy link
Member

@reduz reduz commented Apr 7, 2022

Implements: godotengine/godot-proposals#4296

  • Instead of containing single animations, AnimationPlayer now contains libraries.
  • Libraries, in turn, contain the animations.
  • If the library name is empty, then it acts like it does until now (just play "animationname"). If you name the library, then you must play it as "library/animationname". This ensures backwards compatibility and keeping the existing workflow in case of not wanting to use multiple libraries.
  • Added backwards compatibility (scenes saved with the previous format will create a library on demand for the animations).

This paves the way for implementing the possibility of importing scenes as animation libraries, finally allowing to import animations separate from the 3D models.

Missing (will be done on separate PRs):

  • Make it possible to import scenes (dae/fbx/gltf) as animation libraries.
  • Make it possible for AnimationTree to import animation libraries on its own, so it does not rely on AnimationPlayer for everything.

Screenshots:

Animation Menu simplified, now has a Manage Animations option:
image
This opens the library manager:
image
List of animations also shows libraries if multiple ones exist (otherwise ignore it):
image

Thanks to @tihmas, @fire-forge and @kennethrapp for feedback on adjusting the implementation.

Bonus tracks:

  • Fixed a small bug in Tree where reselecting another item to edit the name triggered the wrong signal.
  • Added ability to add separators and disabling items to OptionButton

@reduz
Copy link
Member Author

reduz commented Apr 7, 2022

Give me a bit and I will post some screenshots of the UI changes
Done

editor/icons/AnimationLibrary.svg Outdated Show resolved Hide resolved
scene/animation/animation_player.cpp Outdated Show resolved Hide resolved
@reduz reduz force-pushed the animation-libraries branch from 8fc1031 to 1e4343d Compare April 7, 2022 13:51
@reduz reduz marked this pull request as ready for review April 7, 2022 13:51
@reduz reduz requested review from a team as code owners April 7, 2022 13:51
@reduz
Copy link
Member Author

reduz commented Apr 7, 2022

Alright, its now ready for review.

@reduz reduz force-pushed the animation-libraries branch 2 times, most recently from e37b86e to 13f0cfa Compare April 7, 2022 14:04
@RPicster
Copy link
Contributor

RPicster commented Apr 7, 2022

It would be great to see an example of real-life use and how it will improve there. I am a bit afraid that many users won't care or really look into this feature until it's too late ( and there is a total mess in the library).

My second fear is that in a bigger project, this will be absolutely cluttered and the animation library window will be unusable at some point. How is this management handled?

Maybe I also didn't grasp how libraries interact with the Animation Player.
If I just want to make a simple animation that has no way of being reusable in a way that makes sense (let's say an explosion on a effect scene).
Is there some way of excluding this from the library?

@reduz
Copy link
Member Author

reduz commented Apr 7, 2022

@RPicster If you don't really want to use the libraries, you don't use them and it just works the same as before (everything is put into the "default" library without you doing anything). This is majorly optional for those who want to take advantage of it.

@bitbrain
Copy link
Contributor

bitbrain commented Apr 7, 2022

I am wondering if these animation libraries could support blend spaces as well across all its animations.

@muno777
Copy link

muno777 commented Apr 7, 2022

Reposting this from my twitter comment:

For duplicate names - maybe there could be a way to change which library the player "prioritizes" during conflicts? Instead of always going alphabetical/etc.

Seems like it'd be a nice way to "swap out" certain animations for stuff like multiple types of weapons.

image

@kennethrapp
Copy link

kennethrapp commented Apr 7, 2022

Matching animation names are no longer necessarily "conflicts" when a new layer of abstraction (libraries) is added, they could be an intentional design decision. Rather than simply using the first option found in alphabetical order (which is unlikely to be what the user expects and intends), perhaps Godot should have a way of specifying the library to be used in that case or give an error when unspecified?

Maybe have AnimationPlayer.play use the default behavior when the first parameter is a string, but when an array is used, the first element refers to the library?

@fire
Copy link
Member

fire commented Apr 7, 2022

Many systems do use alphabetical or in this case lexicographical order because of sorting problems.

@tihmas
Copy link

tihmas commented Apr 7, 2022

Is there a reason why duplicate names can't be treated the same way they are in the editor? E.g. when two scenes are open with the same name the editor shows their full path so you can differentiate them:

/library_one/jump
/library_two/jump

@fire-forge
Copy link
Contributor

As mentioned above, the possibility of conflicts (two libraries having the same animation) still exists, so the editor will make its best effort to let you know about it. Still, the first animation (based on alphabetical order of the libraries) will be used, the second will be ignored

One solution to this is to allow using a NodePath-like syntax for identifying animations, like this: "AnimationLibrary:Animation". This could be made optional so that it doesn't complicate normal usage of animations. If the animation string doesn't include a :, it will fall back to the current behavior of searching for the first animation in alphabetical order.

@kennethrapp
Copy link

kennethrapp commented Apr 7, 2022

As mentioned above, the possibility of conflicts (two libraries having the same animation) still exists, so the editor will make its best effort to let you know about it. Still, the first animation (based on alphabetical order of the libraries) will be used, the second will be ignored

One solution to this is to allow using a NodePath-like syntax for identifying animations, like this: "AnimationLibrary:Animation". This could be made optional so that it doesn't complicate normal usage of animations. If the animation string doesn't include a :, it will fall back to the current behavior of searching for the first animation in alphabetical order.

Or make libraries Nodes (which have to be children of an AnimationPlayer) and move the play method to libraries, keeping AnimationPlayer.play() as a proxy for the play method for the default library. Then you could just use the normal path syntax and it's always explicit whether you want to use a library or not:

$AnimationPlayer/player_lib.play("walk") vs

$AnimationPlayer.play("walk")

@RPicster
Copy link
Contributor

RPicster commented Apr 7, 2022

@RPicster If you don't really want to use the libraries, you don't use them and it just works the same as before (everything is put into the "default" library without you doing anything). This is majorly optional for those who want to take advantage of it.

But when I have ten different things using the animation name "explosion", I would run into the duplicate names issue from my understanding?

@reduz
Copy link
Member Author

reduz commented Apr 7, 2022

@RPicster the libraries are local to the player, they are not global, so I doubt you will have 10 explosions in the same player.

@reduz
Copy link
Member Author

reduz commented Apr 7, 2022

@fire-forge @kennethrapp I already considered something like this, the problem is that, for the most part users are not forced to use libraries, so since you now have to always use the libraries it may get a bit difficult.

An alternative could be to assume you have a default animation library ("") so those are compatible with the current way of using it (just the name) but if the library has a name, you have to supply it, like libname/animation.

Would this work better?

@fire-forge
Copy link
Contributor

fire-forge commented Apr 7, 2022

@reduz Well, then the library name could be optional. If they user adds the [AnimationLibrary name]/ prefix to the animation name, it will know which library to look in. Otherwise, it defaults to the current behavior of searching all libraries in alphabetical order.

For example, if you have multiple libraries with a "walk" animation, you can tell it which one to use:

animation_player.play("library_name/walk")

If you only have 1 library with a "walk" animation, you can use the same syntax that's currently used:

animation_player.play("walk")

That way, nothing is changed for most users who are using only one AnimationLibrary per AnimationPlayer, but it gives more control to those who use multiple libraries.

@reduz
Copy link
Member Author

reduz commented Apr 7, 2022

Lets do this then, I will make it so the "" library is the global one, and the others you will access as library/. If you don't want to use libraries, then its assumed that you want to use the global one automatically, otherwise you have to specify the name. This sounds like it should solve the problem, is more elegant and there should be no collisions.

I will work on doing the changes.

@TokageItLab
Copy link
Member

TokageItLab commented Apr 8, 2022

BTW, I think the breaks compat label is not right. At least old scenes can be opened and the Animations are just grouped in the GlobalAnimationLibrary.

@reduz reduz force-pushed the animation-libraries branch from 854d999 to ada950d Compare April 8, 2022 19:18
@reduz
Copy link
Member Author

reduz commented Apr 8, 2022

Alright, should be finally good to merge now.

@reduz reduz force-pushed the animation-libraries branch from ada950d to 798dfea Compare April 10, 2022 08:59
Copy link
Member

@fire fire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I couldn't find major issues. Posted some small issues. The major reviewer is tokage and he found some crashes earlier. I need to see it in a system to test further.

@reduz
Copy link
Member Author

reduz commented Apr 11, 2022

@fire The crashes he found should be solved now

@TokageItLab
Copy link
Member

I have confirmed that the crash has already been fixed, so unless there are other problems, I think this can be merged.

@Deozaan
Copy link

Deozaan commented Apr 11, 2022

Are there any plans to backport this feature to 3.x?

doc/classes/OptionButton.xml Outdated Show resolved Hide resolved
doc/classes/Tree.xml Outdated Show resolved Hide resolved
editor/import/resource_importer_scene.cpp Show resolved Hide resolved
editor/plugins/animation_library_editor.cpp Outdated Show resolved Hide resolved
editor/plugins/animation_library_editor.cpp Outdated Show resolved Hide resolved
editor/plugins/animation_library_editor.h Outdated Show resolved Hide resolved
editor/plugins/animation_player_editor_plugin.cpp Outdated Show resolved Hide resolved
editor/plugins/animation_player_editor_plugin.cpp Outdated Show resolved Hide resolved
scene/animation/animation_player.cpp Outdated Show resolved Hide resolved
scene/resources/animation_library.cpp Outdated Show resolved Hide resolved
@reduz reduz force-pushed the animation-libraries branch 3 times, most recently from bde5dbe to a4369cb Compare April 11, 2022 10:27
* Instead of containing single animations, AnimationPlayer now contains libraries.
* Libraries, in turn, contain the animations.

This paves the way for implementing the possibility of importing scenes as animation libraries, finally allowing to import animations separate from the 3D models.

Missing (will be done on separate PRs):

* Make it possible to import scenes (dae/fbx/gltf) as animation libraries.
* Make it possible for AnimationTree to import animation libraries on its own, so it does not rely on AnimationPlayer for everything.
@reduz reduz force-pushed the animation-libraries branch from a4369cb to 6f40143 Compare April 11, 2022 10:52
@akien-mga akien-mga merged commit 4ab86c6 into godotengine:master Apr 11, 2022
@akien-mga
Copy link
Member

Thanks!

@fire
Copy link
Member

fire commented Apr 11, 2022

@Deozaan there are no plans because this work is a part of animation retargeting proposal and to achieve that we've had to significantly change Godot Engine's animation systems.

That said, if someone does the work and figures out that the 3.x changes don't break compatibility, maybe it'll be reviewable, but we haven't estimated the work.

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

Successfully merging this pull request may close these issues.