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

Shutdown and cleanup missing behavior #14

Open
AnneKitsune opened this issue Nov 10, 2018 · 11 comments
Open

Shutdown and cleanup missing behavior #14

AnneKitsune opened this issue Nov 10, 2018 · 11 comments

Comments

@AnneKitsune
Copy link
Contributor

Currently, when you cleanly quit the game, there is no clean-up actions that can be ran (disconnecting from a server, saving something to a file, etc...)

There should be a way that the engine call the code from the user when the game is shutting down.

There is a method in Application::shutdown which is unused and could be re-used for that purpose.

There are two places (or more?) where exit code could be placed: State and System.

State is annoying, because you will probably end up copy pasting the shutdown method between your states.

System on the other hand would be a bit better since you could have a shutdown() method added to them. However, usually you will want your systems to be stateless, so having a cleanup method isn't really idea here either.

An rfc would need to be opened for discussion by the person taking this issue in charge.
Thanks!

@minecrawler
Copy link

minecrawler commented Nov 12, 2018

What about providing ways to do the shutdown in different places for different things?

I don't really get the shutdown method for a system, since its stateless, however, for example when a state is destroyed, it should clean up its resources. There might be more states alive, though. Then, what happens if all states are destroyed? It's on a different level than systems, too, and it probably should be provided in a central place, like the application builder which also initializes the game.

@AnneKitsune
Copy link
Contributor Author

What I meant is that having shutdown() on a system doesn't make sense, so you can skip that one when designing how to make a shutdown procedure.

@DoumanAsh
Copy link

I don't really get the shutdown method for a system, since its stateless, however, for example when a state is destroyed, it should clean up its resources.

I think it is handled by pair on_start/on_stop?

I think it would be good for application level to have some sorta callback, but for that it would be better to Application or to be precise CoreApplication would become trait itself.
Or as alternative you could have trait parameter that could describe set of callbacks: for example application start and application shutdown as set of static methods that can accept World

pub trait ApplicationHook {
    ///Runs when application starts
    fn on_start(_world: &mut World) {
    }

    ///Runs before application is going to be shutdown
    fn on_shutdown(_world: &mut World) {
    }
}

pub struct DefaultAppHook;
impl trait ApplicationHook for DefaultAppHook {}

It would require new trait parameter and changing Application alias to Application<H: ApplicationHook while providing type DefaultApplication = Application<DefaultAppHook>;

@Rhuagh
Copy link
Member

Rhuagh commented Nov 12, 2018

on_stop is called on all pushed states when the state machine stops, so that's one way. In addition to that adding what @DoumanAsh suggest above should catch all other scenarios tbh. Then again, you can do exactly the same thing simply by pushing a baseline state at the bottom of the stack on startup.

@DoumanAsh
Copy link

@Rhuagh Does amethyst::Trans::Quit triggers on_stop for all State in stack?

@Rhuagh
Copy link
Member

Rhuagh commented Nov 12, 2018

Yes.

            while let Some(mut state) = self.state_stack.pop() {
                state.on_stop(StateData { world, data });
            }

@DoumanAsh
Copy link

Hmm, this is certainly on option instead of making global shutdown. Though I feel like it is not the best option for something global, but as currently there is always only single initial state, I think it is nice alternative to adding anything new.

@AnneKitsune
Copy link
Contributor Author

Ah so I guess this issue is already resolved :D
I thought on_stop wasn't called for all states for some reason.

@SkylineP1g3on
Copy link

is 'all_pushed' always the same as 'all_states'?

@khionu
Copy link
Member

khionu commented Jan 7, 2019

This issue could be repurposed into a documentation issue

@fhaynes
Copy link
Member

fhaynes commented Jan 8, 2019

Moving this to the RFC repo.

@fhaynes fhaynes transferred this issue from amethyst/amethyst Jan 8, 2019
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

7 participants