bug with Tween.is_active, fixes #39760

[ChristopheLY]

Fixes #39760

[akien-mga]

The fix would be to fix the logic for all_finished on line 716, not hack it here, possibly losing information about a previous data which was active and not finished (so this would introduce a bug IMO).

[ChristopheLY]

Is it better to make this ?
all_finished = !data.active || data.finish;

[akien-mga]

No, that would erase the value from previous iterations.
It should be (on line 712):

all_finished = all_finished && (data.finish || !data.active)

[ChristopheLY]

but all_finished is false at this moment and it's because it false that set_active(false) and emit_signal("tween_all_completed") was not sent (line 816)

[akien-mga]

In your test project, sure. But if could be true if the previous iteration was true.
Its purpose is to check that all iterations are finished (or, as you're trying to change, inactive). If any iteration is active and not finished, it should end up false when the loop terminates.

[akien-mga]

After the last amend we're back to the initial state, which is wrong. See #39801 (comment)

[bsil78]

this line is useless : second part &&... is true by inclosing test ; then 'all_finished' is not changed by this line of code ; better suppress it if logic is good (do not know)

[bsil78]

all_finished = true; is simpler and forces state as well

[akien-mga]

It still doesn't seem correct to me. Read the comments and the code again and think about how this algorithm is meant to work:

• all_finished is assumed true initially.
• For each tween, we check if it's either inactive (!data.active) or finished (data.finish) to confirm if all_finished is still true (if the tween is active or not finished, then all_finished must become false. It must then stay false regardless of the state of future tweens, as at least one Tween is not finished.
• If the current tween is finished, we skip to the next.

Your current code will make it so that as regardless of previous values of all_finished, the variable with be set to true any time the iterated tween is finished (so missed the point of the all_ in the variable name).

Unroll the code on paper if it helps. For the sake of simplicity, let's assumed "finished" means !data.active || data.finish, so either !active or finish is true.
Here's what your current PR does (two values for all_finished in the loop for each assignment (all_finished = all_finished && (!data.active || data.finish); first and all_finished = true; next, if finished):

Step	Tween state	all_finished
Before loop	---	true
Tween 0	Finished	true / true
Tween 1	Not finished	false / ---
Tween 2	Finished	false / true
After loop	---	true

The Tween will be deactivated and emit tween_all_completed even though Tween 1 is not finished.

[akien-mga]

That's better, but still incorrect. This will now mean that if any previous Tween was unfinished, all_finished will become and stay false (which is good), but subsequent Tweens which are actually finished won't use the early continue, leading to unnecessary computation.

Here's the right fix:

diff --git a/scene/animation/tween.cpp b/scene/animation/tween.cpp
index 854db5fee2..d45fc7dd07 100644
--- a/scene/animation/tween.cpp
+++ b/scene/animation/tween.cpp
@@ -709,7 +709,7 @@ void Tween::_tween_process(float p_delta) {
                InterpolateData &data = E->get();
 
                // Track if we hit one that isn't finished yet
-               all_finished = all_finished && data.finish;
+               all_finished = all_finished && (!data.active || data.finish);
 
                // Is the data not active or already finished? No need to go any further
                if (!data.active || data.finish) {

[akien-mga]

Here's another option which might be better and fixes a possible extra bug in that else if (!repeat) branch, but I haven't tested so I can't say if it works as I think it should:

diff --git a/scene/animation/tween.cpp b/scene/animation/tween.cpp
index 854db5fee2..53dacbadbc 100644
--- a/scene/animation/tween.cpp
+++ b/scene/animation/tween.cpp
@@ -701,21 +701,21 @@ void Tween::_tween_process(float p_delta) {
 	}
 
 	// Are all of the tweens complete?
-	bool all_finished = true;
+	int any_unfinished = 0;
 
 	// For each tween we wish to interpolate...
 	for (List<InterpolateData>::Element *E = interpolates.front(); E; E = E->next()) {
 		// Get the data from it
 		InterpolateData &data = E->get();
 
-		// Track if we hit one that isn't finished yet
-		all_finished = all_finished && data.finish;
-
 		// Is the data not active or already finished? No need to go any further
 		if (!data.active || data.finish) {
 			continue;
 		}
 
+		// Track if we hit one that isn't finished yet
+		any_unfinished++;
+
 		// Get the target object for this interpolation
 		Object *object = ObjectDB::get_instance(data.id);
 		if (object == nullptr) {
@@ -802,17 +802,15 @@ void Tween::_tween_process(float p_delta) {
 			// If we are not repeating the tween, remove it
 			if (!repeat) {
 				call_deferred("_remove_by_uid", data.uid);
+				any_unfinished--;
 			}
-		} else if (!repeat) {
-			// Check whether all tweens are finished
-			all_finished = all_finished && data.finish;
 		}
 	}
 	// One less update left to go
 	pending_update--;
 
 	// If all tweens are completed, we no longer need to be active
-	if (all_finished) {
+	if (any_unfinished == 0) {
 		set_active(false);
 		emit_signal("tween_all_completed");
 	}

[ChristopheLY]

Should I apply the other option ?
It's work, like the last one

[akien-mga]

I guess it might be safe enough, and easier to understand, yeah.

[akien-mga]

Thanks!

[akien-mga]

Cherry-picked for 3.2.3.

[akien-mga]

I reverted the 3.2.3 cherry-pick due to the regression #40679.

Once the regression is solved in the master branch, we can look into cherry-pick this with the regression fix again.

[akien-mga]

Cherry-picked/backported with regression fixed via #47142.