Cleared Tweener/TweenSequence cylic references

[Rubonnek]

Attempting to add a node through the Editor and closing up the editor shows the following warning without this patch:

ERROR: ~List: Condition "_first != __null" is true.
   At: ./core/self_list.h:112.
ERROR: ~List: Condition "_first != __null" is true.
   At: ./core/self_list.h:112.
WARNING: cleanup: ObjectDB instances leaked at exit (run with --verbose for details).
   At: core/object.cpp:2135.
Leaked instance: GDScript:23459 - Resource path: 
Leaked instance: GDScript:23462 - Resource path: 
Leaked instance: GDScript:23356 - Resource path: 
Leaked instance: GDScript:23460 - Resource path: 
Leaked instance: GDScript:23458 - Resource path: 
Leaked instance: GDScript:23457 - Resource path: res://addons/godot-next/references/tween_sequence.gd

This patch fixes that.

Note: to see the above messages, #70 first has to be applied.

[Rubonnek]

I simply separated the classes. Again, there should be no negative impact at all.

[Rubonnek]

There are other classes which can be broken down too to avoid having memory issues.

Some of these group of classes have dependencies that can be grouped in folders.

I'm going to split this into folders and have a LICENSE file and a USAGE.md file on each.

[willnationsdev]

Changes seem okay-ish to me. @KoBeWi, you mind taking a look at this to confirm?

[KoBeWi]

Well, the functionality didn't change (although changing TweenSequence return value to Reference will disable nice autocompletion).

IMO it's better to keep it all in one file (the cyclic reference problems will be gone in 4.0). But that's just my preference, other than that it should be ok to merge.

[willnationsdev]

I wonder if the memory leak issues Rubonnek has been finding will be eliminated in 4.0 once cyclic reference issues are resolved. If it causes memory issues though, I don't think we can afford to leave it as it is.

I think that, generally, it's best practice for separately used classes to be in their own dedicated files. The only other example I can think of off the top of my head is FileSearch which has other classes, but they are generally only for internal use and aren't meant to be used directly as it has static methods exposed for using the classes. So, I'm inclined to keep them in separate files in this case, unless there are specific objections.

Or would you guys rather this all be done via a namespace script instead?

class_name Tweens
const Sequence = preload("..../tween_sequence.gd")
const Interval = preload("...../interval_tweener.gd")
# etc.

# usage
var seq = Tweens.Sequence.new()

[KoBeWi]

Or would you guys rather this all be done via a namespace script instead?

Doesn't really matter, as none of these classes inherit from node, so they don't appear on node list anyways.

Although Tweener and derived classes are somewhat internal to TweenSequence. It makes little sense for them to exist on their own.

[Rubonnek]

Well, the functionality didn't change (although changing TweenSequence return value to Reference will disable nice autocompletion).

Casting the returned object to TweenSequence enables the autocompletion again, but I get that having to remember to do so could be aggravating.

I agree about dropping some of the scripts into a namespace. I'll update this PR accordingly this week.

[Rubonnek]

Or would you guys rather this all be done via a namespace script instead?

class_name Tweens
const Sequence = preload("..../tween_sequence.gd")
const Interval = preload("...../interval_tweener.gd")
# etc.

# usage
var seq = Tweens.Sequence.new()

I attempted this but it creates another cyclic reference between tweens.gd and tween_sequence.gd since Tweens.Sequence has a Tweens reference within it (i.e., Tweens.Interval, Tweens.Property, etc).

I think our best shot is just to keep them in separate class_names.

[willnationsdev]

Although Tweener and derived classes are somewhat internal to TweenSequence. It makes little sense for them to exist on their own.

Checking back over it, and yeah, instantiating them separately wouldn't really have any meaning. If they were standalone in some capacity, it would make more sense to make them script classes as I'd been thinking. But since they really are more like inner classes, it would actually be best practice not to expose them directly to the global namespace (not that they'd pollute the node/resource create dialogs, but still...).

Since a namespace doesn't work, I would just leave them as inner classes. I know I said, "I'm inclined to keep them in separate files in this case," but I then thought about why people would ever even want to instantiate them globally and there isn't really a reason for it. Might as well use the TweenSequence class itself as the namespace for them, i.e. make them class constants.

[Rubonnek]

I've updated the PR accordingly.

[Rubonnek]

Perhaps it's worth mentioning that I moved the Tweener derived classes inside the TweenSequence class itself instead of making having them in a const namespace because TweenSequence itself calls these classes explicitly (i.e. right now you don't pass a reference to a subclass of PropertyTweener to create an object for example), so it didn't make sense to keep them separate to me.

In order to avoid a cyclic reference, only the abstract Tweener class must be global.

[willnationsdev]

@Rubonnek Thanks. Appreciate all your work on this (and other changes in the repo recently).

[Rubonnek]

@willnationsdev Thank you for actively maintaining this repository, and specifically for your video on youtube that I stumbled upon recently. It was just what I was looking for and this repository turned out to have much more useful code than I expected.