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

Fix issues with BlendSpace2D BLEND_MODE_DISCRETE_CARRY #48375

Merged
merged 1 commit into from
Jul 13, 2021
Merged

Fix issues with BlendSpace2D BLEND_MODE_DISCRETE_CARRY #48375

merged 1 commit into from
Jul 13, 2021

Conversation

rune-scape
Copy link
Contributor

@rune-scape rune-scape commented May 2, 2021 •
edited by akien-mga
Loading

bug fix for #41028. introduced in #20135.
I left my notes on what the problems were in #41028

Bugsquad edit: Fixes #41028.

@rune-scape rune-scape requested a review from a team as a code owner May 2, 2021 03:39
@Calinou Calinou added bug topic:core cherrypick:3.3 cherrypick:3.x Considered for cherry-picking into a future 3.x release labels May 2, 2021
@Calinou Calinou added this to the 4.0 milestone May 2, 2021
@fire
Copy link
Member

fire commented May 2, 2021

I asked @TokageItLab to review.

fire
fire approved these changes May 2, 2021
View reviewed changes
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.

@TokageItLab 's review said this also affects 3d animations.

He says the pr fixes the stated problem.

@TokageItLab
Copy link
Member

TokageItLab commented May 2, 2021 •
edited
Loading

2021-05-03_4.33.18.mov

blendspace2d_blendmode_test_with_3d.zip

This video blends three same animations, but it is wrong for such a transition to occur in BLEND_MODE_DISCRETE_CARRY.　BLEND_MODE_DISCRETE_CARRY is the process of seeking a newly prepared animation. The problem is that it does not inherit the time correctly. This PR fixes that problem, so I am in favor of it.

Apart from this, BlendSpace2D has a sync problem, but at least in a situation where there are three same animations, the preview in the Editor starts playing at the same time, so the sync problem does not occur. (If the animations are out of sync, turn on/off the Active checkbox in AnimationTree.) Moreover, the sync problem has nothing to do with this problem in the first place, and is not an issue when time is inherited correctly. So I concluded that the problem of glitchy transitions is due to wrong time inheritance.

One fear I have is that the BLEND_MODE_DISCRETE_CARRY feature itself is ambiguous in BlendSpace2D without sync. If a point in a BlendSpace2D calls a NodeAnimation with an animation of the same length, it will mostly behave as expected, but if the point calls a NodeBlendTree, the notion of animation length and elapsed time will not work because multiple animations are referenced, and the seek will not be correct. There is also a question about the blending behavior of NodeAnimations with animations of different lengths. This problem may be solved in the future if BlendSpace2D has sync, but I think it has to be noted in the documentation.

@akien-mga akien-mga requested a review from reduz May 3, 2021 13:37
@akien-mga akien-mga changed the title discrete carry bug fix Fix issues with BlendSpace2D BLEND_MODE_DISCRETE_CARRY May 5, 2021
@akien-mga
Copy link
Member

Could you amend the commit message to be more explicit about what it does? Referencing a GitHub issue is useful, but not sufficient information to know what the commit does when not on GitHub.

@rune-scape
Fixes BlendSpace2D BLEND_MODE_DISCRETE_CARRY.
bcb1e2b 
When BlendSpace2D switches animations, it will now correctly
calculate the previous animation position and length and
apply is to the new animation.
@rune-scape
Copy link
Contributor Author

done

fire
fire approved these changes Jun 28, 2021
View reviewed changes
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.

Didn't want this to be in limbo.

Already approved earlier.

@fire fire mentioned this pull request Jun 28, 2021
Open
@akien-mga akien-mga merged commit f2ad067 into godotengine:master Jul 13, 2021
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

@akien-mga
Copy link
Member

Cherry-picked for 3.4.

@akien-mga akien-mga removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Jul 13, 2021
@akien-mga
Copy link
Member

I prefer not to cherry-pick for 3.3 at this stage.

This was referenced Jan 9, 2024
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fire fire fire approved these changes

@reduz reduz Awaiting requested review from reduz

Assignees
No one assigned
Labels
bug topic:animation
Projects
None yet
Milestone
4.0
Development

Successfully merging this pull request may close these issues.

Blendspace2D BLEND_MODE_DISCRETE_CARRY freezes or doesn't carry
5 participants
@rune-scape @fire @TokageItLab @akien-mga @Calinou