Wait until FixedUpdate can see events before dropping them

[maniwani]

Objective

Currently, events are dropped after two frames. This cadence wasn't chosen for a specific reason, double buffering just lets events persist for at least two frames. Events only need to be dropped at a predictable point so that the event queues don't grow forever (i.e. events should never cause a memory leak).

Events (and especially input events) need to be observable by systems in FixedUpdate, but as-is events are dropped before those systems even get a chance to see them.

Solution

Instead of unconditionally dropping events in First, require FixedUpdate to first queue the buffer swap (if the TimePlugin has been installed). This way, events are only dropped after a frame that runs FixedUpdate.

Future Work

In the same way we have independent copies of Time for tracking time in Main and FixedUpdate, we will need independent copies of Input for tracking press/release status correctly in Main and FixedUpdate.

--

Every run of FixedUpdate covers a specific timespan. For example, if the fixed timestep Δt is 10ms, the first three FixedUpdate runs cover [0ms, 10ms), [10ms, 20ms), and [20ms, 30ms).

FixedUpdate can run many times in one frame. For truly framerate-independent behavior, each FixedUpdate should only see the events that occurred in its covered timespan, but what happens right now is the first step in the frame reads all pending events.

Fixing that will require timestamped events.

[tim-blackbird]

Doesn't this create the opposite problem where events could be missed outside of FixedUpdate if the fixed timestep is lower than delta time?

[maniwani]

Doesn't this create the opposite problem where events could be missed outside of FixedUpdate if the fixed timestep is lower than delta time?

Hmm, yeah. I guess it really does need to be an indirect signal after all. I'll change it so that FixedUpdate queues the swap and then applies the swap at the beginning of the next frame.

That should work. FixedUpdate will always be <= the rest of the frame in Time.

[alice-i-cecile]

Related to #6183.

[alice-i-cecile]

It might be nice to document the exact behavior here, but given that this is a known-stopgap solution, I don't think it's particularly important to do so.

[hymm]

This should probably be considered a breaking change. If people are using bevy_time, but using their own main schedule that doesn't run fixedupdate, then this will never update events.

Ideally we'd probably have a FixedUpdate plugin that would do all this. Since we don't, I'd like to keep this a focused change and probably just have a migration guide. I imagine that most people are probably using the default schedule setup. So this probably won't effect many if any users.

[OleStrohm]

