Tween references being reused causes bugs in the projects

[GoogleCodeExporter]

Killing a tween converts the killd instance into an instance of another tween 
created elsewhere, creting the possibility of random bugs in the game that are 
very difficult to debug.

What version of the product are you using? On what operating system?
0.7.265

1) Download the attached project and open it in Unity with the scene located 
at: Assets\Scenes\Tests\Level Build Test.unity

2) Open the following script: 
Assets\Scripts\Character\WalkControllers\DirectionBasedWalkController.cs

3) Go to the TweenTo and StopWalkTween functions, there you have what causes 
the problem.

4) Open Assets\Scripts\Test1.cs There you have a a tween test that executes to 
chech if all OnComplete are called.

5) Run the game, and before the test finishes walk pressing the arrow keys and 
watch the test in the console show the amount of OnStart not being called.

Original issue reported on code.google.com by fer...@gmail.com on 12 Aug 2014 at 4:58

Attachments:

• TestGame2.zip

[GoogleCodeExporter]

Pasting here what I wrote in the previous Issue #3

I studied the issue, and I don't think it can be solved with ease, since there 
is no way in C# to make a reference NULL. Because of that, I was assuming that 
a user would take care of knowing when a tween has been killed, and thus would 
avoid using it further. If you think that is too confusing we have a problem. 
Let me explain the possible solutions:

A. instantiate a new "pointer" class as a tween instead than the real tween 
object (which would be used in the background), so that its reference is not 
recycled.
PROS: isActive would never be TRUE after it's killed (but any reference to it 
would still not be nulled, since that's simply not possible).
CONS: since a new pointer instance is generated for each tween, and no 
recycling happens, some GC allocation would happen each time a tween is created

B. don't return a tween at all. Instead, if a user wants to access a specific 
tween, he can ask its tweenId at creation, and then use that with the various 
filtering options.
PROS: fixes the issue
CONS: I don't really like it. Things like myTween.Pause() would not be possible 
anymore, and one would have to always do something like DOTween.Pause(tweenId)

What do you think? Maybe this problem could simply be solved by stating clearly 
in the documentation that recycling means the user has to pay attention to 
tween references? Maybe not? Let me know your thoughts

Original comment by daniele....@gmail.com on 12 Aug 2014 at 5:19

• Changed state: UnderInvestigation

[GoogleCodeExporter]

I don't think more than 3% of the users realy need the performance boost of 
instance recycling, only the ones that use a crazy amount of tweens. So I think 
you should disable it by default and if the user wants to enable it he can do 
it with a boolean the init function.

What do you think about that?

Original comment by fer...@gmail.com on 12 Aug 2014 at 5:40

[GoogleCodeExporter]

when you set the boolean in the init function the documentation popup can warn 
the user of this possible bug if they don't pay attention to the references.

Original comment by fer...@gmail.com on 12 Aug 2014 at 5:46

[GoogleCodeExporter]

Mhmm that's an interesting idea: I love recycling and would've never thought of 
disabling it, but it makes much sense. I'll study it and see the best way to 
implement it tomorrow.

Original comment by daniele....@gmail.com on 12 Aug 2014 at 6:43

• Changed state: Accepted

[GoogleCodeExporter]

Original comment by daniele....@gmail.com on 12 Aug 2014 at 6:44

• Added labels: Priority-High, Type-Other
• Removed labels: Priority-Medium, Type-Defect

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

New ideas to add to my solution idea:

Sometimes you need a reference, to do stuff with the tween after it was created:

MyTween = DOTween.to([...])

But if you don't need that you don't need a reference:

DOTween.to([...]);

When you don't need the reference, this issue is not a problem, because you 
can't access the instance after was recycled, because you don't have a 
reference.

That said:

1) Add a setting to enable recycling:

DOTween.to([...]).
   SetRecyclable();

In the documentation of that function add a warning: "If you need to re use the 
reference to this tween for a new tween don't enable this to avoid cross 
reference issues, otherwise if you don't need that you can get more performance 
enabling this specially when running a game with a large amount of simultaneous 
tweens".

CONS: Complicates the usage a little, someone that don't like to read or 
understand english very well can fall in this problem.

2) The Idea 1 can be done automatically but it's a little dirty maybe:

public static Tween To(Par parameters){...}
public static void To2(Par parameters){...}

To2 is recyclable, but returns void, you can't have the reference if it's 
recycalble.

