Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Destroyed FlxFlicker error #2083

Closed
MSGhero opened this issue Jun 15, 2017 · 7 comments
Closed

Destroyed FlxFlicker error #2083

MSGhero opened this issue Jun 15, 2017 · 7 comments

Comments

@MSGhero
Copy link
Member

MSGhero commented Jun 15, 2017

Manually destroying a FlxFlicker and making a new one on the same object causes an error because the _boundObjects reference isn't removed. timer is null, and stopFlicker via ForceRestart doesn't check for null.

This is solved if destroy calls stop and if stop is given null checks. If destroy isn't intended to be used in this way, stop should be publicly accessible so a user can stop the flicker via the flicker itself and not solely via the object being flickered.

ff = FlxFlicker.flicker(obj, ...);
// onComplete
ff.destroy();
// later
FlxFlicker.flicker(obj, ...);
// error, object is "still flickering" but the existing flicker has been destroyed

Latest dev

@Gama11
Copy link
Member

Gama11 commented Jun 15, 2017

Yeah, you shouldn't expect that an object is still usable after you call destroy() on it.. (imagine that for FlxSprite).

@MSGhero
Copy link
Member Author

MSGhero commented Jun 15, 2017

I can make a PR, but what's better: adding stop to destroy or making stop public, or both? Destroy doesn't currently stop the timer from executing more callbacks, and it doesn't free up the object map reference.

@Gama11
Copy link
Member

Gama11 commented Jun 17, 2017

Why do you need stop() to be public, isn't that what FlxFlicker.stopFlickering() is for?

@MSGhero
Copy link
Member Author

MSGhero commented Jun 17, 2017

So you don't have to go through the static object map to stop the FlxFlicker if you already have a reference to it. Since flicker returns the FlxFlicker object, it's not unreasonable that someone would hold on to the reference and do things directly with it.

@Gama11
Copy link
Member

Gama11 commented Jun 17, 2017

Right.. I'm not really a huge fan of this API, it probably shouldn't be in core Flixel to begin with... But I don't really see any harm in making stop() public. destroy() should properly clean things up as well.

@Gama11 Gama11 added the Bug label Jun 17, 2017
@MSGhero
Copy link
Member Author

MSGhero commented Jun 17, 2017

stop() already calls destroy() through _pool.put(this), so this is trickier than I thought without causing a stack overflow... destroy() is more like a private helper to stop(), except it has to be public because of the interface.

destroy() should never be called by the user, basically, if that's possible to ensure for the public API. Otherwise, there will be some duplicated code since destroy calls stop/release/put calls destroy ...

@Gama11
Copy link
Member

Gama11 commented Jun 17, 2017

I wouldn't worry about it then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants