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

Rework blending method in Variant animation for Int/Array/String #84815

Merged
merged 1 commit into from
Nov 16, 2023

Conversation

TokageItLab
Copy link
Member

@TokageItLab TokageItLab commented Nov 12, 2023

Follow up #80813

Supersedes / Closes #66771
Closes godotengine/godot-proposals#5501

I wanted to include godotengine/godot-proposals#8085, but it would be a rather complicated implementation, so I will postpone that.

This PR allows for interpolating arrays. In most cases, this is intended for use with polygon animation, so if the arrays are of different lengths, interpolation is performed with the last element of the shorter array.

array_blending.mp4

Also, it allows different strings to be interpolated by treating the characters as numbers. This should provide a behavior similar to interpolating text keyframes in AfterEffects.

For about interpolating discrete Variables (e.g. Int, Rect2i, PackedInt64Array and etc.)

During blending, these Variants must continuous as float.

cast_to_blendwise() / cast_from_blendwise(), ensure that casts are performed before and after the blend calculation.

Interpolations that do not blend, such as Tween and Animation::seek(), are still temporarily transformed, but are refactored to use the above functions.

For about interpolating Strings which have partially same text with different length

interpolate_variant()

string_anim.mp4

interpolate_variant() (used by Tween and Animation::seek()) can only change the length between strings with partially same text. This is because the order is fixed and the priority string is determined.


blend_variant()

srting_blend.mp4

blend_variant() interpolates characters (same as with arrays, it will be interpolated with the last element of the shorter string) at the same time as length. This is because the order is not fixed and it is difficult to apply a priority order.

If the implementation of godotengine/godot-proposals#8085 is successful in the future, the priority order may be able to determine, but for now, I submit the current version as an experimental algorithm; when string blending occur, it show warning once.


BTW, there is a bug with reset on save for arrays, but it exists before this PR, so I will look into it separately.

Demo project (array_blending_r.zip)

@TokageItLab
Copy link
Member Author

Working on improving the String Tween (add/sub_varaint for Tween delta with String is currently problematic).

@TokageItLab
Copy link
Member Author

Alright, I fixed tween. I believe now it is ready for review.

text_bool_tween.mp4

array_blending_r.zip

scene/resources/animation.h Outdated Show resolved Hide resolved
Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

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

I did some tests with Tweens and it works correctly. I did not test animation blending nor any of the linked issues.

Animating emoji is interesting 🤔

godot.windows.editor.dev.x86_64_S9Sh2O1OxP.mp4

@TokageItLab TokageItLab force-pushed the array-animation branch 3 times, most recently from 25f52ae to 0910c78 Compare November 15, 2023 15:20
Copy link
Contributor

@YuriSizov YuriSizov left a comment

Choose a reason for hiding this comment

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

I read through changes and it all looks sensible to me. But I can't say if this is a correct or a complete solution to the problem.

Comment on lines +5893 to +5895
case Variant::PACKED_BYTE_ARRAY: {
// Skip.
} break;
Copy link
Member Author

@TokageItLab TokageItLab Nov 15, 2023

Choose a reason for hiding this comment

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

At last, I just added exception handling for PackedByteArray to eliminated the possibility of interpolation as PACKED_INT_ARRAY in interpolate_variant(), although I think that's a very rare case where the check would be needed.

@akien-mga
Copy link
Member

akien-mga commented Nov 16, 2023

Tested the MRPs from the linked issues.

simplescreenrecorder-2023-11-16_12.19.07.mp4

I'm not sure how useful the String blending shown in this PR's demo properly really is in practice, but it warns users that it's experimental so I think we can try it out and see if users are satisfied with it.

I don't know if this really solves everything properly but it seems to be an improvement and fixes a number of bug reports.

scene/animation/animation_mixer.cpp Outdated Show resolved Hide resolved
scene/animation/animation_mixer.cpp Outdated Show resolved Hide resolved
scene/animation/animation_mixer.cpp Outdated Show resolved Hide resolved
scene/animation/animation_mixer.cpp Outdated Show resolved Hide resolved
@TokageItLab
Copy link
Member Author

TokageItLab commented Nov 16, 2023

#73342: With the test project from #73342 (comment), it doesn't seem to work for me. I don't see any change when tweaking the blend value between the 3 animations/polygons. On the other hand I do see polygon blending working on the test project from this PR, so maybe it's a me issue or specific to that MRP.

This is because that MRP is using UpdateMode::Discrete. If you set UpdateMode::Continuous, the blend will work. Also If you loop the animation or connect a node with a playback function such as NodeOneShot/Transition, the key update of UpdateMode::Discrete will also work.

When changing the z_index blending to Continuous on the ArmRPlayer walk_left animation, I get this warning: WARNING: Value Track: '.:z_index' has different update modes between some animations may be blended. Blending prioritizes UpdateMode.UPDATE_CONTINUOUS, so the process treat UpdateMode.UPDATE_DISCRETE as UpdateMode.UPDATE_CONTINUOUS with InterpolationType.INTERPOLATION_NEAREST.
It's not clear to me what this means exactly, there's no AnimationTree/blending in that test, just one property using Continuous while other unrelated properties use Discrete. Is that really a situation that needs a warning?
The warning is a bit ambiguous, it's not clear what the user needs to do.

This was implemented at the same time as the AnimationMixer implementation. When everything is in DiscreteMode, the AnimationTree only updates the value when it crosses a key. But that MRP has Continuous so AnimationMixer enforce Continuous for that track, meaning that the value is updated even though it does not cross the key.

Also, AnimationMixer does not know when a blend will occur, so it iterates through the entire animation list once and generates a track cache. Changes to blend types are made at the time of cache generation, and warnings are output at the same time, which is why warnings are generated even if the AnimationTree or AnimationPlayer does not have them assigned if that tracks exist in any animation of animation list.

@akien-mga akien-mga modified the milestones: 4.3, 4.2 Nov 16, 2023
@akien-mga akien-mga merged commit 6ae6cc0 into godotengine:master Nov 16, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks for the quick turnaround fixing these issues!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment