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

Ergonomic pause functionality #1353

Closed
alice-i-cecile opened this issue Jan 29, 2021 · 18 comments
Closed

Ergonomic pause functionality #1353

alice-i-cecile opened this issue Jan 29, 2021 · 18 comments
Labels
A-ECS Entities, components, systems, and events C-Enhancement A new feature
Milestone

Comments

@alice-i-cecile
Copy link
Member

alice-i-cecile commented Jan 29, 2021

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

We can disable systems using run criteria (or StateStages) to pause the game. Unfortunately, if you simply disable the systems using StateStage, any events (and change detection) changes that are currently in flight are lost.

What solution would you like?

A pair of methods to pause and unpause a system set, activated via commands. Under the hood, this should implement the work-around listed below, automatically caching any events and changes that the systems in the system set consume.

Under the hood, this might makes sense to be a State, and use the enter / exit logic to handle this.

What alternative(s) have you considered?

To get around this, you could write some logic to handle system entry and exit, but then you need to repeat it for every relevant event type, and also muck around by manually caching component flags (but only the ones that are used by the paused systems?) and then reloading them

Additional context

This relates to #68, #1157 and #1297. This issue was created in response to this comment in #1345 .

@tigregalis
Copy link
Contributor

[...] Unfortunately, if you simply disable the systems using StateStage, any events (and change detection) changes that are currently in flight are lost.

How does this work? Is this intentional?

@bjorn3
Copy link
Contributor

bjorn3 commented Jan 30, 2021

Events are buffered for exactly one frame. Change detection flags are cleared at the end of the frame. One of the proposals at #68 would fix the problem for change detection. As far as I know there is nothing for events (yet).

@alice-i-cecile
Copy link
Member Author

Events are buffered for exactly one frame. Change detection flags are cleared at the end of the frame. One of the proposals at #68 would fix the problem for change detection. As far as I know there is nothing for events (yet).

You can manually handle Events' survival by adding them as a Resource, rather than using add_event. That definitely gets a bit tedious though and if you do it wrong the amount of memory allocated slowly creeps up forever.

@karroffel karroffel added A-ECS Entities, components, systems, and events C-Enhancement A new feature labels Jan 30, 2021
@cart
Copy link
Member

cart commented Jan 30, 2021

As far as I know there is nothing for events (yet).

Yeah theres nothing that solves this problem for events yet. Some options:

  • Export an unbounded crossbeam channel for cases where that is needed. Afaik that only supports a single consumer of a given event (there can be multiple consumers, but only one will get the event), which is very different from bevy's "multiple consumers of the same event" model.
  • Give people the option to opt out of "automatic event buffer flips", which would basically make the event queues unbounded. Users would be responsible for flipping at the appropriate time to ensure memory doesn't grow forever.
  • Add a new event type with "subscribers". It would be very similar to the current implementation. The only major difference is that the event would know when all "subscribers" have received a given event. It would then free the event.

The last one feels like a reasonable approach that could be used everywhere.

@alice-i-cecile
Copy link
Member Author

Add a new event type with "subscribers". It would be very similar to the current implementation. The only major difference is that the event would know when all "subscribers" have received a given event. It would then free the event.

This is a very interesting idea, especially as a standard behavior. You could extend it to support "consuming events" nicely, and I wonder if you could use it to enable #1273. The main drawback I could see is that it would complicate adding systems dynamically mid-execution, but I don't think that's impossible to get around.

I'm partial to the idea of making change detection and events both robust to systems not being run: it seems much more elegant than implementing a special pause method or the like.

@bjorn3
Copy link
Contributor

bjorn3 commented Jan 30, 2021

The main drawback I could see is that it would complicate adding systems dynamically mid-execution, but I don't think that's impossible to get around.

SystemParam could have a method that gets called when a system gets registered which uses this SystemParam. This method could then register the system as subsriber to the event stream.

@Ratysz
Copy link
Contributor

Ratysz commented Jan 31, 2021

An implementation needed for the subscriber approach can be seen in the shrev crate.

@fenduru
Copy link

fenduru commented Feb 20, 2021

👍 for storing events in a RingBuffer and tracking consumers to know when to drop events.

