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

Wait until FixedUpdate can see events before dropping them #10077

Merged
merged 5 commits into from
Nov 26, 2023

Conversation

maniwani
Copy link
Contributor

@maniwani maniwani commented Oct 10, 2023

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
Copy link
Contributor

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
Copy link
Contributor Author

maniwani commented Oct 10, 2023

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 alice-i-cecile added C-Bug An unexpected or incorrect behavior A-ECS Entities, components, systems, and events A-Time Involves time keeping and reporting labels Oct 10, 2023
@alice-i-cecile alice-i-cecile added this to the 0.13 milestone Oct 10, 2023
@alice-i-cecile
Copy link
Member

Related to #6183.

@alice-i-cecile
Copy link
Member

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.

@maniwani maniwani marked this pull request as ready for review October 10, 2023 19:51
@hymm hymm self-requested a review October 10, 2023 20:31
Copy link
Contributor

@hymm hymm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

crates/bevy_app/src/app.rs Outdated Show resolved Hide resolved
@OleStrohm
Copy link

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 cart modified the milestones: 0.13, 0.12.1 Nov 24, 2023
@cart
Copy link
Member

cart commented Nov 24, 2023

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

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Nov 25, 2023
@djeedai
Copy link
Contributor

djeedai commented Nov 26, 2023

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
Copy link
Contributor

djeedai commented Nov 26, 2023

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
Copy link

paholg commented Nov 26, 2023

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
Copy link
Contributor Author

maniwani commented Nov 26, 2023

(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".1 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 alice-i-cecile added the X-Controversial There is active debate or serious implications around merging this PR label Nov 26, 2023
@alice-i-cecile
Copy link
Member

@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.

@djeedai
Copy link
Contributor

djeedai commented Nov 26, 2023

Sorry it looks like I was a bit imprecise with some terms, which caused some confusion. But I think we have the same understanding of the ideal solution.

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.

OS may poll or receive events from hardware, and OS in turn may expose poll or event-based APIs. "Delivered" is probably a better term as you pointed. In any case, the way events are delivered to Bevy is not really important I think for this discussion. 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?

Since you mentioned AR/VR, AR/VR applications want async reprojection.

Yes, but that requires substepping/timestamping as you detailed, and is beyond the scope here, although I agree that this should be the goal. I was thinking more simply as a fallback in the meantime that rendering is ticked against the main dynamic clock, and wants the most recent inputs, so if you read inputs in a different clock (and that clock has a good chance of being at lower frequency, the most common case being physics ticking at 40/30/20 Hz), 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. We could argue some games want to read updates in FixedUpdate only. But the main dynamic clock is probably a safer default.

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).

I don't think enabling "one or the other" can be considered having lifted the limitation. I think you'll agree this doesn't change anything.; you just swap the Update and FixedUpdate terms and the discussion is more or less the same. 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? My take is that they don't want to add internal complexity for the sake of also adding usage complexity for users; 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); otherwise you'll need to reconcile both reads, or you're back to losing events. And that's I think hard to explain to users. Having users handle all inputs in one or the other is safer, and as @paholg mentioned not that more difficult.

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".

The new Time API PR introduces 3 clocks. 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. That doesn't mean we should break it; if we allow the pattern we should support it.

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".1 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.

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. 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). Or do we have "frame timestamp" with those events? I don't think we do.

I think I still have concerns that the current change might steer people into the wrong pattern(s) and increase the risk of footgun. Saying we fix a bug by avoiding lost events when read from FixedUpdate sounds nice, but the users should be made to understand why this "bug" exists in the first place (dynamic vs. fixed clocks). And what this change also does for me is:

  1. Keep the bug and make it more obscure by fixing the common case (fixed clock) and leaving it broken for other cases (custom clocks).
  2. Encourage (or at least, not prevent) users to read input events "from multiple clocks", which will have those users shoot themselves in the foot. I understand that this enables a convenient pattern where disjoint inputs are read where they're needed. But that also enables reading the same inputs twice. And to that respect, Unity's "one or the other" protects users from that footgun.
  3. Make the internals of event management even more complex, by keeping events alive for "a dynamically changing duration based on the relative ticks of a dynamic and a fixed timestamp", which is hard to reason about to make sure the Bevy implementation is correct.

