NetworkWeapon._can_fire() will desync if dependent on NetworkTime.tick

[Lexari0]

The example weapon script's implementation of _can_fire relies on NetworkTime.tick which will tend to be smaller when the client calls the function in their _tick compared to when the server calls it in their _request_projectile rpc.

Adding the line print(multiplayer.get_unique_id(), "] last_fire: ", last_fire, " -> ", NetworkTime.tick) before assigning last_fire = NetworkTime.tick confirms this:

626816780] last_fire_tick: 4050 -> 4052 ; 2 tick delay on the client
1] last_fire_tick: 4053 -> 4055         ; 2 tick delay on the server, but 1 tick behind the client
626816780] last_fire_tick: 4052 -> 4054 ; 2 tick delay on the client, but 1 tick ahead of server
626816780] last_fire_tick: 4054 -> 4055 ; Now a 1 tick delay on the client since _accept_projectile calls _after_fire
1] last_fire_tick: 4055 -> 4057         ; 2 tick delay on the server

; Server now gets the client's _request_projectile(id, 4054, {...}) rpc 4 ticks late
[ERR][netfox.extras::NetworkWeapon] Projectile ghisrnm0t4gq rejected! Peer 626816780 can't use this weapon now (tick=4054; NetworkTime.tick=4058)

This shows that the server is desynced from the client (inherently) and that delay isn't properly accounted for. I think the simplest solution would be to modify _can_fire(), fire(), and _after_fire() to take the tick: int from _request_projectile (or an arbitrary if called locally from fire(tick: int)), though that would break compatibility and require users to fix their _can_fire(), fire(), and _after_fire() implementations. Another option would be to temporarily store the tick: int parameter in an externally accessible variable - potentially even in NetworkTime.tick for maximum compatibility, which would need to be restored after the calls to _can_fire() and _spawn(). The downside here is that a player could potentially cheat by manipulating the tick variable in their RPC request, but I think that should be handled by _is_reconcilable. EDIT: This doesn't work as it leads to projectiles being requested in the past. It seems the real solution involves removing the need to call both fire() and _request_projectile on the server (or at least confirm both of those calls are for the same projectile and skip the second one).

[elementbound]

Really good catch, thanks for bringing this up! Your solution is really compelling, though indeed, it's a bit uncomfortable that it's breaking. I'll see what I can do!