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

Add parameter animation_override to AnimationNodeAnimation #87111

Closed

Conversation

ODtian
Copy link

@ODtian ODtian commented Jan 12, 2024

Add a parameter to override the animation of AnimationNodeAnimation. We can now change different animation for each AnimationNodeAnimation in each AnimationTree node while reuse animation tree resources.

A simple demo is attached: animation-test.zip

Demo gif preview:

animation override test

Related issue: godotengine/godot-proposals#8818

Add a parameter to override the animation of  `AnimationNodeAnimation`. We can now change different animation for each `AnimationTree` node while reuse animation tree resources.
@TokageItLab
Copy link
Member

TokageItLab commented Jan 12, 2024

I can't understand the necessity of this at all. Since NodeAnimation is used as the base for the AnimationTree time progression, it is not very safe to change its animation frequently. This would be like adding unnecessary danger.

If you really need this, a proposal is needed explaining why NodeTransition or NestedStateMachine (which can be used like AnyState) is not sufficient.

@ODtian
Copy link
Author

ODtian commented Jan 13, 2024

@TokageItLab

  • I'm not aware of time progression issue, it seems like node animation will only affect the animation it output, when I switch animation name in demo it just start from previous time. If I missed something, please tell me.
  • StateMachine is powerful but in some cases it brings huge complexity.
    • If you want to simply switch different animation in StateMachine, you are required to build a strongly connected complete graph. The scale of StateMachine will increase fast as the number of animations increase since E=V^2-V.
    • For example, if you want to switch between 10 animation, you will need 10 node in StateMachine, and then add 90 transitions as well as set then up.
    • Once you want to add another animation, you have to add 20 more transitions.

@TokageItLab
Copy link
Member

TokageItLab commented Jan 13, 2024

  • StateMachine is powerful but in some cases it brings huge complexity.
  • If you want to simply switch different animation in StateMachine, you are required to build a strongly connected complete graph. The scale of StateMachine will increase fast as the number of animations increase since E=V^2-V.
  • For example, if you want to switch between 10 animation, you will need 10 node in StateMachine, and then add 90 transitions as well as set then up.
  • Once you want to add another animation, you have to add 20 more transitions.

These are not exact.

A StateMachine works as a simple AnyState as long as it is in Nested mode, so if you have 10 animations, you just add 10 nodes. Also, since StateMachine can be saved as a resource, you can save it like AllAnimationListState.tres and simply copy it to any location.

Then, the only inconvenience is placing the 10 animations, so the addition an editor function that adds all the animations to the StateMachine would be sufficient like "Export AnyState as tres".


NodeAnimation returns remaining time and loop information, which affects all upstream AnimationNodes. It affects the logic, such as which branches are selected and which fades are handled.

NodeAnimation records the time of the previous frame for time calculation purposes, but a particular problem here is that time consistency can be severely corrupted when swapping animations of different lengths. This could be made better by implementing something like a Reset option like NodeTransition, but that would go beyond the role of NodeAnimation in the current state.

To begin with, NodeAnimation is designed to specify Animation as a role, so implementing a duplicate is not a good idea, not only for AnimationTree, but for architecture in general. There shouldn't have duplicate places to select Animation.

This implementation would have duplicate locations for selecting animations in NodeAnimation and AnimationTree. It means that animations selected in NodeAnimation could be overridden at any moment, which would complicate the project and confuse the tutorial. Especially it is the worst that the StateMachineEditor and BlendSpaceEditor do not show overridden values.

It might be bit more acceptable if the original animation selection were eliminated and replaced rather than overridden, like #76788, but at the very least, project compatibility must be ensured and the Editor's GUI must be changed.

However, that is too much to be exaggerated. It should be enough to have a function/extention that adds all the animations to StateMachineEditor as mentioned above.

@ODtian
Copy link
Author

ODtian commented Jan 13, 2024

  • In NodeAnimation process method, play data(time, loop state, backward, step and event to emit) are reculculated and clamped to right position every frame. I think there might be some unexpected behaviors but won't break the whole things up.

  • In case users want to change what will happen after switching an animation and know what they are doing, parameter time can be exposed for writing as well.

  • The idea of override comes from mesh instance, since material resources is nested in mesh resources, any change to material Will affect all mesh instance using the same resource.

  • Same issues here in AnimationTree, since AnimationNode are resources.

    • NodeAnimation's property animation acts as an default or fallback value, actual inner animation name will fallback on it if parameter override is not set. So nothing will change if user don't ever access override parameter.
    • We can't replace the property because NodeAnimation is a resource, once we do that it will change all AnimationTree's animations referring it.
    • Besides, the most common usecase of this function is changing animation at runtime, we may want the default value that is set to property animation, which can be edited in editor, therefore not showing the overridden one in editor may not be a big problem.
    • However, in case of some users do want to edit the override animation in editor and permanently save it in a scene file, we can expose it in editor panel just like other node, it won't be hard.

@TokageItLab
Copy link
Member

TokageItLab commented Jan 13, 2024

As I added to my comment above, this implementation is a terrible UI and will never be merged as is.

TokageItLab:

This implementation would have duplicate locations for selecting animations in NodeAnimation and AnimationTree. It means that animations selected in NodeAnimation could be overridden at any moment, which would complicate the project and confuse the tutorial. Especially it is the worst that the StateMachineEditor and BlendSpaceEditor do not show overridden values.

ODtian:

Besides, the most common usecase of this function is changing animation at runtime, we may want the default value that is set to property animation, which can be edited in editor, therefore not showing the overridden one in editor may not be a big problem.

This may not be confusing if you are developing a project alone, but can be a huge problem if someone are sharing a project with multiple people.


ODtian:

The idea of override comes from mesh instance, since material resources is nested in mesh resources, any change to material Will affect all mesh instance using the same resource.

MeshInstance's Material Override is implemented in consideration of possible reloading resource (to avoid to localize resource) and caching issues. Also it is relatively explicit in the GUI, but this Animation Override is not one of those cases.


Also, as long as StateMachine is used correctly, the complications you describe will not occur.

TokageItLab:

A StateMachine works as a simple AnyState as long as it is in Nested mode, so if you have 10 animations, you just add 10 nodes. Also, since StateMachine can be saved as a resource, you can save it like AllAnimationListState.tres and simply copy it to any location.

Then, the only inconvenience is placing the 10 animations, so the addition an editor function that adds all the animations to the StateMachine would be sufficient like "Export AnyState as tres".

So this PR should be superseded with the implementation of such an editor option.

@ODtian
Copy link
Author

ODtian commented Jan 13, 2024

Yes, traveling through teleport does fix this, closing the PR.

@ODtian ODtian closed this Jan 13, 2024
@AThousandShips AThousandShips removed this from the 4.x milestone Jan 13, 2024
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.

Add animation parameter to AnimationNodeAnimation
3 participants