Demigiant / dotween

A Unity C# animation engine. HOTween v2
http://dotween.demigiant.com
Other
2.36k stars 351 forks source link

Calling ManualUpdate inside of a Callback can lead to IndexOutOfRangeExceptions #656

Open lfg-ryan opened 1 year ago

lfg-ryan commented 1 year ago

If you have a sequence with a callback that kicks off another sequence. Calling ManualUpdate can result in an exception in TweenManager breaking the DoTween engine.

Attach the below script to a button, in the button click call the function in the script and DoTween will throw and be broken until application restart.

using DG.Tweening;
using UnityEngine;

public class BreakDoTween : MonoBehaviour
{
    public void OnButtonTap()
    {
        var sequence = DOTween.Sequence();
        sequence.AppendInterval(0.5f);
        sequence.PrependCallback(() => {
            var sequence2 = DOTween.Sequence();
            sequence2.AppendInterval(0.5f);
            sequence2.ManualUpdate(1, 1);
        });
    }
}

This throws the IndexOutOfRangeException.

IndexOutOfRangeException: Index was outside the bounds of the array.
DG.Tweening.Core.TweenManager.RemoveActiveTween (DG.Tweening.Tween t) (at D:/DG/_Develop/__UNITY3_CLASSES/_Holoville/__DOTween/_DOTween.Assembly/DOTween/Core/TweenManager.cs:1181)
DG.Tweening.Core.TweenManager.Despawn (DG.Tweening.Tween t, System.Boolean modifyActiveLists) (at D:/DG/_Develop/__UNITY3_CLASSES/_Holoville/__DOTween/_DOTween.Assembly/DOTween/Core/TweenManager.cs:236)
DG.Tweening.Core.TweenManager.DespawnActiveTweens (System.Collections.Generic.List`1[T] tweens) (at D:/DG/_Develop/__UNITY3_CLASSES/_Holoville/__DOTween/_DOTween.Assembly/DOTween/Core/TweenManager.cs:1167)
DG.Tweening.Core.TweenManager.Update (DG.Tweening.UpdateType updateType, System.Single deltaTime, System.Single independentTime) (at D:/DG/_Develop/__UNITY3_CLASSES/_Holoville/__DOTween/_DOTween.Assembly/DOTween/Core/TweenManager.cs:496)
DG.Tweening.Core.DOTweenComponent.Update () (at D:/DG/_Develop/__UNITY3_CLASSES/_Holoville/__DOTween/_DOTween.Assembly/DOTween/Core/DOTweenComponent.cs:75)

and

IndexOutOfRangeException: Index was outside the bounds of the array.
(wrapper stelemref) System.Object.virt_stelemref_class_small_idepth(intptr,object)
DG.Tweening.Core.TweenManager.ReorganizeActiveTweens () (at D:/DG/_Develop/__UNITY3_CLASSES/_Holoville/__DOTween/_DOTween.Assembly/DOTween/Core/TweenManager.cs:1157)
DG.Tweening.Core.TweenManager.Update (DG.Tweening.UpdateType updateType, System.Single deltaTime, System.Single independentTime) (at D:/DG/_Develop/__UNITY3_CLASSES/_Holoville/__DOTween/_DOTween.Assembly/DOTween/Core/TweenManager.cs:404)
DG.Tweening.Core.DOTweenComponent.Update () (at D:/DG/_Develop/__UNITY3_CLASSES/_Holoville/__DOTween/_DOTween.Assembly/DOTween/Core/DOTweenComponent.cs:75)

The reason this happens is TweenManager.cs MarkForKilling tries to prevent this but fails when the ManualUpdate is as a result of a PrependCallback because TweenManager.isUpdateLoop is true preventing TweenManager.Despawn from being called directly.

    private static void MarkForKilling(Tween t, bool isSingleTweenManualUpdate = false)
    {
      if (isSingleTweenManualUpdate && !TweenManager.isUpdateLoop)
      {
        TweenManager.Despawn(t);
      }
      else
      {
        t.active = false;
        TweenManager._KillList.Add(t);
      }
    }

This results in sequence2 being added to the _KillList but not being destroyed. On the next update loop sequence2.active == false resulting in a quick call to MarkForKilling and the sequence getting added a second time to _KillList.

With two entries of the same sequence in _KillList when the sequence is passed the first time to RemoveActiveTween its activeId is set to -1, the second time its passed to RemoveActiveTween it causes an index out of range TweenManager._activeTweens[activeId] = (Tween) null;.

I suggest giving MarkForKilling a more robust check to make sure its not being called from a ManualUpdate within a broader update call and adding a simple check for activeId == -1 and earlying out from RemoveActiveTween.

Another possible fix would be something similar to what sequence Kill method does. Where it just sets the t.active but does not add to a kill list. This would look something like this. I would still suggest the -1 check in RemoveActiveTween with some logging given how catastrophic it fails.

      if (isSingleTweenManualUpdate)
      {
            if (!TweenManager.isUpdateLoop)
            {
                TweenManager.Despawn(t);
            }
            else
            {
                t.active = false;
            }
      }
      else
      {
             t.active = false;
             TweenManager._KillList.Add(t);
      }

The real world use case for this was a view animating in, where when it was shown a prepended callback resulted in a Activate of a game object, this kicked off another animation to scale from 0->1 which had a ManualUpdate of a single frame so that there would be no visible pop to the scale 0.

lfg-ryan commented 1 year ago

For those that run into this and need a work around. Here is how I prevented this. I have scriptable objects that define animations and many places in UI can trigger them. I added a check before the ManualUpdate to make sure we never call it with a value less than its duration. And if the duration is so fast just skip the ManualUpdate all together.

                    var duration = activeSequence.Duration();
                    if (duration > Time.deltaTime && duration > Time.unscaledDeltaTime) {
                        activeSequence.ManualUpdate(Time.deltaTime, Time.unscaledDeltaTime); 
                    }
j1mmie commented 11 months ago

Thanks for sharing this repro case, and workaround. I'm going to test it out myself in a bit. It would be nice if DOTween had a way to detect a breakage like this and recover