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

Add data init and dispose functions to systems #164

Closed
wants to merge 3 commits into from

Conversation

chemicstry
Copy link
Member

@chemicstry chemicstry commented Jul 24, 2020

This PR is based on the amethyst discord discussion regarding system initialization and dispose.

In amethyst we have a lot of systems that have to initialize resources that they use prior to first execution. This was achieved by passing World and Resources to the system creation function so any required resources could be initialized. It works, but the nested syntax doesn't look too nice. However, there is no way to dispose of those resources once schedule is destroyed. Implementing this would require a big wrapper around all legion types and is really inconvenient. So instead I propose to add these utility function directly into legion.

Here is how a system would look like:

pub fn create_my_system() -> impl Schedulable {
    SystemBuilder::new("my_system")
        .write_resource::<MyResource>()
        .with_init(|_world, resources| {
            resources.insert(MyResource::default());
        })
        .with_dispose(|_world, resources| {
            let my_res = resources.remove::<MyResource>();
            my_res.unwrap().do_some_cleanup();
        })
        .build(|_, _, my_res, _| {
            // do something with my_res
        });
}

Init and dispose functions for all systems are called by Schedule::init and Schedule::dispose in the order of system insertion. It is up to the user to call those functions.

Thread local systems are now a ThreadLocalRunnable trait which has init, dispose and run functions and implements Runnable trait. This simplifies Schedule code and API a bit. Unfortunatelly, I couldn't find a way to make SystemId static, so I used the lazy_static library. Any ideas how to solve this without additional lib?

This is a rough proposal and I'm looking for feedback

@chemicstry
Copy link
Member Author

So I've been trying to incorporate these changes into amethyst and see how it feels. Most of the problems were solved by these changes, but there are still some specific cases which fail with the legion API.

One such case is shrev::EventChannel. When you initialize a system, a ReaderId has to be registered in EventChannel resource. During system execution any reads from the channel have to use this unique ReaderId to fetch events. However, there is no simple way to pass this id variable from init closure to run closure. Only options are Arc<Mutex<T>> and system specific resource containing state, which both are not nice.

All these similar problems are fundamentally caused by systems not being structs and unable to keep state. The struct system syntax was explored in #31 and it doesn't seem possible with the query filter implementation. So the only alternative I see is to emulate similar ergonomics in the closure API.

Init closure would have a definition of FnOne(&mut World, &mut Resources) -> T where T is custom system state. System run closure would get &mut T as an additional parameter. Dispose closure would be defined as FnOnce(&mut World, &mut Resources, T).

This is far from perfect solution, but it's currently the only alternative I can see. I will continue exploring ergonomics of this approach and maybe find some new alternatives.

@TomGillen
Copy link
Collaborator

It seems this PR was automatically closed when I merged the experimental branch into master. You are welcome to re-open this PR targetting the new master.

I am not sure what would be the best way to handle resource initialization and destruction. If we were to build that functionality into legion, then something like this PR looks reasonable to me. However, lifecycle management issues such as when to create/destroy worlds/systems/resources/schedules, and when to run what schedules on what worlds, seems to be like broader game state management that is out of the scope of the ECS to manage.

@lberrymage
Copy link

@chemicstry have you looked at this again recently? I've been messing with legion/shrev myself and have encountered the same issue with registering ReaderIds (cleanly), so I'm curious if you've considered this PR more or if Amethyst has figured out how to resolve this issue on its own.

@chemicstry
Copy link
Member Author

@lberrymage it was implemented with system bundles in amethyst/amethyst#2400. It is a bit inconvenient for single system that needs shrev::EventChannel, though. I think shrev should be refactored/replaced to the way bevy's events work, where you don't need to register reader in advance.

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.

3 participants