fix: avoid panicking when local frame adv exceeds i8 range

CHANGELOG.md

@@ -7,8 +7,9 @@ In this document, all remarkable changes are listed. Not mentioned are smaller c
 - allow non-`Clone` types to be stored in `GameStateCell`.
 - added `SyncTestSession::current_frame()` and `SpectatorSession::current_frame()` to match the existing `P2PSession::current_frame()`.
 - added `P2PSession::desync_detection()` to read the session's desync detection mode.
-- ggrs no longer panics when trying to send an overly large UDP packet, unless debug assertions are on.
-- fixed: ggrs would panic when trying to send a message over a custom socket implementation if that message exceeded the maximum safe UDP packet size, even though the underlying socket might have totally different applicable thresholds for what messages can be safely delivered.
+- fix: ggrs no longer panics when a client's local frame advantage exceeds the range of an i8 ([#35](https://github.com/gschup/ggrs/issues/35))
+- fix: ggrs no longer panics when trying to send an overly large UDP packet, unless debug assertions are on.
+- fix: ggrs no longer panics when trying to send a message over a custom socket implementation if that message exceeded the maximum safe UDP packet size, even though the underlying socket might have totally different applicable thresholds for what messages can be safely delivered.
 - fix a false positive in `P2PSession`'s desync detection; it was possible for a desync to incorrectly be detected when `P2PSession::advance_frame()` would 1. enqueue a checksum-changing rollback, 2. mark a to-be-rolled-back frame as confirmed, and 3. send that newly-confirmed frame's still-incorrect checksum to peers.
 
 ## 0.10.2

src/network/messages.rs

@@ -63,7 +63,19 @@ impl Default for InputAck {
 
 #[derive(Copy, Clone, Debug, PartialEq, Eq, Serialize, Deserialize, Default)]
 pub(crate) struct QualityReport {
-    pub frame_advantage: i8, // frame advantage of other player
+    /// Frame advantage of other player.
+    ///
+    /// While on the one hand 2 bytes is overkill for a value that is typically in the range of say
+    /// -8 to 8 (for the default prediction window size of 8), on the other hand if we don't get a
+    /// chance to read quality reports for a time (due to being paused in a background tab, or
+    /// someone stepping through code in a debugger) then it is easy to exceed the range of a signed
+    /// 1 byte integer at common FPS values.
+    ///
+    /// So by using an i16 instead of an i8, we can avoid clamping the value for +/- ~32k frames, or
+    /// about +/- 524 seconds of frame advantage - and after 500+ seconds it's a pretty reasonable
+    /// assumption that the other player will have been disconnected, or at least that they're so
+    /// far ahead/behind that clamping the value to an i16 won't matter for any practical purpose.
+    pub frame_advantage: i16,
     pub ping: u128,
 }
 

src/network/protocol.rs

@@ -516,8 +516,11 @@ impl<T: Config> UdpProtocol<T> {
     fn send_quality_report(&mut self) {
         self.running_last_quality_report = Instant::now();
         let body = QualityReport {
-            frame_advantage: i8::try_from(self.local_frame_advantage)
-                .expect("local_frame_advantage bigger than i8::MAX"),
+            frame_advantage: i16::try_from(
+                self.local_frame_advantage
+                    .clamp(i16::MIN as i32, i16::MAX as i32),
+            )
+            .expect("local_frame_advantage should have been clamped into the range of an i16"),
             ping: millis_since_epoch(),
         };