It seems like there is kind of a general pattern between this and change detection where basically the system wants to apply its own state to some resource/component before passing a view of the data to the user's function. For instance:

  • Component stores change timestamps/counters
  • System maps that to Flags based off of the timestamp of its last run
  • Function the user wrote gets to consume Flags::changed: bool just like they can today

Or for events

  • Resource stores the event buffer
  • System maps that to a reader using its own state, i.e. events.get_reader(self.id) // events tracks consumers by system ID
  • Function the user wrote gets to consume EventReader just like they can today

I think there is an opportunity to implement this generally enough to support various cases like this, but I'm not familiar enough with the inner workings of bevy to know exactly how to accomplish this.

@Davier
Copy link
Contributor

Davier commented Feb 21, 2021

Would it be possible for the scheduler to calculate the number of EventReader<T> and store it in Events<T>?
If so, we could copy that number into any new EventInstance<T>, decrement it when a reader reads the event, and drop the event when it reaches 0.
That way we don't have to register and map the event readers like in shrev.

@fenduru
Copy link

fenduru commented Feb 21, 2021

@Davier don't you still need a way for each consumer to individually track what events they've seen? Otherwise systems that run every tick will see repeated events as long as they're kept alive from systems that haven't run yet. Also seems like excess memory to track things on an event-by-event basis rather than just tracking an index.

@Davier
Copy link
Contributor

Davier commented Feb 21, 2021

don't you still need a way for each consumer to individually track what events they've seen?

Event readers already do this by saving the event counter, what I suggested builds on top of it.

Also seems like excess memory to track things on an event-by-event basis rather than just tracking an index.

Yes, if we register the event readers we can have a system that periodically checks their event counter and drops events, avoiding the extra memory per event. I think that it's a trade-off between memory and execution time.

@YohDeadfall
Copy link
Member

There's another ongoing discussion where one of proposals replaces the flag by the generation number field exactly to solve that kind of issue, events won't be lost in that case. Would not it solve this problem too?

@Davier
Copy link
Contributor

Davier commented Mar 2, 2021

There's another ongoing discussion where one of proposals replaces the flag by the generation number field exactly to solve that kind of issue, events won't be lost in that case. Would not it solve this problem too?

Currently events are not components, they are stored in their own buffers. It could be great to have each event instance be an entity and the event type a component, but the problem is that events need to be sent instantly while entities are spawned at the end of the stage.
Edit: that doesn't solve the question of when to drop them though.

@alice-i-cecile
Copy link
Member Author

Currently events are not components, they are stored in their own buffers. It could be great to have each event instance be an entity and the event type a component, but the problem is that events need to be sent instantly while entities are spawned at the end of the stage.

You could get around this by spawning an event entity on startup when the event is initialized. The larger issue, IMO, is that by making them entities with components, you lose the ability to write to events from multiple systems at once by appending to the end of the buffer.

@huhlig
Copy link
Contributor

huhlig commented Apr 5, 2021

Additional use case example. I would like to build a universe simulation with an in game clock. The Res<WorldTime> would need to be in sync with Res<Time> but could be paused or accelerated. While certain systems may still be run for things like animation, input handling, etc and use realtime. Certain other simulations like physics, orbital mechanics, day night cycles, etc would be tied to the WorldTime, especially ones which use that time to control certain aspects of their updates or component mutations. It would be nice if Time and WorldTime were synced within the engine and ergonomic to use.

@alice-i-cecile alice-i-cecile added the S-Needs-RFC This issue or PR is particularly complex, and needs an approved RFC before it can be merged label Dec 12, 2021
@alice-i-cecile
Copy link
Member Author

Tied up in the life-cycle / states / scheduling designs discussed in #2801.

@alice-i-cecile
Copy link
Member Author

Looking at the implementation and demo of #8063, I think we can very lightly tweak the tools added there to get this basically for free!

@alice-i-cecile alice-i-cecile added this to the 0.11 milestone Mar 12, 2023
@alice-i-cecile alice-i-cecile removed the S-Needs-RFC This issue or PR is particularly complex, and needs an approved RFC before it can be merged label Jun 19, 2023
@alice-i-cecile alice-i-cecile modified the milestones: 0.11, 0.12 Jun 19, 2023
@alice-i-cecile
Copy link
Member Author

I'm calling this fixed by #8964 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
Status: Needs Design
Development

No branches or pull requests

10 participants