HaxeFlixel / flixel

Free, cross-platform 2D game engine powered by Haxe and OpenFL
https://haxeflixel.com/
MIT License
1.99k stars 439 forks source link

FlxSubState close callback not used #2429

Open gamerbross opened 3 years ago

gamerbross commented 3 years ago

FlxSubState's closeCallback is not used when you close it from its own function

fixed code:

    public function close():Void
    {
        if (closeCallback != null)
            closeCallback();
        if (_parentState != null && _parentState.subState == this)
            _parentState.closeSubState();
    }
raf13lol commented 3 years ago

pr it then lol

NQNStudios commented 2 years ago

I think this needs adjustment, based on reading through the code. The call to _parentState.closeSubState() within the if statement sets a flag that results in all the proper callbacks and destruction logic being called during the next tryUpdate(). So except in cases where _parentState is null or this is not _parentState's current substate, the callback will be called properly. The suggested code would result in sometimes double-calling the callback.

There may be cases where close() is called on a substate that doesn't currently belong to the parent. What I think you'd really want to do for those is like this

        public function close():Void
    {

        if (_parentState != null && _parentState.subState == this) {
            _parentState.closeSubState();
                } else {
                        if (closeCallback != null)
                    closeCallback();
                        destroy();
                }
    }

This opens up another can of worms which is that the parent state's substateClosed event won't dispatch, and the parentState's destroySubStates flag is ignored. I guess the programmer might be aware of that, if they're closing a substate while it isn't active.

Geokureli commented 2 years ago

can I get more info on what the problems and the expectations are? Like a specific case or intended flow. This sounds like a good change but I'm afraid of breaking existing games. any pr will likely need new unit tests, too

NQNStudios commented 2 years ago

My use-case (which may be different from @gamerbross) is that I'm making a visual novel where global state is stored in a FlxState, but different backgrounds, character sprites, props, etc. are added to different FlxSubStates for each location the player visits. One episode of my game will create a bunch of FlxSubStates, then at the end, try to clean up their memory by calling .close() directly on each one. Currently, only the last scene in the episode (the one that currently == _parentState.subState) would actually have its closeCallback called. I have destroySubStates set false on the parent state because scenes will get revisited eventually and I want the sprites to remain in them. So, when I call .close() on each scene's state, I'm actually expecting both the closeCallback to get called, and for destroy() to get called on them, to free the memory they used. Although on that last point, I could call .destroy() myself.

NQNStudios commented 2 years ago

Maybe FlxSubState could have a Bool field, destroyOnClose, which determines whether to call destroy after calling the closeCallback.