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

Event Stages #1041

Closed
alec-mccormick opened this issue Dec 10, 2020 · 5 comments
Closed

Event Stages #1041

alec-mccormick opened this issue Dec 10, 2020 · 5 comments
Labels
A-ECS Entities, components, systems, and events C-Enhancement A new feature

Comments

@alec-mccormick
Copy link

alec-mccormick commented Dec 10, 2020

What problem does this solve or what need does it fill?

Systems that consume events currently must do so using EventReader to iterate through unprocessed events. It would be more intuitive if systems instead could be run only when an event occurs and passed that event as input. The changes proposed in #1021 make this possible with Stages and RunCriteria.

Describe the solution would you like?

We should add in a struct EventStage or EventSystemSet to represent a collection of systems that should be run when an event is fired. The stage would check if any events have been fired since the last frame as its run criteria, and trigger each system to run one time for each event.

Here is a working implementation of this feature forked off #1021.

let stage = EventStage::<SomeEvent>::default()
    .with_system(event_handler_system_a)
    .with_system(event_handler_system_b);

app.add_stage_after(stage::UPDATE, stage);

This code is essentially equivalent to:

let stage = SystemStage::parallel()
  .with_run_criteria(run_criteria::<T>)
  .with_system(next_event_system.chain(event_handler_system_a))
  .with_system(next_event_system.chain(event_handler_system_b));

app.add_stage_after(stage::UPDATE, stage);

// Returns ShouldRun::YesAndLoop as long as there remain unprocessed events.
fn run_criteria<T>(mut reader: Local<EventReader<T>>, events: Res<Events<T>>) -> ShouldRun {
    if reader.earliest(&events).is_some() {
        ShouldRun::YesAndLoop
    } else {
        ShouldRun::No
    }
}

// Pops off the next event and returns it. option.unwrap() should be okay here because this is only run if run_criteria() is met.
fn next_event_system<T>(mut reader: Local<EventReader<T>>, events: Res<Events<T>>) -> T {
    reader.earliest(&events).unwrap().clone()
}

Describe the alternative(s) you've considered?

There are a few issues to address with the above implementation and in general with adding in this feature.

  1. Event references across systems As I understand it, system inputs are required to be 'static, which causes obvious problems with attempting to pass a reference of an event from one system to another. I attempted to solve this by storing the events internally as Arc<T> and force systems to take In<Arc<T>>, however this did not mesh well with Events.drain() so I imagine this is not quite the right solution. I opted to just add a restriction of Clone to the event types for now to get the EventStage functional, but I'm sure there's a better solution. We could just use Arc<Event> instead of Event in our applications but it's easily prone to error and doesn't seem like a decision that should be left up to the end user.

  2. SystemStageExecutor Currently every system added to EventStage is added as next_event_system.chain(event_handler_system), however this creates a new reader for every system added. I imagine it would be slightly more efficient to read the event once from EventStage and pass it in to the executor, however SystemStageExecutor does not currently allow for any input to be passed in to its systems. It shouldn't be too difficult, but my attempts ended up thwarted by downcast!

Additional context

It is also worth discussing the possibility of composing events together. It would be very useful to be able to create system sets that run when multiple events are fired. However, I think it is debatable whether this should be an aspect of EventStages or instead an aspect of the event system itself (by creating event streams composed of other event streams). Event streams could also be composed together in different ways, say by retaining the latest value for each stream and firing them all together.

I have here an implementation and working example for AnyEventStage, which runs its systems if any of its 3 events fire.

@alec-mccormick alec-mccormick changed the title Event driven system input Event Stages Dec 10, 2020
@alice-i-cecile
Copy link
Member

This seems like a useful abstraction: handling events in a sequence of systems definitely seems valuable.

EventSystemSet seems like a better way to handle this than EventStage. You might not always want these to run in their own unique stage, and if you do, you can always just stick them in a stage you initiated.

From an ergonomic perspective, is it possible to write this as a modifier to systems / system sets in the same way as .with_run_criteria? I think the builder pattern is clearer here than directly exposing types, and might give us more room to refactor under the hood as the need arises.

@alec-mccormick
Copy link
Author

That is a very interesting point. Maybe instead this sort of functionality could be expressed like:

SystemStage::parallel()
  .with_system(event_handler_system.with_event_input::<MyEvent>())
  .with_system(event_handler_system.with_event_input::<MyEvent>());

In this case an EventSystemSet/Stage wouldn't be necessary at all.

@cart
Copy link
Member

cart commented Dec 10, 2020

I don't think a manual type on with_event_input is even required. It can be inferred from the input type.

We could implement an IntoEventSystem<T> trait for any System<In = T, Out = ()>

Then you could just do this:

app.add_system(handler.event_system());

fn handler(In(event): In<MyEvent>, something: Res<Something>) {
}

@Moxinilian Moxinilian added A-ECS Entities, components, systems, and events C-Enhancement A new feature labels Dec 10, 2020
@alice-i-cecile
Copy link
Member

I think this is a particular design that could be used to solve #2192.

@james7132
Copy link
Member

With stageless (#6587) soon to land in main via #7267, this issue may no longer be relevant. The goal of trying to limit systems to run only when events may still require work like #2192 or event-specific run conditions. Closing this for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Enhancement A new feature
Projects
None yet
Development

No branches or pull requests

5 participants