Update to latest legion and implement new dispatcher

[chemicstry]

Sorry for a big one, but this due to changes to the way dispatcher and system bundles work.

There was a big discussion on discord about system data initialization/disposal (resources and/or entities). It was decided that systems should not contain any initialization logic and it should be solely left to system bundles. Systems are now simple fn build() -> impl Runnable functions instead of the horrible fn build() -> Box<dyn FnOnce(&mut World, &mut Resources) -> Box<dyn Schedulable>>. New legion codegen macros can be used as well. This is how SystemBundle looks now:

/// A SystemBundle is a structure that adds multiple systems to the [Dispatcher] and loads/unloads all required resources.
pub trait SystemBundle {
    /// This method is lazily evaluated when [Dispatcher] is built with [DispatcherBuilder::build].
    /// It is used to add systems or bundles (recursive) into [Dispatcher]. [World] and [Resources] are
    /// also provided to initialize any entities or resources used by the system.
    fn load(
        &mut self,
        world: &mut World,
        resources: &mut Resources,
        builder: &mut DispatcherBuilder,
    ) -> Result<(), Error>;

    /// This method is called once [Dispatcher] is disposed. It can be used to cleanup entities or resources from ECS.
    fn unload(&mut self, _world: &mut World, _resources: &mut Resources) -> Result<(), Error> {
        Ok(())
    }
}

This solves most of the problems that were present previously and also reduces code size considerably. This introduces some inconveniece for simple systems that use shrev::EventChannel, because a SystemBundle has to be implemented for it to register ReaderId. But I believe these ergonomic issues can later be solved with a macro how it was done in specs.

Dispatcher currently does not use ArcThreadPool as this is blocked on a fix in legion. It will be resolved soon.

Material example builds & works. Other already ported crates and examples will follow shortly.

[chemicstry]

One thing I forgot to mention is the removal of stages and instead using the legion's way of scheduling. Systems are scheduled arbitrarily, but they respect data dependency order. For example if your system writes Position and rendering system reads Position, and you do dispatcher_builder.add_system(my_system()).add_system(rendering_system()) they will never violate their order because rendering system has data dependencies on Position.

I think this greatly improves code readability as you can observe the full execution order by just looking at dispatcher builder. With stages there was no way to see system order without going into bundle definitions and looking at what stage they are executed.

Another thing is that you can easily reorder systems by just swapping add_system (or add_bundle) lines in dispatcher builder. With stages this was done with RelativeStage and you had to count priorities. Adding system in between meant rescaling all priority numbers to make space, which imo was tedious.

There is one regression though. System bundles now have no way to specify system insertion order. For example if there was RenderingAndInputBundle it would have no way to insert input system at the beginning and rendering system at the end. However, I didn't observe any such bundles in amethyst and worst case they can be split up into two.

Any objections?

[chemicstry]

I believe this is ready for review now. Updated all ported crates and material, fly_camera, arc_ball_camera examples.

Everything (that is ported so far) compiles without warnings

[khionu]

I don't have time to review this until the weekend, but because of the size, it should have at least two sets of eyes give the entire thing a review. These reviews don't need to guarantee correctness, but they should make an attempt to find any obvious issues.

[AnneKitsune]

Wow, very impressive work!
I have a few remarks, but nothing major.
Also, I think all the dispatcher stuff should be moved into a separate crate. legion_dispatch maybe?
Thanks a lot! ♥️

[chemicstry]

Wow, very impressive work!
I have a few remarks, but nothing major.
Also, I think all the dispatcher stuff should be moved into a separate crate. legion_dispatch maybe?
Thanks a lot! ♥️

Thanks!

Dispatcher is just a single file so I don't know if it's worth moving it into a crate?

All the EventChannel<Event> unwraps should not fail if you use Application, because it is inserted before anything else. It is possible to change it to get_mut_or_default, but then it would silently wont work if you forgot to insert your custom input system. Maybe I could just add expect everywhere.

[AnneKitsune]

Dispatcher is just a single file so I don't know if it's worth moving it into a crate?

It is very convenient, so I'm sure other legion users would love it.
SystemBundle could even be placed in that new crate. :3

All the EventChannel unwraps should not fail if you use Application [...]

Exactly, we can't know if the user will use application or not. We should handle the insertion for them in the case where they don't, instead of crash.

[chemicstry]

Exactly, we can't know if the user will use application or not. We should handle the insertion for them in the case where they don't, instead of crash.

We cannot handle all the custom use cases. If user does not use Application then it's up to him to do much more than just inserting window event channel. I think that inserting default is much worse, because instead of crashing and telling you what is missing it would just silently continue and you would start pulling your hair out why no events are coming.

[chemicstry]

It is very convenient, so I'm sure other legion users would love it.
SystemBundle could even be placed in that new crate. :3

What about moving it into amethyst_dispatcher? There is still the dependency of amethyst_error though

[AnneKitsune]

Good points, let's go with how it is currently 👍

[chemicstry]

Ping @khionu

[khionu]

This PR is pretty big. Since CI is just around the corner, I'd want to wait for that and have it run on it. That, and the handful of concerns I noted, and it looks pretty good!

[chemicstry]

CI wouldn't work on legion branch yet because there is still a lot of thinngs to do: update all remaining crates, examples, inline examples, book.

Also merge with master is quite big and I reserved another PR for that

[AnneKitsune]

Yeah definitely don't merge master into legion before most of the legion port is completed.

[chemicstry]

Merging master is not necessarily bad and is probably easier now since not everything is ported yet.

I just don't want to do that in this PR as it is already big.

[khionu]

Even if it's not going to work, CI on the Legion branch should happen soon, so we can have some indicator of progress. CI on the Legion branch needs to pass before it's merged.