SceneTreeTween.finished signal is not consistent with Tween.tween_all_completed

[dalexeev]

Godot version

v3.5.stable.official [991bb6a], v4.0.alpha.custom_build [4610372]

System information

Kubuntu 22.04

Issue description

Bug 1:

Press the Up or Down key and wait. The finished signal may be emitted before the displayed value reaches 10 or -10. At the same time, an additional update/queue_redraw allows you to workaround the bug. That is, the bug is not that Tween does not bring the value to the end, but that it reports this too early (in the wrong frame).

This bug is unstable, in some cases it is reproducible, in others it is not. For me, it is almost always reproduced for the first time, then it is not reproduced, then it can appear again.

Old 3.x Tween does not have this bug, tween_all_completed is always emitted after the visible value has become correct.

Bug 2 (need a separate discussion?):

(Found by accident while testing MRP for Bug 1. So some of the things below are not quite correct, the asynchronous approach for this task works with the old Tween, but does not work with the new one. Need to implement it somehow differently.)

Press the Up and Down buttons alternately to keep the value around 0, preventing it from reaching 10 or -10.

In the case of the old Tween, this will work, since there is only one Tween node, and new calls to interpolate_property will override the old ones. You can keep the value around 0 for as long as you like, and the tween_all_completed signal will only be emitted once when the variable's value reaches 10 or -10 (multiple console messages because multiple asynchronous calls continued waiting for the same signal).

Something strange is happening in the case of the new Tween.

We no longer have a single Tween node, but multiple Tween objects created each time the animate is called. The new Tween is designed for a "create and use" approach, it's not entirely clear how to use it in this case (put it in a class member?). There was an issue recently that raised a similar question (about Tween's lifetime, and that it is invalidated when the animation finishes).

Is it acceptable to have multiple instances of Tween animate the same property of the same object? Now something strange is displayed in the console: alternately 10 and -10, although the value should always stay somewhere around 0 (apparently, each instance of Tween operates with its own copy of the value).

Steps to reproduce

See above.

Minimal reproduction project

tween_3.5.zip
tween_4.0.zip

[KoBeWi]

Bug 1 (which isn't really a bug) comes from the fact that Tweens don't process together with the nodes (unlike the old Tweens). You are disabling process between frames, so the next process does not get called. Your workaround is the intended action here in this case.

For 2, the first tween that finishes will stop process, so your label stops updating. If you uncomment your queue_redraw(), the label will finish with the correct value. When you have 2 Tweens tweening the same property, the last one takes priority (like with AnimationPlayers). In your example above you should keep the Tween in a member variable and kill it when starting new animation.

var tween
func animate():
    if tween:
        tween.kill()
    tween = create_tween()

EDIT:
Maybe this snippet should be in the docs, with a mention how 2 tweens work on 1 property.

[kleonc]

Bug 1 (which isn't really a bug) comes from the fact that Tweens don't process together with the nodes (unlike the old Tweens).

@KoBeWi That's what I initially suspected too but that's not the reason. In the MRP processing is disabled instantly on the finished signal so the order of tweens/nodes processing doesn't really matter here, after the signal the next _process call won't happen regardless if it would be during the current/next processing frame.

It seems like the actual reason is that the 3.x Tween node is buggy as it's emitting its tween_all_completed signal one processing frame too late. Specifically:

• all_finished is updated based on the value of data.finish:
  

  godot/scene/animation/tween.cpp

  Lines 737 to 738 in 13d99d1

  	// Track if we hit one that isn't finished yet
  	all_finished = all_finished && data.finish;

• but data.finish used above is not yet updated for the given frame, it's being updated later (also > instead of >= in this check looks like another bug 🤔):
  

  godot/scene/animation/tween.cpp

  Lines 762 to 767 in 13d99d1

  	// Are we at the end of the tween?
  	if (data.elapsed > (data.delay + data.duration)) {
  	// Set the elapsed time to the end and mark this one as finished
  	data.elapsed = data.delay + data.duration;
  	data.finish = true;
  	}

  
  And of course tween_all_completed signal is emitted based on the value of all_finished:
  

  godot/scene/animation/tween.cpp

  Lines 853 to 857 in 13d99d1

  	// If all tweens are completed, we no longer need to be active
  	if (all_finished) {
  	set_active(false);
  	emit_signal("tween_all_completed");
  	}

To better visualize why such behavior is wrong: if each frame would last 1 second and Tween node would be used to interpolate something for 2 seconds then the tween_all_completed signal would be emitted after 3 seconds (tween_completed signal would be correctly emitted after 2 seconds, seems like an issue specific to tween_all_completed signal).

For SceneTreeTimer (4.x's Tween) its finished signal is emitted at the exact frame when it's finished, it behaves correctly.

@dalexeev The problem with update/queue_redraw looks like a user error / incorrect logic, that's on your side. It has nothing to do with the correctness of SceneTreeTween/Tween(4.x) which seem to work fine. The 3.x version of your code works because of the mentioned bug, it's not supposed to work as you're expecting it to. I wonder for how many users fixing this bug would actually cause issues because of their current incorrect logic...
If you want to ensure _process is called once more after tweening is done then I'd simply stop the processing from the _process itself:

extends Control

# warning-ignore-all: return_value_discarded

var _num := 0.0
onready var _font := get_font('font')

func _ready() -> void:
	set_process(false)

func _process(_delta: float) -> void:
	update()
	if not tween.is_running():
		set_process(false)

func _draw() -> void:
	draw_string(_font, Vector2(), '%.3f' % _num)

func _input(event: InputEvent) -> void:
	if event.is_action_pressed('ui_down'):
		animate(-10.0)
	if event.is_action_pressed('ui_up'):
		animate(10.0)

var tween := SceneTreeTween.new()
func animate(to: float) -> void:
	set_process(true)
	if tween.is_running():
		tween.kill()
	tween = create_tween()
	tween.tween_property(self, '_num', to, 2.0)

---

@KoBeWi Regarding the documentation it might be a good idea to mention somewhere that SceneTreeTweens/SceneTreeTimers are processed after nodes' _process/_physics_process (depending on the processing mode).

[dalexeev]

Thank you. It seems to me that this should be mentioned in the description of the signal, although this does not apply directly to it (it should always be taken into account when working with _process). Now I understand that I was wrong, but it seems to me that many people have the same misunderstanding of this signal as I have. Especially because of the bug in 3.x.

As for the new Tween (several Tweens working with the same property), I think this snippet should be added to the corresponding tutorial. Creating a Tween on the fly is convenient, but can be confusing in non-trivial cases.