Adding UNITASK_DOTWEEN_SUPPORT scripting define results in a ton of warnings about not awaiting a tween/sequence
Stever1388 opened this issue · 5 comments
I'm using DOTween for tweens and UniTask for async. I didn't realize for a while that DOTween was supported directly but a script define had to be added. So I went to add it today and it results in a ton of warnings on my tweens/sequences saying "Because this call is not awaited, execution of the current method continues before the call is completed. Consider applying the 'await' operator to the result of the call.". Some examples of code:
Sequence s = DOTween.Sequence();
// every line after this shows the warning
s.Append(container.DOFade(1, 0.75f));
s.AppendInterval(this.mediaDuration);
s.Append(container.DOFade(0, 0.75f));
s.SetId(this.GetInstanceID());
Tween t = container.DOFade(0, 0.25f);
bool wasKilled = false;
// the following line shows the warning
t.OnKill(() => wasKilled = true);
// all regular tweens show it too
this.rightQuestion.DOFade(1, 1);
Any way to make these warnings go away? I think the issue is that Tweens are directly awaitable (await this.rightQuestion.DOFade(1, 1)). I wonder if it would make more sense to require calling a function on the tween/sequence in order to await the tween. For example, without the script define, you have to do this.rightQuestion.DOFade(1, 1).AsyncWaitForCompletion() in order to await the tween. For UnityWebRequests, you have to call SendWebRequest in order to await it. Seems like it would be more consistent with other objects if you had to call a function and it would remove all the warnings.
I was just thinking about the same problem.
For safety reasons, I would like to treat CS4014 as an error, but it is difficult to correct all the parts that use DOTween.
Is it possible to provide a define symbol for disable the GetAwaiter extension method?
Duplicate of #452 .
Unfortunately it seems difficult to support anything officially.
There is an option to use #pragma warning disable CS4014 . And if you want to convert to UniTask you can use .Forget().
Duplicate of #452 .
Unfortunately it seems difficult to support anything officially. There is an option to use
#pragma warning disable CS4014. And if you want to convert to UniTask you can use.Forget().
Is it not possible to switch so that the awaiting happens on a function call instead of the Tween itself? I don't really understand how this all works, but without your package, it's not possible to do await container.DOFade(1, 0.75f). You have to do await container.DOFade(1, 0.75f).AsyncWaitForCompletion(). Is there something stopping you from being able to use this? Or even just add a new extension function on DOTween objects that returns an awaitable object (or something). You can't do await myUnityWebRequest, you have to do await myUnityWebRequest.SendWebRequest(). So wouldn't it make sense to be consistent with these two?
Is it not possible to switch so that the awaiting happens on a function call instead of the Tween itself?
Yeah, not impossible, but breaking changes with significant impact.
Apparently, when this library was created, the compiler did not warn about the extension methods. So at the time, the API seemed reasonable. However, making a major change to this is a big deal.
You have to do await container.DOFade(1, 0.75f).AsyncWaitForCompletion(). Is there something stopping you from being able to use this?
I wonder if this is a method provided by DOTWeen. This uses System.Threading.Task; UniTask provides a more optimized implementation than Task.
even just add a new extension function on DOTween objects that returns an awaitable object (or something). You can't do await myUnityWebRequest, you have to do await myUnityWebRequest.SendWebRequest(). So wouldn't it make sense to be consistent with these two?
SendWebRequest() has exactly the same problem as this issue. It returns Unity's AsyncOperation, which Unity is not designed to await, but UniTask provides an extension that allows it to await.
If an object representing an asynchronous operation has been created and is running, being able to await it as is is certainly easier to use. I think the problem is that tween is used in a way that does not wait for completion. Your suggestion is one way to do it, but I think it has a lot of impact and is something to consider.
Is it not possible to switch so that the awaiting happens on a function call instead of the Tween itself?
Yeah, not impossible, but breaking changes with significant impact. Apparently, when this library was created, the compiler did not warn about the extension methods. So at the time, the API seemed reasonable. However, making a major change to this is a big deal.
Hmm, yeah that makes sense. I wonder how many people use this package with DOTween enabled? I didn't even realize it could be enabled and I've been using this for awhile. Sometimes breaking changes for the betterment of the package is necessary... sometimes I wish Unity would just break some things to make their APIs better, like the fact that actions (like OnClick) don't return the sender as the first parameter (which is what Microsoft recommends for events). But I could see an argument for not breaking it... but honestly my preference would be to break it with a major version update.
You have to do await container.DOFade(1, 0.75f).AsyncWaitForCompletion(). Is there something stopping you from being able to use this?
I wonder if this is a method provided by DOTWeen. This uses System.Threading.Task; UniTask provides a more optimized implementation than Task.
Yes, this is a function that DOTween provides that can be directed awaited. I usually call AsUniTask() on it, not really sure if that's needed it makes it more optimized or not, so it looks something like await aCanvasGroup.DOFade(1, 0.75f).AsyncWaitForCompletion().AsUniTask();. Right now, since I don't have the DOTween part enabled, if I want to add a cancellation token to it, I then chain .AttachExternalCancellation(token); to it, and in the catch part I kill the tween manually (since the cancellation doesn't cancel it itself, I'm assuming because it's an "external" cancellation). If I enable DOTween support, I can do just the regular WithCancellation(token) (I think) and it will kill the tween without me having to do it.
even just add a new extension function on DOTween objects that returns an awaitable object (or something). You can't do await myUnityWebRequest, you have to do await myUnityWebRequest.SendWebRequest(). So wouldn't it make sense to be consistent with these two?
SendWebRequest()has exactly the same problem as this issue. It returns Unity's AsyncOperation, which Unity is not designed to await, but UniTask provides an extension that allows it to await. If an object representing an asynchronous operation has been created and is running, being able to await it as is is certainly easier to use. I think the problem is that tween is used in a way that does not wait for completion. Your suggestion is one way to do it, but I think it has a lot of impact and is something to consider.
Hmmm, I think Unity's AsyncOperations are allowed to be awaited. I've done await www.SendWebRequest() even without UniTask installed and it works as expected. With UniTask installed, I don't know if it's better to leave it like that or do .ToUnitTask() on it and await that. If I need to add a cancellation token to it, I do await await www.SendWebRequest().WithCancellation(token);
But in general, I would still support just breaking it and letting people know it's going to change/break. As an alternative, I wonder if a fork could be created that has these breaking changes in it? I don't know if there's another solution. Maybe a different scripting define? So you have two versions of the DOTween extensions, one that is the original and one that changes the way it works and then the scripting define can be like UNITASK_DOTWEEN_SUPPORT_ALT or something. So people can choose to stick with what they are using or use the new one.