Allow Tween to accept Arrays & Packed*Arrays #66771

Mickeon · 2022-10-02T16:27:13Z

See this discussion godotengine/godot-proposals#5501.

This PR allows Arrays, but most especially Packed*Arrays to be tweened with a single tween_property() call. For example:

PackedColorArray	PackedVector2Array

PackedStringArray

Interpolation fails if the starting array and the new array do not have the same size (this behavior can be further discussed).

By extension, it also fixes a crash that would occur when passing any Packed*Array.
It seemed like the internal Animation.interpolate_variant() had partial support for interpolating these arrays, already. There just was never any occasions to put the gears to work, so to speak. In fact, the previous casting was also kind of botched, as it would simply return nothing and later crash.

TokageItLab · 2022-10-02T18:32:57Z

Animation::interpolate_variant() already supports PackedArrays Not only Color, but also some types. You can apply it to other calc methods for variant with that as a reference.

Mickeon · 2022-10-02T18:42:10Z

@TokageItLab Let me reiterate, the previous support in interpolate_variant() seemed to be flawed, at least to me. The Variant casting would fail, returning a NUL pointer, and the engine would subsequently crash. Copying and pasting similar code to all of the functions necessary for this has previously lead to the same result.

TokageItLab · 2022-10-02T19:10:43Z

Fixing bugs is fine, but I would like to see it applied to more than just Color for consistency.

Mickeon · 2022-10-02T19:20:26Z

Yep, indeed, of course, it's a draft for that reason.

Halfmania33 · 2022-10-02T19:27:12Z

@Mickeon Oh what a good surprise !! Thank you for trying to implement this feature ! I'm looking forward to seeing the end result !!!

Mickeon · 2022-10-03T14:45:46Z

Updated this PR to support all Packed*Arrays and even Arrays, as well. However, I really, really need a technical review, if that may asked, because I am afraid that the way I avoided to write duplicate code, by converting every Packed*Array, is way too slow.

I tried using templates, and that would work, actually, but it'd still duplicate code in a way that felt unnecessarily bloated to me.

To keep it in mind, open this to see the alternative method.

