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

Allocate unique track_blends vector for animation states #51448

Conversation

SaracenOne
Copy link
Member

@SaracenOne SaracenOne commented Aug 9, 2021

This is a fix for the bug logged here (#51408). The only downside is that the fix introduces an extra memory allocation since it know longer uses the pointers to AnimationNode blends directly in order to prevent memory being overwritten on instanced animation state machines. There are ways this could be cached, so requesting feedback for profiling and alternative suggestions on how to fix.

Bugsquad edit: Fixes #51408.

@akien-mga
Copy link
Member

Needs a rebase to solve merge conflicts (and that should remove the unwanted merge commit too).

@reduz
Copy link
Member

reduz commented Aug 10, 2021

Animation resources are meant to be reused by different players, so they should not contain state data. Eventually also, the idea is to process different players in different threads, so my plan was to create a global state pointer using TLS or something.

@SaracenOne
Copy link
Member Author

Hang on, let me squash the commits...

@SaracenOne SaracenOne force-pushed the animation_tree_parallel_state_machine_fix branch from 3fe3331 to 68ca133 Compare August 10, 2021 13:33
@SaracenOne
Copy link
Member Author

Okay, squashed back into a single commit. I can backport this to 3.x too if you want. I like the notion of threading the AnimationTree, never really thought much about it, but it makes a lot of sense.

@reduz
Copy link
Member

reduz commented Aug 10, 2021

What I meant mostly is that you should not store permanent data in these resources.

@SaracenOne
Copy link
Member Author

I agree, this I'm basically treating this as a quick and dirty fix to this specific bug caused by overwritten track_blends. I realise it doesn't fully solve the underlying problem, especially if the plan is fully thread the tree.

@YuriSizov
Copy link
Contributor

Is this "quick and dirty fix" from over a year ago still relevant? :)

@YuriSizov YuriSizov modified the milestones: 4.0, 4.x Feb 16, 2023
@TokageItLab
Copy link
Member

TokageItLab commented Apr 25, 2023

I conclude that this PR is still valid/necessary for fixing #51408 bug. So, I want to merge this IMO.

However, since @reduz is considering multi-threading the animation blend calculation, we need to know if this will be a problem.


As far as I see it, this fix does not hold parameters in animation resource permanently, but rather avoids using references around during the blend calculation, so I guess this PR remove this concern.

reduz: What I meant mostly is that you should not store permanent data in these resources.

@SaracenOne SaracenOne force-pushed the animation_tree_parallel_state_machine_fix branch from 68ca133 to be10200 Compare May 16, 2023 16:30
@SaracenOne
Copy link
Member Author

@TokageItLab Okay, rebased now!

@SaracenOne SaracenOne force-pushed the animation_tree_parallel_state_machine_fix branch 2 times, most recently from 7747e80 to 9f1a8d5 Compare May 16, 2023 22:56
Copy link
Member

@TokageItLab TokageItLab left a comment

Choose a reason for hiding this comment

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

The bug in #51408 still exists and this PR will fix it.

Considering the possibility of AnimationNode reuse, this blend values should not be reused and should be rewritten each time the blend calculation. Since there is currently no better solution, I recommends that this fix be applied.

@TokageItLab TokageItLab requested a review from akien-mga May 17, 2023 10:01
@YuriSizov YuriSizov changed the title Allocate unique track_blends vector for animation states Allocate unique track_blends vector for animation states May 18, 2023
@YuriSizov YuriSizov modified the milestones: 4.x, 4.1 May 18, 2023
@YuriSizov YuriSizov removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label May 18, 2023
@YuriSizov
Copy link
Contributor

@SaracenOne Could you use this PR's title as your commit message title, and put the rest into the commit message body?

@SaracenOne SaracenOne force-pushed the animation_tree_parallel_state_machine_fix branch from 9f1a8d5 to 6d38e16 Compare May 18, 2023 19:01
Quick fix for a bug which occurs when blending the result of multiple instanced state machines outputting the same animation, but using filter tracks.
@SaracenOne SaracenOne force-pushed the animation_tree_parallel_state_machine_fix branch from 6d38e16 to 18c792f Compare May 18, 2023 19:03
@SaracenOne
Copy link
Member Author

Okay, updated now!

@YuriSizov YuriSizov merged commit 5bb7d58 into godotengine:master May 18, 2023
@YuriSizov
Copy link
Contributor

Thanks!

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.

Cannot blend the output of two animation state machines outputting the same animation using filter tracks
6 participants