Instead, we could be educating users (via docs and/or helpers and/or API limitations) on how to best handle input events by reading and processing them all from a single system against a single clock, possibly multiple times per frame for advanced usage, and then derive from there any logic which might affect systems ticking against other clocks (like physics simulation). As @paholg confirmed, this is not very difficult to do. 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. The complex and precise input interactions mentioned are very specific to some genres and some particular user actions (player movement) and I think can perfectly be implemented without reading events from multiple clocks.

No need to block this PR on me. I don't think I have any consensus here. But I wanted to make sure some of those drawbacks are raised.

@alice-i-cecile alice-i-cecile removed the X-Controversial There is active debate or serious implications around merging this PR label Nov 26, 2023
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Nov 26, 2023
@alice-i-cecile
Copy link
Member

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.

Merged via the queue into bevyengine:main with commit 53919c3 Nov 26, 2023
22 checks passed
@maniwani
Copy link
Contributor Author

maniwani commented Nov 27, 2023

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
Copy link
Contributor

rlidwka commented Nov 27, 2023

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
Copy link
Contributor Author

maniwani commented Nov 28, 2023

  • 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
Copy link
Contributor

rlidwka commented Nov 28, 2023

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.

cart pushed a commit that referenced this pull request Nov 30, 2023
## 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.

---------

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
@Elogain
Copy link

Elogain commented Jan 28, 2024

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
Copy link

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), 

github-merge-queue bot pushed a commit that referenced this pull request Feb 2, 2024
# Objective

Fix an issue where events are not being dropped after being read. I
believe #10077 introduced this issue. The code currently works as
follows:

1. `EventUpdateSignal` is **shared for all event types**
2. During the fixed update phase, `EventUpdateSignal` is set to true
3. `event_update_system`, **unique per event type**, runs to update
Events<T>
4. `event_update_system` reads value of `EventUpdateSignal` to check if
it should update, and then **resets** the value to false

If there are multiple event types, the first `event_update_system` run
will reset the shared `EventUpdateSignal` signal, preventing other
events from being cleared.

## Solution

I've updated the code to have separate signals per event type and added
a shared signal to notify all systems that the time plugin is installed.

## Changelog

- Fixed bug where events were not being dropped
tjamaan pushed a commit to tjamaan/bevy that referenced this pull request Feb 6, 2024
# Objective

Fix an issue where events are not being dropped after being read. I
believe bevyengine#10077 introduced this issue. The code currently works as
follows:

1. `EventUpdateSignal` is **shared for all event types**
2. During the fixed update phase, `EventUpdateSignal` is set to true
3. `event_update_system`, **unique per event type**, runs to update
Events<T>
4. `event_update_system` reads value of `EventUpdateSignal` to check if
it should update, and then **resets** the value to false

If there are multiple event types, the first `event_update_system` run
will reset the shared `EventUpdateSignal` signal, preventing other
events from being cleared.

## Solution

I've updated the code to have separate signals per event type and added
a shared signal to notify all systems that the time plugin is installed.

## Changelog

- Fixed bug where events were not being dropped
github-merge-queue bot pushed a commit that referenced this pull request Jun 12, 2024
…13808)

# Objective

As discovered in
Leafwing-Studios/leafwing-input-manager#538,
there appears to be some real weirdness going on in how event updates
are processed between Bevy 0.13 and Bevy 0.14.

To identify the cause and prevent regression, I've added tests to
validate the intended behavior.
My initial suspicion was that this would be fixed by
#13762, but that doesn't seem to
be the case.

Instead, events appear to never be updated at all when using `bevy_app`
by itself. This is part of the problem resolved by
#11528, and introduced by
#10077.

After some investigation, it appears that `signal_event_update_system`
is never added using a bare-bones `App`, and so event updates are always
skipped.

This can be worked around by adding your own copy to a
later-in-the-frame schedule, but that's not a very good fix.

## Solution