if (a.is_array() && a.get_type() != Variant::ARRAY) {
switch (a.get_type()) {
	case Variant::PACKED_COLOR_ARRAY: {
		return interpolate_packed_array(PackedColorArray(a), PackedColorArray(b), c);
	}
	case Variant::PACKED_VECTOR2_ARRAY: {
		return interpolate_packed_array(PackedVector2Array(a), PackedVector2Array(b), c);
	}
}
				
template <class T>
Vector<T> Animation::interpolate_packed_array(Vector<T> a, Vector<T> b, float c) {
	int size = a.size();
	//WARN_PRINT(vformat("%d, %d", a.size(), b.size())); 
	if (size == 0 || b.size() != size) {
		return a;
	} else {
		Vector<T> result;
		result.resize(size);

		T *result_ptr = result.ptrw();
		const T *a_ptr = a.ptr();
		const T *b_ptr = b.ptr();

		for (int i = 0; i < size; i++) {
			result_ptr[i] = interpolate_variant(a_ptr[i], b_ptr[i], c);
		}
		//WARN_PRINT(vformat("%s, %s", a[0], result[0]));
		return result;
	}
}

TokageItLab · 2022-10-03T20:15:16Z

That implementation would result in processing a large number of unnecessary switch/case statements (but I understand that this is better than the current status that requires a for loop externally), so other approaches seem better IMO. Are @akien-mga or @reduz familiar with this area?

kleonc · 2022-10-03T22:01:17Z

I think the current impementation is not really acceptable, there should be no conversions when not needed.

When interpolating packed arrays of the same type the result should be of the same type and there should be no conversions to generic arrays whatsoever (it's inefficient).

Then there's a question what should be the behavior for e.g. PackedInt64Array and PackedFloat64Array args as there's this part in Animation::interpolate_variant allowing interpolating any number types:

godot/scene/resources/animation.cpp

Lines 5742 to 5750 in 1371a97

    
           Variant Animation::interpolate_variant(const Variant &a, const Variant &b, float c) { 
        
           	if (a.get_type() != b.get_type()) { 
        
           		if (a.is_num() && b.is_num()) { 
        
           			real_t va = a; 
        
           			real_t vb = b; 
        
           			return va + (vb - va) * c; 
        
           		} 
        
           		return a; 
        
           	}

Also note that currently there's no simliar parts in Animation::add_variant/Animation::subtract_variant so this inconsistency should probably be addressed too (probably out of this PR's scope):

godot/scene/resources/animation.cpp

Lines 5563 to 5566 in 1371a97

    
           Variant Animation::add_variant(const Variant &a, const Variant &b) { 
        
           	if (a.get_type() != b.get_type()) { 
        
           		return a; 
        
           	}

godot/scene/resources/animation.cpp

Lines 5610 to 5613 in 1371a97

    
           Variant Animation::subtract_variant(const Variant &a, const Variant &b) { 
        
           	if (a.get_type() != b.get_type()) { 
        
           		return a; 
        
           	}

Then typed arrays should also be respected. When interpolating e.g. two Array[int] (TypedArray<int64_t>) I'd expect the result to also be such typed array.
Then for two generic Arrays something like the current implementation.
Then if the array types mismatch (potentially besides the case 2. from the list above) maybe it should convert to generic Arrays like in the current implementation. Or maybe it should be disallowed. Not sure.

Regarding the code duplication I'd say in this specific case (arrays so potentially big chunks of data) the execution speed should definitely be prioritized. So yes for reducing the code duplication but not if it would hurt the performance.

Mickeon · 2022-10-03T23:01:32Z

Thank you very much for your insightful feedback, it was very necessary!

Then there's a question what should be the behavior for e.g. PackedInt64Array and PackedFloat64Array args as there's this part in Animation::interpolate_variant allowing interpolating any number types [...]

Conversion between float and int is pretty common everywhere, but I feel like for how "hardcoded" all Packed Arrays are, that there should be no subtle casting here. Casting between PackedInt64Array and PackedInt32Array within the Animation functions feels like asking for trouble, and may even warrant a more noticeable error so that the user can handle it more graciously.

Then if the array types mismatch (potentially besides the case 2. from the list above) maybe it should convert to generic Arrays like in the current implementation. Or maybe it should be disallowed. Not sure.

It's niche, but it avoids some annoyance. It shouldn't be too difficult to make it work between TypedArrays of float and int, at least for now, as that would be consistent across the engine.

Mickeon · 2022-10-04T20:44:17Z

Updated the PR following the suggestions above.

2. Also note that currently there's no simliar parts in Animation::add_variant/Animation::subtract_variant so this inconsistency should probably be addressed too (probably out of this PR's scope):

This had to be implemented here, otherwise interpolating between [0.5, 2.75] and [1, 0] would not work as expected, for example.

If the starting and final arrays have different sizes, tweening will not happen. If the arrays are typed and they're different, tweening will not happen, except between floats and ints.

scene/resources/animation.cpp

TokageItLab · 2022-10-11T20:38:16Z

scene/resources/animation.cpp

+
+						Array result;
+
+						if (arr_a.is_typed() && arr_b.is_typed()) {


if (!arr_a.is_typed() || !arr_b.is_typed()) { return a; }

is maybe better.

What would the suggestion do?

It just recommends an earlier return for non-typed arrays.

The idea was to allow interpolation of non-typed Arrays, so long as a[i] and b[i] are of the same type

We need to think about it a bit to see if it will work and if it will be the correct casting. What do you think? @reduz @kleonc @KoBeWi

So what happens when a[i] and b[i] have different type? Are they skipped? Is there an error?
I think it's ok to support untyped arrays if it doesn't affect performance of the typed ones.

The current behavior of this PR is that, if a[i] and b[i] are different types they are skipped altogether.

However, as I come back to this PR, I realise that these methods are also generally used by AnimationPlayer. So, I'm not sure if the current implementation break or introduce some odd and unnecessary code smells, right now.

TokageItLab · 2022-10-11T20:48:41Z

scene/resources/animation.cpp

+						int size = arr_a.size();
+						if (size == 0 || arr_b.size() != size) {
+							return a;
+						}


int size = MAX(arr_a.size(), arr_b.size());

Shouldn't we consider the maximum size? It seems that at least in blend_packed_array() the maximum size should be considered if you implement it.

If maximum size is used, it would go out of bounds when operating on one of the two arrays when the sizes do not match. If anything, it could be MIN(), and anything above that index is ignored.

You can use initial values for elements out of bounds. Then if resize() is properly done, access out of bounds will not occur.

For example, if we do the following tween:

[1, 2] -> [2, 4, 2] -> [2, 1]

The result would be as follows:

When using MIN: [1, 2] -> [2, 4] -> [2, 1]
When using MAX: [1, 2] -> [2, 4, 2] -> [2, 1, 0]

If you use MIN, the final result may be smaller than the element in the middle of the tween, so someone may access out of bounds unless they know after the tween is over that it is smaller than the element in the middle of the tween. So it is safer to use MAX.

I do understand your theoretical implementation. I'm just not sure it would be expected behavior to make a tweener capable of resizing the Array it ends up returning, on the first frame the tween starts.

At least interpolate_variant() implicitly resizes the array for PackedStringArray.

I tried interpolating polygon from the default color array to another array and the result was that the value jumped to the final value at the end of animation. Would using MAX handle this more gracefully?

TokageItLab · 2022-10-11T20:51:05Z

scene/resources/animation.h

+	template <class T>
+	static Vector<T> add_packed_array(Vector<T> a, Vector<T> b);
+	template <class T>
+	static Vector<T> subtract_packed_array(Vector<T> a, Vector<T> b);
+	template <class T>
+	static Vector<T> interpolate_packed_array(Vector<T> a, Vector<T> b, float c);
+


static Vector<T> blend_packed_array(Vector<T> a, Vector<T> b, float c);

Could we also add a blend_packed_array() for consistency?

I could, although this PR is specifically for supporting tweening the Packed Arrays, which doesn't technically need the method.

TokageItLab · 2023-02-15T10:14:19Z

@Mickeon What is the progress on this? Also, we currently do not handle Array types in blend_variant() (#73342) and it would be worth brushing up and merging this.

Halfmania33 · 2023-06-12T17:20:04Z

Hello Hardworkers !

I come back to check if there are somes good news.
Is there some hope about implementing this feature in 4.1 ? And backporting it to 3.X ?

Calinou · 2023-06-12T17:28:37Z

Is there some hope about implementing this feature in 4.1 ? And backporting it to 3.X ?

4.1 is in feature freeze, so any new feature development can only be merged in 4.2 at the earliest. A 3.x backport for 3.6 is feasible, but the feature needs to be merged in master first before it can be merged in 3.x.

Also, this PR needs additional work before it can be merged.

TokageItLab · 2023-10-15T15:50:43Z

As we mentions in #73342 and #83384, this issue came to the surface after implement AnimationMixer (#80813), so I will try to salvage this PR later.

Godot 4.2 features have been frozen and @Calinou said this is a new feature. However, I believe there is no problem to include it in 4.2 as a bug fix as long as during beta, since there is no danger for breaking compatibility. Considering #83384, this is just a fix for regression.

akien-mga · 2023-11-14T22:49:25Z

Superseded by #84815.

KoBeWi added enhancement topic:animation labels Oct 2, 2022

KoBeWi added this to the 4.x milestone Oct 2, 2022

TokageItLab requested a review from a team October 2, 2022 19:33

Mickeon force-pushed the tween-packed-arrays branch from a05dbd5 to bef86fe Compare October 3, 2022 14:45

Mickeon changed the title ~~Allow Tween to accept Packed*Arrays~~ Allow Tween to accept Arrays & Packed*Arrays Oct 3, 2022

Mickeon marked this pull request as ready for review October 3, 2022 14:46

Mickeon force-pushed the tween-packed-arrays branch from bef86fe to bddefdf Compare October 3, 2022 14:57

Mickeon force-pushed the tween-packed-arrays branch from bddefdf to 79cc84b Compare October 4, 2022 20:42

Mickeon force-pushed the tween-packed-arrays branch from 79cc84b to e9dce9f Compare October 4, 2022 20:47

Allow Tween to accept Array & Packed*Arrays

ad402af

If the starting and final arrays have different sizes, tweening will not happen. If the arrays are typed and they're different, tweening will not happen, except between floats and ints.

Mickeon force-pushed the tween-packed-arrays branch from e9dce9f to ad402af Compare October 4, 2022 20:49

akien-mga requested review from KoBeWi and TokageItLab October 11, 2022 12:43

TokageItLab assigned TokageItLab and unassigned TokageItLab Oct 11, 2022

TokageItLab requested changes Oct 11, 2022

View reviewed changes

kleonc mentioned this pull request Nov 26, 2022

PackedVector2Array reset when animated with cubic interpolation #69188

Open

TokageItLab modified the milestones: 4.x, 4.1 Dec 13, 2022

TokageItLab mentioned this pull request Feb 15, 2023

Animation::blend_variant() can't handle Array type #73342

Closed

TokageItLab added the salvageable label Jun 1, 2023

YuriSizov modified the milestones: 4.1, 4.x Jun 14, 2023

KnightNine mentioned this pull request Jun 30, 2023

t->value within animation_tree.cpp is always null for Packed Array Types if without RESET track. #78869

Closed

TokageItLab mentioned this pull request Oct 24, 2023

Continuous animation of Polygon2D points no longer works. (Animation Player) #83841

Closed

TokageItLab mentioned this pull request Nov 12, 2023

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

Merged

akien-mga closed this Nov 14, 2023

akien-mga added archived and removed salvageable labels Nov 14, 2023

AThousandShips removed this from the 4.x milestone Nov 15, 2023

Mickeon deleted the tween-packed-arrays branch December 30, 2023 11:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow Tween to accept Arrays & Packed*Arrays #66771

Allow Tween to accept Arrays & Packed*Arrays #66771

Mickeon commented Oct 2, 2022 •

edited

Loading

TokageItLab commented Oct 2, 2022 •

edited

Loading

Mickeon commented Oct 2, 2022

TokageItLab commented Oct 2, 2022

Mickeon commented Oct 2, 2022

Halfmania33 commented Oct 2, 2022

Mickeon commented Oct 3, 2022 •

edited

Loading

TokageItLab commented Oct 3, 2022 •

edited

Loading

kleonc commented Oct 3, 2022

Mickeon commented Oct 3, 2022 •

edited

Loading

Mickeon commented Oct 4, 2022

TokageItLab Oct 11, 2022

Mickeon Oct 11, 2022

TokageItLab Oct 12, 2022

Mickeon Oct 12, 2022

TokageItLab Oct 12, 2022

KoBeWi Oct 24, 2022

Mickeon Oct 24, 2022

TokageItLab Oct 11, 2022

Mickeon Oct 11, 2022 •

edited

Loading

TokageItLab Oct 11, 2022 •

edited

Loading

TokageItLab Oct 11, 2022

Mickeon Oct 11, 2022 •

edited

Loading

TokageItLab Oct 11, 2022 •

edited

Loading

KoBeWi Oct 24, 2022

TokageItLab Oct 11, 2022

Mickeon Oct 11, 2022

TokageItLab commented Feb 15, 2023

Halfmania33 commented Jun 12, 2023 •

edited

Loading

Calinou commented Jun 12, 2023 •

edited

Loading

TokageItLab commented Oct 15, 2023 •

edited

Loading

akien-mga commented Nov 14, 2023

Allow Tween to accept Arrays & Packed*Arrays #66771

Allow Tween to accept Arrays & Packed*Arrays #66771

Conversation

Mickeon commented Oct 2, 2022 • edited Loading

TokageItLab commented Oct 2, 2022 • edited Loading

Mickeon commented Oct 2, 2022

TokageItLab commented Oct 2, 2022

Mickeon commented Oct 2, 2022

Halfmania33 commented Oct 2, 2022

Mickeon commented Oct 3, 2022 • edited Loading

TokageItLab commented Oct 3, 2022 • edited Loading

kleonc commented Oct 3, 2022

Mickeon commented Oct 3, 2022 • edited Loading

Mickeon commented Oct 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mickeon Oct 11, 2022 • edited Loading

Choose a reason for hiding this comment

TokageItLab Oct 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mickeon Oct 11, 2022 • edited Loading

Choose a reason for hiding this comment

TokageItLab Oct 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TokageItLab commented Feb 15, 2023

Halfmania33 commented Jun 12, 2023 • edited Loading

Calinou commented Jun 12, 2023 • edited Loading

TokageItLab commented Oct 15, 2023 • edited Loading

akien-mga commented Nov 14, 2023

Mickeon commented Oct 2, 2022 •

edited

Loading

TokageItLab commented Oct 2, 2022 •

edited

Loading

Mickeon commented Oct 3, 2022 •

edited

Loading

TokageItLab commented Oct 3, 2022 •

edited

Loading

Mickeon commented Oct 3, 2022 •

edited

Loading

Mickeon Oct 11, 2022 •

edited

Loading

TokageItLab Oct 11, 2022 •

edited

Loading

Mickeon Oct 11, 2022 •

edited

Loading

TokageItLab Oct 11, 2022 •

edited

Loading

Halfmania33 commented Jun 12, 2023 •

edited

Loading

Calinou commented Jun 12, 2023 •

edited

Loading

TokageItLab commented Oct 15, 2023 •

edited

Loading