PROS: The user don't have to understand the problem or worry about, it's more 
solid.
CONS: Adds a limitation because mybe you want to do things with a recycalble 
tween and you can't.

I prefer the option 2.

Original comment by fer...@gmail.com on 12 Aug 2014 at 10:04

[GoogleCodeExporter]

1) This would make recycling tween-based instead than framework-based, which 
would add more checks to the system and make things messier. Also, maybe if 
someone wants recycling it's useful to have it for everything. That said, it's 
still an interesting point, also considering I could add a 
DOTween.defaultRecyclingMode which would set all new tweens as recyclable by 
default or viceversa, thus achieving the result of your previous suggestion. 
Gotta ponder more about this.

2) This one is interesting but if done with a simple additional bool option 
like this:
DOTween.To(parameters, bool recyclable = defaultGlobalRecyclableSetting);
CONS: recyclable should be an optional parameter, which would force me to add 2 
overloads for each tween method (because Unity has a bug and doesn't support 
optional parameters with T, where T is a Unity struct like Vector3/Color). 
Gotta ponder more about this too.

Mhmm now I'm undecided about all 3 methods you suggested. Lots of pondering to 
do before going to sleep :D

Original comment by daniele....@gmail.com on 12 Aug 2014 at 10:25

[GoogleCodeExporter]

You are right, didn't think about that, I like the idea of blocking acess to 
the reference in the recyclable tweens. So I have 2 new ideas:

3) A mix of the previous 2 ideas:

DOTween.to([...]).
   SetSpeedBased().
   SetEase(Ease.Linear).
   OnComplete(OnTweenXCompleted);
   SetRecyclable();

SetRecyclable returns null, so it must be the last setting you add.

PROS: You can't acess a recyclable tween reference and don't break all like the 
second idea.
CONS: The user must learn and rememeber that SetRecyclable must be the last 
option.

4) Make recyclng framework-based but when is enabled, all tweens creation 
returns null to block the reference usage.

Original comment by fer...@gmail.com on 12 Aug 2014 at 10:35

[GoogleCodeExporter]

Mhmm I prefer the previous ones better...

3) If a user needs to remember to apply SetRecyclable as the last one, then I 
prefer they remember instead that a recycled tween must be handled with care 
(also storing recyclable tweens can be pretty useful: I personally do that a 
lot)

4) This can't be done, because if tween creations return null then chaining 
wouldn't work

Also, to allow handling references to recyclable tweens better, I will 
implement an OnKill callback, so if a user is afraid they might be storing a 
recyclable tween longer than expected, they can eventually do something like 
this:

myTween = DOTween.To([...]).
  SetSpeedBased().
  OnKill(()=> { myTween = null; });

Original comment by daniele....@gmail.com on 12 Aug 2014 at 10:58

[GoogleCodeExporter]

I like that. You can even add that code snippt in the documentation of the 
parameter that enables recycling

Original comment by fer...@gmail.com on 12 Aug 2014 at 11:00

[GoogleCodeExporter]

Side note: pondering about these things just made me think of a trick which 
might make the general API way better! I'm not saying anything because I'm not 
sure it will work, but tomorrow I'll test it thoroughly and cross my fingers :)

Original comment by daniele....@gmail.com on 12 Aug 2014 at 11:19

• Changed title: Tween references being reused causes bugs in the projects

[GoogleCodeExporter]

How is that going? let me know if you need me to run some tests

Original comment by fer...@gmail.com on 14 Aug 2014 at 11:34

[GoogleCodeExporter]

I was delayed by the implementation of DOLocalAxisRotate, which is a way to 
tween an object around its local axis (instead than just using its parent-based 
localRotation). I thought it would be simple instead it took me until now to 
implement it in the best way :P
Gonna push an update in a few moments and then work on this. I'll have an 
update tomorrow or Saturday.

P.S. same goes for Flash compatibility. This comes first. Then Flash 
compatibility.

Original comment by daniele....@gmail.com on 14 Aug 2014 at 4:23

[GoogleCodeExporter]

Done :) Latest update implements choice of recycling behaviour when calling 
DOTween.Init or by setting DOTween.defaultRecyclable or by setting 
SetRecyclable on specific tweens.
http://dotween.demigiant.com/download.php

Original comment by daniele....@gmail.com on 15 Aug 2014 at 12:15

• Changed state: Fixed