Closed cBournhonesque closed 3 months ago
This is quite straightforward and the test cases are confidence-inspiring. Definitely reduces footgunnyness, but I do also think that doing input in fixed update is inadvisable in the first place. I dont think that means it should explode on people who do that, but I do wonder if the copying around of structs has a performance impact that everyone will have to pay even if they only do input in update.
I agree that there is a additional cost, so maybe we could gate this change behind a feature flag. But in general there are a lot of cases where handling inputs in fixed-update is necessary; anything related to the simulation or to networking generally has to run in FixedUpdate (physics, simulation, etc.).
In my usecase, I maintain a networking library that has rollback networking; for rollback to work properly most of the simulation must happen in FixedUpdate, so users will have a lot of input-handling systems inside FixedUpdate.
Are there any cases where you'd want input both in fixed update and normal update? If you're doing rollback netcode sim stuff you wouldnt want any input in normal update, right? Maybe the right approach is to just switch between fixed/notfixed entirely? In any case I think this can be merged as is, without feature gates. Gating would have to be motivated by benchmarks that show the additional complexity is warranted.
Are there any cases where you'd want input both in fixed update and normal update? If you're doing rollback netcode sim stuff you wouldnt want any input in normal update, right? Maybe the right approach is to just switch between fixed/notfixed entirely? In any case I think this can be merged as is, without feature gates. Gating would have to be motivated by benchmarks that show the additional complexity is warranted.
In most cases it would probably be: player inputs (move, fire weapons, etc.) in FixedUpdate, and admin inputs (UI, settings, chat) in Update.
So yeah maybe it's worth having a Mode
enum where an input would be either in FixedUpdate or Update.
But it's probable that in some cases it would be useful to have inputs work in both FixedUpdate / Update, maybe we could provide 3 modes: Update
, FixedUpdate
and Both
. Then it's a matter of ergonomics or UX whether we want to expose this kind of complexity to the user or hide it by always using Both
. Maybe @alice-i-cecile has an opinion here
Needs formatting now before I can merge 😄
Context
Fixes https://github.com/Leafwing-Studios/leafwing-input-manager/issues/252 (see description of the issue in https://github.com/bevyengine/bevy/issues/6183)
It would also fix https://github.com/cBournhonesque/lightyear/issues/349
(I did a write-up here: https://hackmd.io/_TGuaUTnRBeuisvUMr0QoQ?both)
i.e. that we cannot reliably use
action.just_pressed()
inFixedUpdate
systems because:FixedUpdate
runs 2 times in the same frame, which means that they would both hadjust_pressed() = True
which is misleadingsituation 1 (S1):
F - FU - FU - F
FixedUpdate
runs 0 times in one frame, which means thatjust_pressed
becomespressed
and the input is never seen by the FixedUpdate system.situation 2 (S2):
F - F - FU - F
Solution
Outline
This is a an initial version that can probably be improved, but it solves the issues with fixed timesteps.
We will have the same approach as the
Time
API: this is how it works: https://github.com/bevyengine/bevy/blob/main/crates/bevy_time/src/fixed.rs#L237 There are 3 resourcesTime<Fixed>
,Time<Virtual>
,Time<()>
. The resourceTime<()>
is set to eitherTime<Fixed>
andTime<Virtual>
depending on the schedule we are running in.Here we do the same thing; the
ActionData
has 3 fields to represent state:We will update
update_state
inUpdate
andfixed_update_state
inFixedUpdate
. The user-facing interface will bestate
which switches betweenupdate_state
andfixed_update_state
depending on the schedule; but maybe we could also exposeupdate_state
andfixed_update_state
(similarly to howTime<Virtual>
andTime<Res>
are exposed)We still keep
state
so that:state
with eitherupdate_state
orfixed_update_state
depending on which schedule we are runningstate
and they will access the correct data depending on whether they are running in FixedUpdate or UpdateSystem order
So the order is
Schedule =
PreUpdate
ActionState
state
. (new button presses, etc.)before Schedule =
RunFixedMainLoop
(we use this because we don't want to run the systems once per FixedUpdate; there could be multiple FixedUpdate runs in a frame)update_state = state
to apply the changes; then dostate = fixed_update_state
to load the new statestate
(the input events need to be applied event because we essentially maintain 2 independentActionState
, one in Update and one in FixedUpdate)Schedule =
FixedPreUpdate
ui
feature does)Schedule =
FixedPostUpdate
ActionState
(we want to run this every FixedUpdate to go fromjust_pressed
topressed
and have better timing information)after Schedule =
RunFixedMainLoop
fixed_update_state = state
to save the changes, and dostate = update_state
to load the new stateI think having the fields inside
ActionData
instead of having 3 separateActionData
is a feature, not a bug: some fields need to be updated only once (value, axis_pair), and should even be shared between the two schedules (consumed).The
timing
information is also shared, but only updated inPreUpdate
becauseTime<Real>
is only updated in pre-updated. I don't think it's worth worrying about handling timing information separately inTime<Fixed>
because:Test
Added 2 unit tests to show that the problems 1 and 2 described above are solved.