Ensure that if we're not using a `FixedUpdate` schedule, events are
always updated every frame.

To do this, I've modified the logic of `event_update_condition` and
`event_update_system` to clearly and correctly differentiate between the
two cases: where we're waiting for a "you should update now" signal and
where we simply don't care.

To encode this, I've added the `ShouldUpdateEvents` enum, replacing a
simple `bool` in `EventRegistry`'s `needs_update` field.

Now, both tests pass as expected, without having to manually add a
system!

## Testing

I've written two parallel unit tests to cover the intended behavior:

1. Test that `iter_current_update_events` works as expected in
`bevy_ecs`.
2. Test that `iter_current_update_events` works as expected in
`bevy_app`

I've also added a test to verify that event updating works correctly in
the presence of a fixed main schedule, and a second test to verify that
fixed updating works at all to help future authors narrow down failures.

## Outstanding

- [x] figure out why the `bevy_app` version of this test fails but the
`bevy_ecs` version does not
- [x] figure out why `EventRegistry::run_updates` isn't working properly
- [x] figure out why `EventRegistry::run_updates` is never getting
called
- [x] figure out why `event_update_condition` is always returning false
- [x] figure out why `EventRegistry::needs_update` is always false
- [x] verify that the problem is a missing `signal_events_update_system`

---------

Co-authored-by: Mike <mike.hsu@gmail.com>
mnmaita pushed a commit to mnmaita/bevy that referenced this pull request Jun 14, 2024
…evyengine#13808)

As discovered in
Leafwing-Studios/leafwing-input-manager#538,
there appears to be some real weirdness going on in how event updates
are processed between Bevy 0.13 and Bevy 0.14.

To identify the cause and prevent regression, I've added tests to
validate the intended behavior.
My initial suspicion was that this would be fixed by
bevyengine#13762, but that doesn't seem to
be the case.

Instead, events appear to never be updated at all when using `bevy_app`
by itself. This is part of the problem resolved by
bevyengine#11528, and introduced by
bevyengine#10077.

After some investigation, it appears that `signal_event_update_system`
is never added using a bare-bones `App`, and so event updates are always
skipped.

This can be worked around by adding your own copy to a
later-in-the-frame schedule, but that's not a very good fix.

Ensure that if we're not using a `FixedUpdate` schedule, events are
always updated every frame.

To do this, I've modified the logic of `event_update_condition` and
`event_update_system` to clearly and correctly differentiate between the
two cases: where we're waiting for a "you should update now" signal and
where we simply don't care.

To encode this, I've added the `ShouldUpdateEvents` enum, replacing a
simple `bool` in `EventRegistry`'s `needs_update` field.

Now, both tests pass as expected, without having to manually add a
system!

I've written two parallel unit tests to cover the intended behavior:

1. Test that `iter_current_update_events` works as expected in
`bevy_ecs`.
2. Test that `iter_current_update_events` works as expected in
`bevy_app`

I've also added a test to verify that event updating works correctly in
the presence of a fixed main schedule, and a second test to verify that
fixed updating works at all to help future authors narrow down failures.

- [x] figure out why the `bevy_app` version of this test fails but the
`bevy_ecs` version does not
- [x] figure out why `EventRegistry::run_updates` isn't working properly
- [x] figure out why `EventRegistry::run_updates` is never getting
called
- [x] figure out why `event_update_condition` is always returning false
- [x] figure out why `EventRegistry::needs_update` is always false
- [x] verify that the problem is a missing `signal_events_update_system`

---------

Co-authored-by: Mike <mike.hsu@gmail.com>
mnmaita pushed a commit to mnmaita/bevy that referenced this pull request Jun 14, 2024
…evyengine#13808)

As discovered in
Leafwing-Studios/leafwing-input-manager#538,
there appears to be some real weirdness going on in how event updates
are processed between Bevy 0.13 and Bevy 0.14.

To identify the cause and prevent regression, I've added tests to
validate the intended behavior.
My initial suspicion was that this would be fixed by
bevyengine#13762, but that doesn't seem to
be the case.

Instead, events appear to never be updated at all when using `bevy_app`
by itself. This is part of the problem resolved by
bevyengine#11528, and introduced by
bevyengine#10077.

