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 AnimationMixer as a base class of AnimationPlayer and AnimationTree #80813

Merged
merged 1 commit into from
Sep 29, 2023

Conversation

TokageItLab
Copy link
Member

@TokageItLab TokageItLab commented Aug 20, 2023

Implement godotengine/godot-proposals#5972.

Currently AnimationPlayer and AnimationTree have separate duplicate implementations of playback and blending, so implement the base class to unify them.

Internally, this is a major change, but most compatibility will be kept.

However, Changes to conflicting method and property names between AnimationPlayer and AnimationTree are unavoidable, since retaining only one of them will destroy compatibility with the other.

  • AnimationPlayer.playback_active
  • AnimationTree.active
    • -> AnimationMixer.active (but not change the setter/getter method since AnimationPlayer has already have the AnimationPlayer.set_active() as setter of AnimationPlayer.playback_active)
  • AnimationPlayer.playback_process_mode
  • AnimationTree.process_callback
    • -> AnimationMixer.callback_mode_process
  • AnimationPlayer.method_call_mode
    • -> AnimationMixer.callback_mode_method (to follow above change to make group in the inspector)

Note: Old methods and property names will be kept for compatibility.

image

This change allows AnimationTree to have an AnimationLibrary. Also, when an AnimationTree is selected, a dummy AnimationPlayer is added to the editor internally.

image

If the AnimationTree specifies an AnimationPlayer, the dummy AnimationPlayer's resources will be referenced specified AnimationPlayer (make them read-only in AnimationTree).

image

Todo:


@TokageItLab TokageItLab added this to the 4.x milestone Aug 20, 2023
@TokageItLab TokageItLab changed the title Implement AnimationManager as base class of AnimationPlayer and AnimationTree Implement AnimationManager as a base class of AnimationPlayer and AnimationTree Aug 20, 2023
@TokageItLab TokageItLab force-pushed the rework-animation-manager branch 3 times, most recently from 009814c to ad35ee2 Compare August 20, 2023 11:16
@TokageItLab
Copy link
Member Author

TokageItLab commented Aug 20, 2023

For compatibility, the old setters and getters have been added. It means that some internal properties will have two property names, setters and getters.

@reduz has rejected this and suggested implementing separate property names, setters and getters for AnimationPlayer and AnimationTree, but I disagree strongly with that.

It makes the result in missing properties during interconversion between AnimationPlayer and AnimationTree. This causes problems with the "Change type" option of the SceneTreeDock and the creation of the DummyPlayer (these use set(prop.name, prop.value) iteratively). So we must have at least one shared property name.

For that, @reduz suggested putting an option in AnimationPlayerEditor, but honestly it's confusing and there is no need to ignore SceneTreeDock.

Having two property names, setters and getters is certainly redundant, but I argue that redundancy is far better than going the inconvenient way and having a method that only those who have read the manuals can understand.

Edited:
On Rocket chat we agreed on redundancy for this case.

@YuriSizov
Copy link
Contributor

YuriSizov commented Sep 5, 2023

Just to be clear, this is still a breaking compatibility change because the inheritance chain is modified. This may or may not be an issue for 3rd party language bindings. And we may or may not accept it as allowed.

@RandomShaper
Copy link
Member

After rebasing #80939 on top of this, onion skinning can no longer work because backing up and restoring animated values now requires a reset animation to be present. That didn't use to be a limitation. And, even if there happens to be a reset animation, I'm not sure if onion skinning would behave the same as it did. @TokageItLab, can you give me a hint on how could I capture the current animated values with the new API, to, say, create a bespoke reset animation for onion skinning purposes? Alternatively, I could avoid enabling onion skinning unless there's a reset animation present, but I'm not very fond of that.

@lyuma
Copy link
Contributor

lyuma commented Oct 2, 2023

@RandomShaper From the testing I did, I was generally able to use the Deterministic checkbox to control whether it follows the old rules for AnimationPlayer (keeping all existing animations) or the original rules for AnimationTree (restoring the animated values).

Would it be possible to test both Determinstic on and off and see if one has behavior closer to what you're looking for?

From what I skimmed, onion skinning uses AnimationPlayer, so I would expect Deterministic = off should work similar to before.

@TokageItLab
Copy link
Member Author

Onion skin was disabled as an experimental/broken feature during AnimationManager testing, so we could not confirm its correct behavior.

I remember that saw the reset/restore function used there, but I have not touched it so much.

I have rewritten the reset/restore function itself and changed the reset_on_save behavior, so the onion skin may need to be changed to follow/match that change. Would copy-pasting a line similar to the code I rewrote in the reset_on_save change be sufficient for it to work?

@RandomShaper
Copy link
Member

The problem is that my original backup/restore implementation was able to work without a reset animation and, as far as I can see, that is gone. I surely understand that happened because there was no (enabled) use case for it.

Now, for the time being, I'll have to make onion skinning require a reset animation. Hopefully someone (maybe myself) will be able to lift that requirement by bringing reset-less backup back.

@TokageItLab
Copy link
Member Author

TokageItLab commented Oct 3, 2023

What I remember for about reset_on_save is if there is no reset animation, I think it is made to use the initial (mostly zero) value to avoid updating the scene file. If you disable reset_on_save, does that mean that onion skin will work? And for onion skin, does it mean that reset_on_save is not available?

@RandomShaper
Copy link
Member

Unless it changed after I implemented it, reset-on-save would only work in AnimationPlayers having a reset animation. Without such an animation, the feature is no-op.

The problem with onion skinning is that it works like this:

  • Save all current values for every track and the current position (time).
  • For every step, seek to the relevant time, render and take screenshot.
  • Seek back to the original position and restore the backup.

It didn't require a reset animation, nor it it fitting for this purpose the way I envisioned it, because the current values in the animated properties may not even be the ones in the animation. For instance, say you have a Node2D animated such as its X position moves from 0 to 100 in some track. The current animation at the current time sets position to 50. The user drags the node manually to 15 and then enables onion skinning. After all the process involved (done on every frame), the user wants to still see the position is 15. The fact that seeking the AnimationPlayer is being used under the hood to capture the onion layers is kept hidden and unnoticed to the user.

With the reset animation there are a couple of downsides:

  1. Well, you need to have a proper reset animation involving all the values that you want to behave correctly in the onion layers.
  2. The onion skinning internal machinery is no longer hidden, since, the second you enable it, properties the user may have modified manually will be reset to the values mandated by the animation.

Neither is the end of the world, and I think that is good enough to bring onion skinning back to life. However, as I said, I'd like it to work as transparently (no pun intended) as it used to do in a future iteration.

@TokageItLab TokageItLab deleted the rework-animation-manager branch February 14, 2024 05:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment