cBournhonesque / lightyear

A networking library to make multiplayer games for the Bevy game engine
https://cbournhonesque.github.io/lightyear/book
Apache License 2.0
456 stars 47 forks source link

crashing with 'attempt to subtract with overflow' in input_buffer #560

Open RJ opened 2 months ago

RJ commented 2 months ago

had a couple of crashes like this:

thread 'Compute Task Pool (0)' panicked at lightyear-2cfb5e6660946fe3/356f61d/lightyear/src/inputs/leafwing/input_buffer.rs:188:53:
attempt to subtract with overflow

https://github.com/cBournhonesque/lightyear/blob/main/lightyear/src/inputs/leafwing/input_buffer.rs#L188

something like this maybe?

if tick < start_tick || tick > Tick(start_tick.wrapping_add_signed(self.buffer.len() as i16 - 1)) {
    return None;
}
cBournhonesque commented 2 months ago

thanks, yes i'll fix that

RJ commented 2 months ago

same crash here in get_last https://github.com/cBournhonesque/lightyear/blob/main/lightyear/src/inputs/leafwing/input_buffer.rs#L210

and probably a similar one at least in get_last_with_tick

cBournhonesque commented 2 months ago

I don't think the tick addition is the problem, it's already handled with wrapping_add: https://github.com/cBournhonesque/lightyear/blob/87806d0c9cb6fef22978cf7b170089e37711d329/lightyear/src/utils/wrapping_id.rs#L135-L135 https://github.com/cBournhonesque/lightyear/blob/87806d0c9cb6fef22978cf7b170089e37711d329/lightyear/src/utils/wrapping_id.rs#L157

For some reason it seems to be the subtract, but I don't understand how it could be happening

RJ commented 2 months ago

hmm. this is in the 'sandbox' version of my client/server game, which although it adds the lightyear plugins and components, just like the client and server code, it doesnt ever start or connect. it's a local only testbed.

perhaps something that would normally be pruned or some maintenance tasks aren't running as a result

RJ commented 2 months ago

https://github.com/cBournhonesque/lightyear/blob/main/lightyear/src/inputs/leafwing/input_buffer.rs#L102 too, in set()

cBournhonesque commented 2 months ago

You still get this? Could you try adding logs so we see the values that trigger the crash?

RJ commented 2 months ago

i believe it's happening when buffer_len (u16) reaches i16::MAX + 1, then the cast as i16 converts it to i16::MIN, which you can't subtract 1 from. (after approx. 10mins at 64 ticks/sec)

buffer normally would be pruned i'm guessing, and isn't because i've not started the client or server.

cBournhonesque commented 2 months ago

Ah I see, that's helpful thanks

RJ commented 2 months ago

I'm avoiding the crash by adding the following system when running under my local sandbox environment, which doesn't start the lightyear server or client, it just adds the plugins.

Not much of a bug in the end, since i'm doing something weird.

However, pop was pub(crate), which i had to change to pub to make this possible. (want a small pr for that?)

app.add_systems(FixedPostUpdate, prune_input_buffer);

/// when running in sandbox mode, normal lightyear systems aren't running, so we have to
/// prune the input buffer manually.
fn prune_input_buffer(
    mut q: Query<
        &mut InputBuffer<PlayerActions>,
        With<Player>,
    >,
    tick_manager: Res<TickManager>,
) {
    for mut buffer in q.iter_mut() {
        buffer.pop(tick_manager.tick());
    }
}
cBournhonesque commented 2 months ago

(want a small pr for that?)

Sure! I think it's ok to make things like that pub; I guess most users wouldn't use it but advances users might

mvlabat commented 1 week ago

I've just triggered the same panic on 0.17.

Could we consider adding the logic of the system that @RJ posted to the crate, which would run whenever neither client or server isn't started? In my case, I also add the plugins but I call ClientCommands::connect_client only when a user decides to join a multiplayer game.

I don't think we should treat this as an advanced or a niche case, I'd guess that any game that wants to support both singleplayer and multiplayer modes or has a server borwser would be doing the same.

cBournhonesque commented 1 week ago

Sure, feel free to open a PR! (I'm busy with job search currently so I don't have too much time)

I guess it would just be a system that runs when the client is not connected or the inputs are disabled, and it would disable clear the InputBuffer? In general, I would like to not include anything in the buffer if the ActionState or Inputs are disabled. I remember tackling this but I was blocked by something, can't remember why ... But yea a good first step is not buffer inputs in the InputBuffer if the client is not connected