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

Replace FlxSound.onComplete with FlxSound.onFinish #3336

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from

Conversation

DetectiveBaldi
Copy link
Contributor

@DetectiveBaldi DetectiveBaldi commented Jan 10, 2025

Adjusts FlxSound to comply with new changes in flixel involving changing a bunch of single callbacks to use FlxSignals instead.

Copy link
Member

@Geokureli Geokureli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also wanna make sure init() can't be called multiple times, I.E. via recycling or something. if it can then we would need to find a way to remove the listener added there, but not any added externally

/**
* Signal that is dispatched on sound complete.
*/
public var onFinish:FlxSignal;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs to be read only, lets use final

Suggested change
public var onFinish:FlxSignal;
public final onFinish:FlxSignal;

and add onFinish.removeAll(); to destroy() to clear up memory

@DetectiveBaldi
Copy link
Contributor Author

I also wanna make sure init() can't be called multiple times, I.E. via recycling or something. if it can then we would need to find a way to remove the listener added there, but not any added externally

I did some more testing with recycling. I noticed that the callbacks would not be cleared if you killed a sound, and it got recycled. That is fixed with commits I'm pushing now, however I do want to note that some of the line ordering may look a bit inconsistent due to when FlxSound calls certain functions, which might be a problem

Also regarding making onFinish a final: It requires me to assign the variable in the constructor compared to in the reset method like all other variables in the class, might not be important, just something to considered

@Geokureli
Copy link
Member

Thanks, won't be able to test this for a short while, though

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

Successfully merging this pull request may close these issues.

2 participants