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

Make AnimationNodeAdd actually additive 3.x #53893

Open
wants to merge 1 commit into
base: 3.x
Choose a base branch
from

Conversation

rafallus
Copy link
Contributor

@rafallus rafallus commented Oct 16, 2021

I took #42302 and did the following:

  • Adapt it to 3.x branch
  • Make it work with Animation::TYPE_VALUE tracks
  • Make it work with Animation::TYPE_BEZIER tracks
  • Consider input coming from AnimationNodeStateMachine
  • Consider input coming from AnimationNodeBlendSpaceXd
  • Make add_directly default to true

Animation::TYPE_TRANSFORM already implemented in original PR. Nothing done for Animation::TYPE_METHOD, Animation::TYPE_AUDIO and Animation::TYPE_ANIMATION tracks

Edit: Could fix #37661

@rafallus rafallus requested review from a team as code owners October 16, 2021 19:57
@rafallus
Copy link
Contributor Author

Using AnimationNodeStateMachine and AnimationNodeBlendSpaceXd as inputs to an AnimationNodeAdd2 needs more testing. I used very simple state machines and blend spaces and it worked, but need to check for more complex systems

@rafallus rafallus force-pushed the fix_animation_tree_node_add branch 5 times, most recently from 0ce1060 to bf8aec7 Compare October 16, 2021 21:46
@rafallus rafallus force-pushed the fix_animation_tree_node_add branch from bf8aec7 to a163d7e Compare October 16, 2021 22:00
@Calinou Calinou added this to the 3.5 milestone Oct 16, 2021
@akien-mga akien-mga force-pushed the 3.x branch 2 times, most recently from 71cb8d3 to c58391c Compare January 6, 2022 22:40
@darkconsole
Copy link

noticed this didn't make it into 3.5 - is it still going to be relevant for a 3.x milestone?

@Calinou
Copy link
Member

Calinou commented Jun 13, 2022

noticed this didn't make it into 3.5 - is it still going to be relevant for a 3.x milestone?

Yes, as a 3.6 version is planned.

@akien-mga akien-mga modified the milestones: 3.5, 3.x Jul 2, 2022
@akien-mga
Copy link
Member

@TokageItLab expressed some concerns over the solution in #42302 here: #37661 (comment)

@Riordan-DC
Copy link

A suggestion,
I wrote a different solution based on #42302

My only issue with the solution in #42302 is that if we want to add a subtraction mode down the road (which is commonly used with additive animation to create a delta animation) we would need to add another flag to blend_input. Adding the add_directly flag is good enough to ask process_graph to do something other than blending but ultimately the add_directly flag should become a enum for a blending mode (regular blending by default to preserve functionality).

I understand adding a new node to the animation tree will be its own PR but the change I suggest would make adding any new nodes easier in future.

An image showing the sort of workflow you might use Add2 in; with a new Sub2 node.
image

Snippet from my solution with existing behavior preserved and add direct and subtract modes implemented.
https://github.com/Riordan-DC/godot/blob/c98a9924842f9866ea1916544d3861f146e6f8ea/scene/animation/animation_tree.cpp#L938-L971

Reference information on additive and delta animations I used. Explains why a subtract node later will be necessary:
http://guillaumeblanc.github.io/ozz-animation/samples/additive/

I admit my solution is not perfect and I have an issue with how I am passing the enum down the animation tree. Without the hacky code any child blend nodes will override the mix mode enum I added and break the solution. I guess it is just kind of messy passing a variable down recursive function calls.
https://github.com/Riordan-DC/godot/blob/c98a9924842f9866ea1916544d3861f146e6f8ea/scene/animation/animation_tree.cpp#L147-L183

I hope this suggestion is relevant and helpful.

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.

6 participants