I think a good addition here might be to check if the time this event was last sent is really long, and in those cases emit a warning that the user may not be calling this method (for the cases where someone turned off FixedUpdate, and is rolling their own version, but didn't know that calling this was a requirement). The time-based approach would ensure that this is notified to the user at a specific interval regardless of how many events are actually sent.

[cart]

(Moving to 0.12.1 so we don't forget to consider it, not because I think it should land there)

[djeedai]

I'd like to propose a possibly controversial alternative: inputs should never be read in a fixed update system.

This is not just a way to fix this, this is a fundamental design remark: inputs are inherently tied to rendering, in the sense that they're polled in the main loop, that most inputs produce visible reactions, and that in AR/VR inputs determine the camera transform. So querying user input in fixed update I think is a design mistake.

Now, I can understand that this is not convenient, in particular when you want physics to react to user inputs. But I think this should be handled by the user on a case by case basis.

Note that Unity has exactly the same issue/limitation. This is by design. Only problem is in Unity this is poorly documented, short of a tangential note:

Note: Input flags are not reset until Update. You should make all the Input calls in the Update Loop.

[djeedai]

Let me add a few more proofs that this is a bad idea:

• as commented above the reverse problem exists
• this is not an input only issue, this affects all events
• we're talking about Update and FixedUpdate but the new time API allows other fixed clocks; how do we handle events for those? We don't know a priori the frequency of those updates.
• since the ratio of frame to fixed frame is variable, this means the event can be:
  • arbitrarily delayed between its observing in Update and its observing in FixedUpdate; does it make sense to read input events from several frames ago? How will your game logic handle reading different contradicting inputs emitted on different updates but read together on fixed update?
  • that delay is variable frame to frame, which makes reasoning and proper handling very difficult. What if input handling is based on some state but he state changed after the update frame but before the fixed update one?

[paholg]

I'd like to propose a possibly controversial alternative: inputs should never be read in a fixed update system.

I'm working on a game where all logic is done in FixedUpdate, and this seems fine to me. It is not hard to store the latest input in Update, and read it in FixedUpdate.

What this does not work for is other events, in particular RemovedComponents. See, for example, bevy_rapier which handles these in Update even when running physics on FixedUpdate: https://github.com/dimforge/bevy_rapier/blob/cec05c3f48dad2404f0343de5dc0033d9b0f3a10/src/plugin/plugin.rs#L221

[maniwani]

(edit: my original tone was too abrasive, sorry about that)

I'd like to propose a possibly controversial alternative: Inputs should never be read in a FixedUpdate system.

This is not just a way to fix this, this is a fundamental design remark: inputs are inherently tied to rendering, in the sense that they're polled in the main loop, that most inputs produce visible reactions, and that in AR/VR inputs determine the camera transform.

So querying user input in FixedUpdate I think is a design mistake.

Input is not "inherently tied to rendering". It's not inherently tied to anything. We don't poll input. Input events are continuously delivered to us from the OS (through winit and gilrs) as they happen. Those events build up and we just happen to drain them into our Events resources once per frame.

I think your argument really has no basis. I think you may be misinformed about how state changes make their way from input devices to the application. I tried to reference the long-term vision in the PR description, but let me also point to some existing context on this issue that you maybe aren't familiar with.

Since you mentioned AR/VR, AR/VR applications want async reprojection, and that requires being able to, at any arbitrary time, read newer input events that showed up in the middle of the frame. (We can't do this without #9122 though.)

Input is really not coupled to frames at all.

Now, I can understand that this is not convenient, in particular when you want physics to react to user inputs. But I think this should be handled by the user on a case-by-case basis.

Thinking that this is a case-by-case problem shows a lack of understanding.

Note that Unity has exactly the same issue/limitation. This is by design. Only problem is in Unity this is poorly documented, short of a tangential note:

Note: Input flags are not reset until Update. You should make all the Input calls in the Update Loop.

Unity's legacy input module has said limitation, but their "new input system" does not (API docs here). Even then, Unity can't handle events properly in both Update and FixedUpdate at the same time (it's one or the other). That'll just be another way Bevy handles time better than Unity does. Some Unity developers have asked for events to be handled how I'm proposing.

In the first place, Unity isn't a particularly well-designed engine. If you're looking for evidence from another engine, look at Godot. They have added a feature addressing this same issue.

Let me add a few more proofs that this is a bad idea: as commented above the reverse problem exists. This is not an input only issue, this affects all events.

This PR is supposed to affect all events. Why do you consider this a problem? It's a clear answer for someone wanting to read an event in FixedUpdate. All events will benefit from this change and from the soon-to-come timestamps.

We're talking about Update and FixedUpdate but the new time API allows other fixed clocks; how do we handle events for those? We don't know a priori the frequency of those updates.

This PR doesn't have a priori knowledge either. If users demanded more clocks, the solution would be the same—keep events alive until they're in the "past" for all clocks.

But frankly, that's an unreasonable demand. The new Time API allowing other clocks to exist was just a footnote. Users don't actually need other clocks because there is no other essential cadence besides "frames" and "ticks".

Since the ratio of frame to fixed frame is variable, this means the event can be arbitrarily delayed between its observing in Update and its observing in FixedUpdate. Does it make sense to read input events from several frames ago?

It's completely sensible. Look at Overwatch's "High Precision Mouse Input" or Counter Strike 2's "Sub-Tick Input".¹ Those work by timestamping input events as they're received (from the OS) so that they can be seen on the correct tick (and even account for the exact moment within the tick something happened), which is exactly what I'm proposing / where I'm trying to get us.

Does that still seem like a bad design?

How will your game logic handle reading different contradicting inputs emitted on different updates but read together on fixed update?

There are no "contradicting" inputs. Input events arrive continuously. Grouping them by tick is just as arbitrary as grouping them by frame. Update and FixedUpdate see the same time elapse, they just experience different spans of it at different times. Accordingly, they should observe the same events, just in different batches and at different times.

That delay is variable frame to frame, which makes reasoning and proper handling very difficult. What if input handling is based on some state but he state changed after the update frame but before the fixed update one?

This PR and what I've stated in the Future Work section is the proper handling. Users won't need to reason about it if it's just transparently done for all events.

(Honestly, with the amount of effort I've invested into Bevy specifically to improve fixed timestep and input, it's really upsetting that I have to spend so much time convincing people that, like, I know what I'm doing. I've written at length about our outstanding issues with input and how to address them. I've even investigated the winit and gilrs side of this. Now that I'm making PRs to finally make progress on these issues, it's frustrating to get a comment saying "let's not fix this" and "this is a bad idea".)

Footnotes

1. These are all fancy titles describing framerate-independent input handling. ↩

[alice-i-cecile]

@maniwani I share your frustration and perspective, but your tone is quite abrasive.

Note that Unity has exactly the same issue/limitation. This is by design

I think that we should make events work, by default, for all users, regardless of the architecture they choose. The existing design silently and sporadically fails, for all events (not just input) that are sent between the main schedule and FixedUpdate. This is not an implementation detail we should burden users with.

The tradeoffs here for fixing this bug are very mild: events persist slightly longer, which may result in a slight bounded increase in the memory their queues use. I'm not immediately clear how one might integrate this work with other types of user-defined Time, but that's largely because I have not seen anyone using them.

Timestamped input events are an important next step, but I am fully on board with @maniwani's vision of how we can solve this problem.

[alice-i-cecile]

Or maybe we "just" need something like leafwing-input-manager, because this is really what users want (or, need), not reading raw input events at all for most user actions

Definitely agree: I'm looking to eventually upstream this. Users should very rarely be reading raw inputs themselves in real games.

[maniwani]

But the most common and expected way is likely for users that inputs are updated each "frame", and the frame concept is defined by the main dynamic clock. That would be different if we emitted input events multiple times per frame/tick/period though. But if we do it once periodically, then "once per frame" is probably the best model, no?

I did not say events would be emitted more than once. A system in Update can already read events one frame late because of run conditions and double buffering. There's really nothing new here.

Yes, but that requires substepping/timestamping as you detailed, and is beyond the scope here, although I agree that this should be the goal.

You might have 2+ frames with the same input, which will likely induce issues like motion sickness. So again I think this is an argument for inputs being ticked against the main clock.

Like I said, XR applications want to read inputs in advance, specifically to reduce motion sickness. That's not even possible for us now, but it will be once we merge #9122.

I don't think enabling "one or the other" can be considered having lifted the limitation.

I was not praising Unity. You brought up a significant shortcoming of their legacy system. They attempted to partially address that limitation in their newer system, but still fell short.

I'm a bit baffled that you're trying to argue that Unity—with all its half-baked systems (e.g. DOTS)—must have had a good reason for this particular thing being half-baked as well. Am I just misunderstanding Unity and its various shortcomings are all actually clever design points that I just don't understand yet?

I'm glad we're aiming for something better (timestamping), but the fact Unity's new input has the same limitation as their legacy one maybe is a hint that this is on purpose rather than by mistake or lack of expertise?

And maybe it was an oversight and I'm right and developing our event handling according to what I propose will be better.

Asking for inputs from both Update and FixedUpdate only makes sense if the set of inputs you're reading in each is disjoint (e.g. read player WASD in FixedUpdate for driving physics, and read ESC key in Update to open a menu).

They're always disjoint though.

(edit: Even if you propose e.g. using the arrow keys for both moving the player and scrolling a menu, you would never have both active at the same time. It's trivial to use a resource or state to control which one is active. With such an obvious fix, I don't buy the footgun argument. There's no need for any complicated "event claiming" system.)

Otherwise you'll need to reconcile both reads, or you're back to losing events.

No events will be lost and there's no need to reconcile things. As I've written in the PR description, the Input resource needs the Time treatment where there's one for FixedUpdate and one for the rest of the schedule.

Or maybe we "just" need something like leafwing-input-manager, because this is really what users want (or, need), not reading raw input events at all for most user actions.

This is pretty apples-to-oranges. Input-action bindings are orthogonal to this discussion about timing.

The new Time API PR introduces 3 clocks.

I am aware of what that PR did (I did help write it). Technically, we had 3 clocks even before the PR; they were just refactored. Time used to be Time<Virtual> and Time<Real> combined, and the other was FixedTime.

There's also a discussion from a Bevy user immediately asking about how to use the API to create a custom clock. I wouldn't call that a footnote, nor would I assume users don't want custom clocks. I do agree this is probably not very common though.

I wouldn't call that a footnote, nor would I assume users don't want custom clocks.

Just me speaking as one of the co-authors, but "users can make their own clocks" was not a big priority. The underlying clock type exists because we wanted to minimize code duplication, and it seemed like there was no harm in making it pub.

I think it's a bit odd for you to place such an emphasis on it when every other engine doesn't even let you do that. Their users are not clamoring for custom clocks. And a single user inquiring about it is not really a hint that there are many others.

That doesn't mean we should break it; if we allow the pattern we should support it.

Actual usage patterns take priority over mostly hypothetical ones.

Events didn't work in FixedUpdate. Users can use systems in FixedUpdate and those systems can read events. That was a clearly a pattern we needed to support.

Likewise, FixedUpdate still fails to fulfill its puporse of providing framerate-independent behavior. It's event handling is still wrongly coupled to framerate. This is a clear problem that needs to be fixed, but one you do not seem to acknowledge.

I'm talking about an API returning non-timestamped events from 2+ frames ago, which will be very hard for a lot of users to interpret and process against the current game state, since they don't know when that event was first emitted (which frame), do they?

I agree timestamping fixes the issue, but in the meantime without timestamps the API will be hard to use.

Users will not need to interpret or know any of that. They can write systems that read events normally. There's nothing "hard to use" about it. I really don't see your POV here. From my POV, there is no additional cognitive burden being placed on users.

This is what I meant by "contradicting events"; if an event says to move one way (in frame N) and another says to move the other way (in frame N+1), but you don't know which frame(s) they were emitted from because you're reading them both in a later FixedUpdate, then they may contradict each other when applied to the current game state (which is wrong; they should be applied to the state at the frame they were captured; but you can't do that without knowing the frame or timestamp).

You are talking about frames in the context of FixedUpdate, which exists to provide framerate-independent behavior, i.e. these systems explicitly do not need to care about the frame something happened.

I don't think this was a productive discussion.

[rlidwka]

About custom clocks (and schedules)

There's also a discussion from a Bevy user immediately asking about how to use the API to create a custom clock. I wouldn't call that a footnote, nor would I assume users don't want custom clocks.

I'm the guy who asked for a custom clock. Let me explain my use-case here.

I was connecting physics engine with bevy (that's bevy_mod_physx now) for a particular project.

It needed to be:

• very modular (with ability add custom logic in between physics calculations - custom code within each substep is something rapier can't do as far as I found)
• with the ability to control speed and (importantly) pause the entire thing

I arrived at a custom schedule (called PhysicsSchedule) as a solution (you can maybe argue for CustomSet.run_if() inside FixedUpdate, but it didn't exist yet in bevy 0.10). I can all the custom logic there, and run that schedule whenever I want, thus adding the required flexibility. Then custom time was added to keep track of time.

Looking back, what I've got is just slightly different FixedUpdate and Time<Fixed>, implemented in user-land. With one important exception: I can pause it.

About this actual PR

I'm going to agree with @djeedai here: this is a bad idea.

• if you emit events from FixedUpdate, they are going to be missed (withdrawn, see below)
• if you disable FixedUpdate schedule, events are going to pile up infinitely

This is not solving the problem, it's a hacky workaround. (withdrawn, see below)

There's also .run_if() conditions that force you to miss events. A good idea would be to fix all those as well, but I don't know whether that's feasible.

Thinking about this some more, we've got 2 options:

Option 1: tie events to schedules

app.add_event::<EventOne>();
app.add_fixed_event::<EventTwo>();

// this would panic:
app.add_systems(FixedUpdate, |_: EventReader<EventOne>| {})

This is a simple and stupid solution. Downside: .run_if() still misses events.

Option 2: get creative

1. Maybe track how many systems requested access to a particular event. Based on that, we can potentially calculate whether every system received the event or not. Drop when all of them did.

2. Or make event storage a circular buffer, and only clean events when new ones are pushed.

or some combinations of those

but I'm sure that was all considered before

[maniwani]

• If you emit events from FixedUpdate, they are going to be missed.

No, that's not correct. If you emit an event in FixedUpdate it will be queued in the front buffer. The front buffer will be swapped to the back buffer at the beginning of the next frame, so those events will be sitting pretty in that back buffer (both buffers are visible to readers) until after the next frame that runs FixedUpdate. So the next FixedUpdate will see those events before it queues another swap, and then those events will finally be dropped on the frame after.

This PR improved upon the status quo by making it so events are only "missed" when they're ignored by run_if.

• If you disable FixedUpdate schedule, events are going to pile up infinitely.

True, but it's easy enough for users who are already doing such a fancy thing to either remove this new signalling resource or schedule the signalling system inside their custom schedule.

tie events to schedules
This is a simple and stupid solution.
Downside: .run_if() still misses events.

I'll give you simple and ill-advised. But it's not a solution. I won't support that. It was like the second idea I considered and rejected a year ago. Because it doesn't really solve anything. Us just pretending the problem doesn't exist would be silly.

(edit: FixedUpdate aside, Main is composed of different schedules. I don't think we can have a situation where Update can't read events emitted by systems in PreUpdate. Or maybe a user has an exclusive system in Update with a big match clause that runs a different schedule under different conditions. It'd suck if those schedules couldn't read events from the rest of the frame. And singling out FixedUpdate would require tracking the "call stack" of schedules. Or what if someone calls the same schedule in both FixedUpdate and Update?)

This is not a problem that can (or should) be fixed by redefining it.

I'm sure that was all considered before.

Yes, I did consider all of this.

Buffered events have never claimed to be reliable (and they clearly can't be without leaving room for a memory leak). Waiting for an event to be read by all systems with the corresponding reader will suffer from the opposite problem of one forgotten run_if causing events to never be dropped.

I'm sure it feels like such a longstanding issue shouldn't be fixable by a tiny PR like this, but this is the most concise, least "hacky" solution to this issue that I landed on after months. And the most substantial criticism thus far has been that it doesn't just work seamlessly with the brand-new thing that just came out.

When we introduce timestamps, we can make it so that only events with timestamps that are in the past from the POV of the "most behind" Time registered get dropped. (That has been my plan from the start.) It won't harm y'all to trust the process.

(Also using double buffering vs. a single ring buffer can be pretty functionally equivalent. Double buffering will use more memory but it can be cheaper to drop the events.)

[rlidwka]

If you emit events from FixedUpdate, they are going to be missed.

No, that's not correct.
This PR improved upon the status quo by making it so events are only "missed" when they're ignored by run_if.

Sorry, you're correct there. Then, this PR is actual improvement, besides the potential infinite buffering case (which we should have a warning for in the future).

tie events to schedules

Main is composed of different schedules. I don't think we can have a situation where Update can't read events emitted by systems in PreUpdate.

Read that as independent schedules. In Update and PreUpdate case, all events there can be tied to Main (as long as we can ensure that Update is only called by Main, and never by user). Aside: I'm still thinking in old paradigms when Update and PreUpdate were base system sets.

But if someone calls the same schedule from Update and FixedUpdate, that schedule would be isolated (can only read its own events, and its own events can only be read by it). If that's an issue, then this solution is not adequate.

Buffered events have never claimed to be reliable (and they clearly can't be without leaving room for a memory leak).

They can be reliable (assuming you have a certain definition of reliability):

1. you introduce limits (keep no more than X events AND no event can be stored for more than Y seconds).
2. raise a warning when those limits are reached
3. these limits can be flexibly defined by the user
   • either per event (define limits when you define event)
   • or per-event-reader, like struct<E: Event, L: Limits> LimitedEventReader;, with default EventReader having some sane values

Those "limits" are called QoS policies, and that's well-developed topic in enterprise systems. "No more than X events" is called Depth, "no more than Y seconds" is called Lifespan. If you are looking to pitch a startup for investors, that's a very profitable idea!

This can in theory solve all EventReader problems! In practice, users won't understand it, limits will be misconfigured, memory will be leaked, but we would blame the users. It isn't worth it just for run_if's (and custom conditional schedules), but maybe we can get some ideas there.

When we introduce timestamps, we can make it so that only events with timestamps that are in the past from the POV of the "most behind" Time registered get dropped. (That has been my plan from the start.)

Okay, sounds great.

[Elogain]

FWIW this change def has impact on certain ways you were able to use events to move the camera for example. I just updated from 0.11 to 0.12.1 and was wondering why my camera suddenly ended up in weird positions.

Its not he prettiest code but it got stuff done:
add_systems(Update, mouse_motion.run_if(in_state(GameState::BallGame).and_then(input_pressed(MouseButton::Left))))

Inside the mouse_motion system the EventReader<MouseMotion> was read and I was quite surprised to see a whole pile of mouse movements stacked up, making the camera ending up at some weird position due to applying lots of undesired movements. I do understand the desire to persist events longer, but it is a big breaking change.

[CaptainOachkatzl]

FWIW this change def has impact on certain ways you were able to use events to move the camera for example. I just updated from 0.11 to 0.12.1 and was wondering why my camera suddenly ended up in weird positions.

Its not he prettiest code but it got stuff done: add_systems(Update, mouse_motion.run_if(in_state(GameState::BallGame).and_then(input_pressed(MouseButton::Left))))

Inside the mouse_motion system the EventReader<MouseMotion> was read and I was quite surprised to see a whole pile of mouse movements stacked up, making the camera ending up at some weird position due to applying lots of undesired movements. I do understand the desire to persist events longer, but it is a big breaking change.

i have a very similar setup for mouse wheel input. i switch between scrolling and zooming with keyboard presses. every time the buttons are pressed, all the mouse wheel events are replayed and the camera ends up in wild positions.

  mouse_wheel_input
      .pipe(zoom_game)
      .run_if(zoom_key_pressed),
  mouse_wheel_input
      .pipe(scroll_game)
      .run_if(scroll_key_pressed),