After some investigation, it appears that `signal_event_update_system`
is never added using a bare-bones `App`, and so event updates are always
skipped.

This can be worked around by adding your own copy to a
later-in-the-frame schedule, but that's not a very good fix.

Ensure that if we're not using a `FixedUpdate` schedule, events are
always updated every frame.

To do this, I've modified the logic of `event_update_condition` and
`event_update_system` to clearly and correctly differentiate between the
two cases: where we're waiting for a "you should update now" signal and
where we simply don't care.

To encode this, I've added the `ShouldUpdateEvents` enum, replacing a
simple `bool` in `EventRegistry`'s `needs_update` field.

Now, both tests pass as expected, without having to manually add a
system!

I've written two parallel unit tests to cover the intended behavior:

1. Test that `iter_current_update_events` works as expected in
`bevy_ecs`.
2. Test that `iter_current_update_events` works as expected in
`bevy_app`

I've also added a test to verify that event updating works correctly in
the presence of a fixed main schedule, and a second test to verify that
fixed updating works at all to help future authors narrow down failures.

- [x] figure out why the `bevy_app` version of this test fails but the
`bevy_ecs` version does not
- [x] figure out why `EventRegistry::run_updates` isn't working properly
- [x] figure out why `EventRegistry::run_updates` is never getting
called
- [x] figure out why `event_update_condition` is always returning false
- [x] figure out why `EventRegistry::needs_update` is always false
- [x] verify that the problem is a missing `signal_events_update_system`

---------

Co-authored-by: Mike <mike.hsu@gmail.com>
mockersf pushed a commit that referenced this pull request Jun 14, 2024
…13808) (#13842)

# Objective

- Related to #13825

## Solution

- Cherry picked the merged PR and performed the necessary changes to
adapt it to the 0.14 release branch.

---------

As discovered in
Leafwing-Studios/leafwing-input-manager#538,
there appears to be some real weirdness going on in how event updates
are processed between Bevy 0.13 and Bevy 0.14.

To identify the cause and prevent regression, I've added tests to
validate the intended behavior.
My initial suspicion was that this would be fixed by
#13762, but that doesn't seem to
be the case.

Instead, events appear to never be updated at all when using `bevy_app`
by itself. This is part of the problem resolved by
#11528, and introduced by
#10077.

After some investigation, it appears that `signal_event_update_system`
is never added using a bare-bones `App`, and so event updates are always
skipped.

This can be worked around by adding your own copy to a
later-in-the-frame schedule, but that's not a very good fix.

Ensure that if we're not using a `FixedUpdate` schedule, events are
always updated every frame.

To do this, I've modified the logic of `event_update_condition` and
`event_update_system` to clearly and correctly differentiate between the
two cases: where we're waiting for a "you should update now" signal and
where we simply don't care.

To encode this, I've added the `ShouldUpdateEvents` enum, replacing a
simple `bool` in `EventRegistry`'s `needs_update` field.

Now, both tests pass as expected, without having to manually add a
system!

I've written two parallel unit tests to cover the intended behavior:

1. Test that `iter_current_update_events` works as expected in
`bevy_ecs`.
2. Test that `iter_current_update_events` works as expected in
`bevy_app`

I've also added a test to verify that event updating works correctly in
the presence of a fixed main schedule, and a second test to verify that
fixed updating works at all to help future authors narrow down failures.

- [x] figure out why the `bevy_app` version of this test fails but the
`bevy_ecs` version does not
- [x] figure out why `EventRegistry::run_updates` isn't working properly
- [x] figure out why `EventRegistry::run_updates` is never getting
called
- [x] figure out why `event_update_condition` is always returning false
- [x] figure out why `EventRegistry::needs_update` is always false
- [x] verify that the problem is a missing `signal_events_update_system`

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Co-authored-by: Mike <mike.hsu@gmail.com>
@maniwani maniwani deleted the drop-events-later branch June 20, 2024 00:24
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 A-Time Involves time keeping and reporting C-Bug An unexpected or incorrect behavior S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.