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.

Sign up for GitHub

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

Jump to bottom

Fixed broken root motion calculation in internal process of AnimationBlendTree such as NodeOneShot #60774

Merged
merged 1 commit into from
May 18, 2022
Merged

Fixed broken root motion calculation in internal process of AnimationBlendTree such as NodeOneShot #60774

merged 1 commit into from
May 18, 2022

Conversation

TokageItLab
Copy link
Member

@TokageItLab TokageItLab commented May 5, 2022 •
edited
Loading

Fixed #40294.

The root motion is calculated by subtracting delta from the current time, but if the current time is start or end an error occurred because the clamped current time was passed. This is regression by #53819 (sorry it's my mistake). In addition, make NodeOneShot not fade by default, because it would be confusing to have NodeOneShot fade by default, which would change the value of the root motion.

Fixed godotengine/godot-proposals#4418.

In the case of an implicit seek performed by an internal process such as NodeOneShot, the root motion should not be calculated. Conversely, if the user explicitly seeks with NodeSeek, the root motion should be calculated.

If NodeSeek or NodePhase(#57959) supports loops, the p_seek_root may need to be an int (enum) instead of a bool, but for now, this PR fixes the unusable broken calculation of the root motion for NodeOneShot.

Before:

2022-05-05.11.03.03-1.mov

After:

2022-05-05.10.25.42-1.mov

In addition, this includes small fix of the enum notation in Animation.
Animation::LoopMode::LOOP_LINEAR -> Animation::LOOP_LINEAR

@TokageItLab TokageItLab added this to the 4.0 milestone May 5, 2022
@TokageItLab TokageItLab requested review from a team as code owners May 5, 2022 01:58
@TokageItLab TokageItLab changed the title Implement selection of whether or not to seek root motion Fixed broken calculation of the root motion in internal process of AnimationBlendTree such as NodeOneShot May 5, 2022
@TokageItLab TokageItLab changed the title Fixed broken calculation of the root motion in internal process of AnimationBlendTree such as NodeOneShot Fixed broken root motion calculation in internal process of AnimationBlendTree such as NodeOneShot May 5, 2022
@TokageItLab TokageItLab changed the title Fixed broken root motion calculation in internal process of AnimationBlendTree such as NodeOneShot Fixed broken root motion calculation in internal process of AnimationBlendTree such as NodeOneShot May 5, 2022
@TokageItLab TokageItLab force-pushed the root-seek-mode branch 3 times, most recently from dddc0ba to 9c627b9 Compare May 8, 2022 22:52
@TokageItLab TokageItLab marked this pull request as draft May 8, 2022 22:52
@TokageItLab TokageItLab marked this pull request as ready for review May 8, 2022 22:58
@TokageItLab
Copy link
Member Author

TokageItLab commented May 8, 2022 •
edited
Loading

I found some errors and fixed them additionally. Ready to review.

@TokageItLab TokageItLab mentioned this pull request May 8, 2022
Closed
@TokageItLab TokageItLab force-pushed the root-seek-mode branch 2 times, most recently from 46bf30b to 5c80eda Compare May 8, 2022 23:26
TokageItLab
TokageItLab commented May 8, 2022
View reviewed changes
scene/animation/animation_blend_tree.cpp
time = 0;
} else if (time > anim_size) {
step += anim_size - time;
Copy link
Member Author

Choose a reason for hiding this comment

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

Adjusted by the overflow value.

scene/animation/animation_tree.cpp
@@ -1057,7 +1058,7 @@ void AnimationTree::_process_graph(double p_delta) {
}
a->position_track_interpolate(i, 0, &loc[1]);
t->loc += (loc[1] - loc[0]) * blend;
prev_time = 0;
prev_time = (double)a->get_length();
Copy link
Member Author

@TokageItLab TokageItLab May 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Fixed the simple bug which is broken backward root motion loop.

TokageItLab
TokageItLab commented May 8, 2022
View reviewed changes
scene/animation/animation_tree.cpp
@@ -967,6 +967,7 @@ void AnimationTree::_process_graph(double p_delta) {
int pingponged = as.pingponged;
#ifndef _3D_DISABLED
bool backward = signbit(delta);
bool calc_root = !seeked || as.seek_root;
Copy link
Member Author

Choose a reason for hiding this comment

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

AnimationTree performs a seek to change the playback time, but should be able to choose whether to calculate the root motion or not.

@TokageItLab
Copy link
Member Author

I left a few comments to make the review easier. The other parts of these sections are argument rewrites and style corrections.

@TokageItLab TokageItLab mentioned this pull request May 9, 2022
Closed
@akien-mga akien-mga merged commit 5b3d596 into godotengine:master May 18, 2022
@akien-mga
Copy link
Member

Thanks!

@TokageItLab TokageItLab mentioned this pull request May 18, 2022
Closed
@TokageItLab TokageItLab deleted the root-seek-mode branch May 23, 2022 15:18
@Valeryn4
Copy link
Contributor

Valeryn4 commented Jun 5, 2022 •
edited
Loading

#58061 - not fixed
the problem is not fixed. The necessary global transformation of the root-bone.

problem actual in 4.0, last version!
rotation no sync!
see video 2:00 time: https://youtu.be/Z78KnTzFswg

im use:

var tr2: Transform3D = _animation_tree.get_root_motion_transform() 
_core.transform.basis *= tr2.basis.orthonormalized()

i rotate the model using root_motion_transform.
But I can't get the absolute transformation of the main bone, which causes the animation to shift over time.

@TokageItLab
Copy link
Member Author

TokageItLab commented Jun 5, 2022 •
edited
Loading

@Valeryn4 I commented in #29458 (comment).

@TokageItLab TokageItLab mentioned this pull request Sep 15, 2022
Closed
@TokageItLab TokageItLab mentioned this pull request Nov 26, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@reduz reduz reduz approved these changes

Assignees
No one assigned
Labels
bug topic:animation topic:3d
Projects
None yet
Milestone
4.0
4 participants
@TokageItLab @akien-mga @Valeryn4 @reduz