From cbe537f8d0640b03bf10d9217591c06d0a47af0c Mon Sep 17 00:00:00 2001
From: Max Whitehead <35712032+MaxCWhitehead@users.noreply.github.com>
Date: Mon, 8 Apr 2024 16:50:43 -0700
Subject: [PATCH] fix: Fix Zero'd DenseMoveDir not representing no input.
 (#380)

When GGRS sends predicted input on first frame (no prev input to use),
it [zeroes
PlayerInput](https://github.com/gschup/ggrs/blob/0af1a044b96465bd10398947b7fb5e0a34a75f70/src/frame_info.rs#L56)
and uses that.

Due to how we quantize move direction, zeroed bits did not represent no
movement input, it instead was `(-1.0, -1.0)`. This sometimes caused
desync on beginning of online match.

Previously used 6 bit representation over range, was changed to quantize
[0, 1.0] with 5 bits, and include a sign bit to map from [0, 1.0] ->
[-1.0, 1.0]. Because range is symmetric and we quantize only half of it
with one less bit, should still be same amount of precision. Now all
zero bits represents 0 which is compatible with ggrs.

Additionally added debug log (not enabled by default) in networking to
dump net inputs.
---
 .../bones_framework/src/networking.rs         |  7 ++-
 .../bones_framework/src/networking/proto.rs   | 52 ++++++++++++++++---
 2 files changed, 50 insertions(+), 9 deletions(-)

diff --git a/framework_crates/bones_framework/src/networking.rs b/framework_crates/bones_framework/src/networking.rs
index b9648aa09c..bb4792a17d 100644
--- a/framework_crates/bones_framework/src/networking.rs
+++ b/framework_crates/bones_framework/src/networking.rs
@@ -5,7 +5,7 @@ use std::{fmt::Debug, marker::PhantomData, sync::Arc};
 use ggrs::{NetworkStats, P2PSession, PlayerHandle};
 use instant::Duration;
 use once_cell::sync::Lazy;
-use tracing::{debug, error, info, warn};
+use tracing::{debug, error, info, trace, warn};
 
 use crate::prelude::*;
 
@@ -469,6 +469,11 @@ where
                                         for (player_idx, (input, status)) in
                                             network_inputs.into_iter().enumerate()
                                         {
+                                            trace!(
+                                                "Net player({player_idx}) local: {}, status: {status:?}, input: {:?}",
+                                                self.local_player_idx == player_idx,
+                                                input
+                                            );
                                             player_inputs.network_update(
                                                 player_idx,
                                                 &input,
diff --git a/framework_crates/bones_framework/src/networking/proto.rs b/framework_crates/bones_framework/src/networking/proto.rs
index 695bd41d8f..126b052c6a 100644
--- a/framework_crates/bones_framework/src/networking/proto.rs
+++ b/framework_crates/bones_framework/src/networking/proto.rs
@@ -11,24 +11,35 @@ use crate::prelude::*;
 pub struct DenseMoveDirection(pub Vec2);
 
 /// This is the specific [`Quantized`] type that we use to represent movement directions in
-/// [`DenseMoveDirection`].
-type MoveDirQuant = Quantized<IntRange<u16, 0b111111, -1, 1>>;
+/// [`DenseMoveDirection`]. This encodes magnitude of direction, but sign is encoded separately.
+type MoveDirQuant = Quantized<IntRange<u16, 0b11111, 0, 1>>;
 
 impl From<u16> for DenseMoveDirection {
     fn from(bits: u16) -> Self {
         // maximum movement value representable, we use 6 bits to represent each movement direction.
-        let max = 0b111111;
+        // Most significant is sign, and other 5 encode float value between 0 and
+        let bit_length = 6;
+        let quantized = 0b011111;
+        let sign = 0b100000;
         // The first six bits represent the x movement
-        let x_move_bits = bits & max;
+        let x_move_bits = bits & quantized;
+        let x_move_sign = if bits & sign == 0 { 1.0 } else { -1.0 };
         // The second six bits represents the y movement
-        let y_move_bits = (bits >> 6) & max;
+        let y_move_bits = (bits >> bit_length) & quantized;
+        let y_move_sign = if (bits >> bit_length) & sign == 0 {
+            1.0
+        } else {
+            -1.0
+        };
 
         // Round near-zero values to zero
         let mut x = MoveDirQuant::from_raw(x_move_bits).to_f32();
+        x *= x_move_sign;
         if x.abs() < 0.02 {
             x = 0.0;
         }
         let mut y = MoveDirQuant::from_raw(y_move_bits).to_f32();
+        y *= y_move_sign;
         if y.abs() < 0.02 {
             y = 0.0;
         }
@@ -39,10 +50,20 @@ impl From<u16> for DenseMoveDirection {
 
 impl From<DenseMoveDirection> for u16 {
     fn from(dir: DenseMoveDirection) -> Self {
-        let x_bits = MoveDirQuant::from_f32(dir.x).raw();
-        let y_bits = MoveDirQuant::from_f32(dir.y).raw();
+        let x_bits = MoveDirQuant::from_f32(dir.x.abs()).raw();
+        let y_bits = MoveDirQuant::from_f32(dir.y.abs()).raw();
+        let x_sign_bit = if dir.x.is_sign_positive() {
+            0
+        } else {
+            0b100000
+        };
+        let y_sign_bit = if dir.y.is_sign_positive() {
+            0
+        } else {
+            0b100000
+        };
 
-        x_bits | (y_bits << 6)
+        (x_bits | x_sign_bit) | ((y_bits | y_sign_bit) << 6)
     }
 }
 
@@ -59,3 +80,18 @@ impl From<DenseMoveDirection> for u32 {
         bits_16 as u32
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use crate::prelude::proto::DenseMoveDirection;
+
+    #[test]
+    /// GGRS currently uses zero'd player input as prediction on first frame,
+    /// so ensure our move direction representation is no input when built from
+    /// 0 bits.
+    pub fn zeroed_dense_move_dir() {
+        let bits: u16 = 0;
+        let dense_move_dir = DenseMoveDirection::from(bits);
+        assert_eq!(dense_move_dir.0, glam::Vec2::ZERO);
+    }
+}