Leaking entities when you don't clean up your tweens

[Leander1P]

We've built a couple of scenes with tween-driven animations etc; some on delays, some looping.

If the user skips or otherwise causes the scene to change early, the tween.entity links can keep the entities alive despite our scene changer calling pc.app.root.findByName('Root').destroy().

We've put a canary into Tween's update method to help us find these (and clean them up, although this is later than desired):

        update: function (dt) {
            if (this.stopped) return false;

            // 1P PATCH
            if ( this.entity && ! this.entity.parent ) {
                console.error( "Tween on unparented entity, likely leak (stopping tween): " + this.entity.name + ", guid: " + this.entity.getGuid() );
                return false;
            }            

But this requires the fix at https://github.com/playcanvas/engine/issues/994 to set _parent = null as a canary, and it might be the case that one would have a legit reason to keep a tween on an unparented/unenabled/etc entity (although no good reason for a destroyed entity, presumably -- we just don't know how to tell them apart).

Is there a good pattern for removing these? I notice as Element and Screen went into the mainline there have been some event handlers for removed-style events that we might listen to? Would love some input on an approach, and we might even have time to convert it into a PR as a starting point. =)

Thanks!

[vkalpias]

I pushed some fixes to the engine. I added a 'destroy' event on pc.Entity which is fired after the entity is destroyed. I also pushed this fix here https://github.com/playcanvas/playcanvas-tween/commit/6a9c9f5017ec77c5f8c645cc3ee2435290872921 which handles the 'destroy' event so that the tween is stopped when the entity is destroyed.

[Leander1P]

Looks good while testing https://github.com/playcanvas/engine/issues/994 with this applied. Closing. =)

---

Tangential babbling:

This does seem to defer releasing the entity until one update of TweenManager later; my (admittedly dated from Nintendo GBA/DS dev days, heh) inclination is usually to get low-water-mark as low/immediate as possible. It's not entirely clear to me how to do that without complicating the TweenManager update loop more, since entity destroy could be triggered indirectly from e.g. an oncomplete event from an updating tween. I'm not sure it matters terribly for our (or anyone else's) usecase anyway -- the canary patch above had a similar delay and we didn't see any impact on minspec device stability. GC likely isn't going to collect everything immediately anyway, and we already have a ~3-update delay in between scene destruction and next scene load for other reasons.

I'd be curious to hear if you had any other thoughts/opinions on this, though, if/when you have a moment. I'm a near-neophyte in the javascript dev arena, not sure if there are patterns/best-practices regarding this sort of thing floating about.

[vkalpias]

Well the alternative thing we could do in the 'destroy' handler in the tween is instead of stopping the tween just remove it right away. Although to be honest one frame difference is probably not worth the extra effort.