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

Remove differences of the code between old AnimationTree and AnimationMixer #85794

Merged
merged 1 commit into from
Dec 11, 2023

Conversation

TokageItLab
Copy link
Member

@TokageItLab TokageItLab commented Dec 5, 2023

A few lines were overlooked during those porting, sorry. I think the problem only occurred in complex use cases.

It would be helpful if someone could test on other complex trees besides the above issues.

@TokageItLab TokageItLab added bug regression topic:animation cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels Dec 5, 2023
@TokageItLab TokageItLab added this to the 4.3 milestone Dec 5, 2023
@TokageItLab TokageItLab requested a review from a team as a code owner December 5, 2023 17:35
@TokageItLab TokageItLab changed the title Remove differences of the code between old AnimationTree and Mixer Remove differences of the code between old AnimationTree and AnimationMixer Dec 5, 2023
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.

p_test_only does seem like dead code. The changes here suggest that it is always false. Only did a github review and not via a comprehensive test suit.

Edited:

It's a bit tricky to understand though. Do you have a test case where it fails? One of the attached issues mentioned that this fixes things by not creating empty nodes.

@TokageItLab
Copy link
Member Author

TokageItLab commented Dec 7, 2023

test_only is currently dead code unintentionally, but in 4.1 it works to process the AnimationNode virtually for estimating remaining time; This PR activate and revive that. However, test_only is used only complex nested/root StateMachines, it is difficult to cover test cases.

Comment on lines +568 to +573
root_animation_node->_pre_process(&process_state, pi, false);
started = false;
} else {
pi.time = p_delta;
root_animation_node->_pre_process(&process_state, pi);
}
pi.seeked = false;
pi.time = p_delta;
root_animation_node->_pre_process(&process_state, pi, false);
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we supposed to call pre-process (and thus process) twice here when started is true?

Copy link
Member Author

Choose a reason for hiding this comment

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

Before 4.1, it handled both the current position immediately after playback and the delta afterwards, so the purpose is to revert the behavior for now. If there is a problem, it should already be a problem with AnimationTree in 4.1, so this probably won't be a problem.

@akien-mga akien-mga merged commit e1d4b3c into godotengine:master Dec 11, 2023
15 checks passed
@akien-mga
Copy link
Member

akien-mga commented Dec 11, 2023

Thanks! :D

@YuriSizov
Copy link
Contributor

Given that #86644 addresses my concern from one of the changes here, I think they should be cherry-picked together. So setting this aside for now since #86644 is not merged yet.

@TokageItLab TokageItLab deleted the stablemixer branch February 14, 2024 05:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release regression topic:animation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AnimationNodeStateMachinePlayback.travel() gets stuck for nested state machines
4 participants