Propagate Tween.kill() to subtweens

[Meorge]

This PR fixes a problem with tween/subtween logic I noticed while investigating another issue. Previously, when a tween was killed with kill(), if it had subtweens in it that hadn't yet been run, those subtweens would never be executed, but would also never be explicitly killed or made invalid. As a result, if user code was monitoring the status of a subtween (say, waiting for it to be killed or finished), they would end up waiting forever.

With this PR, a kill() call on a Tween is passed down to all of its subtweens as well, ensuring that they are all marked as dead/invalid.

Note

Previously this PR also cleared subtweens when clear() was called on the parent. As I thought it through more I realized this didn't make sense, so I removed that but kept the kill() behavior.

[KoBeWi]

I'm not sure about that. It makes kill() more expensive to use even if you don't use sub-Tweens.
Maybe Tween could have a LocalVector<Ref<Tween>>, containing sub-Tweens and appended in tween_subtween().

[Meorge]

Thanks for taking a look!

It will definitely have some level of an adverse effect on the performance. It was programmed this way to keep a single source for the tween's contents. Adding a second vector/list/etc for subtweens means we have to ensure it is always updated in sync with the actual tween list; if we forget to update both structures in some operation, we could get some strange bugs. While the time complexity of the current approach is unarguably worse than what you're proposing, I also don't know if tweens would be large enough for that to become practically noticeable, especially given that kill() is a one-time operation (i.e., not something that should be run each frame).

This all being said, if you still think the 1D subtween vector approach would be a net improvement to what I have here, please let me know and I can change it to use that instead 😄

[KoBeWi]

we have to ensure it is always updated in sync with the actual tween list

Sub-Tweens can be added only by a single method and can't be removed, so it's not really a problem.

kill() is a one-time operation (i.e., not something that should be run each frame).

Well, there is this pattern, which I think is quite common

# Kill old Tween if it exists.
if tween:
	tween.kill()
# Start new animation.
tween = create_tween()

Though not sure if people call it often enough to have problems.

[Meorge]

Sub-Tweens can be added only by a single method and can't be removed, so it's not really a problem.

Very good point, yeah! Nothing's coming to mind for me for future points where this list would need to be updated to keep it in sync, so perhaps that worry was for (relatively) nothing 😅

Well, there is this pattern, which I think is quite common
Though not sure if people call it often enough to have problems.

Ah, I use that pattern a lot myself! In my own experience, at least, the new animations aren't created frequently enough, and they don't have enough complexity/steps, where I feel like the game overall would be prone to a major slowdown...

Although, it did just occur to me as I was typing this, that the worse time complexity on this isn't just a worst-case scenario, it's guaranteed because there's no early exit condition. Between that and the point you made about few places where the lists need to be updated, I think it does make sense to reimplement this fix using an approach like the one you described. I should hopefully have that up soon!