From a06b63bc185e87e984698b0b803e68f87f027a74 Mon Sep 17 00:00:00 2001 From: Caspar Krieger Date: Wed, 13 Nov 2024 09:45:15 +0800 Subject: [PATCH] feat: Input type must be Default+Serde but not POD Remove the bytemuck bounds on Config::Input, and replace them with Default and Serialize + DeserializeOwned. 1. Default is a less strict bound than Zeroable, and it's all we need - so it's more ergonomic. 2. NoUninit + CheckedBitPattern are also fairly stringent requirements, but it turns out that the only place that we actually rely on them is when we serialize or deserialize the input type. So we can just rely on the relevant Serde trait bounds instead, which again is more ergonomic. Both changes are backwards incompatible, though they should be quite easy to resolve for existing users of GGRS (unless they have a philosophical objection to using Serde directly, I suppose). However, number 2 does have some significant tradeoffs associated with it; in order of least to most bad: * Serde becomes part of our public API. * Bincode's chosen serialization scheme may interfere with the "XOR with previous input then run length encode the result" trick we use to shrink the size of inputs when we send them to connected endpoints. If this is a problem for a user, they can always manually serialize to a byte array in such a way that the run length encoding works better for their data. * Users may bloat their input struct to the point where it (or N instances of it, where N == number of local players) is larger than the maximum size we limit ourselves to for a single UDP packet. This is particularly problematic since right now we just panic (due to a failed assertion) when this happens. --- Cargo.toml | 1 - examples/ex_game/ex_game.rs | 3 +-- src/frame_info.rs | 22 ++++------------------ src/input_queue.rs | 6 +++--- src/lib.rs | 24 +++++++----------------- src/network/protocol.rs | 10 ++++++---- src/sync_layer.rs | 7 +++---- tests/stubs.rs | 4 ++-- tests/stubs_enum.rs | 11 +++-------- 9 files changed, 29 insertions(+), 59 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 46418ef..3de288d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -21,7 +21,6 @@ rand = "0.8" bitfield-rle = "0.2.1" parking_lot = "0.12" instant = "0.1" -bytemuck = {version = "1.9", features = ["derive"]} getrandom = {version = "0.2", optional = true} [target.'cfg(target_arch = "wasm32")'.dependencies] diff --git a/examples/ex_game/ex_game.rs b/examples/ex_game/ex_game.rs index fb2c733..f0ee57d 100644 --- a/examples/ex_game/ex_game.rs +++ b/examples/ex_game/ex_game.rs @@ -1,6 +1,5 @@ use std::net::SocketAddr; -use bytemuck::{Pod, Zeroable}; use ggrs::{Config, Frame, GameStateCell, GgrsRequest, InputStatus, PlayerHandle, NULL_FRAME}; use macroquad::prelude::*; use serde::{Deserialize, Serialize}; @@ -24,7 +23,7 @@ const MAX_SPEED: f32 = 7.0; const FRICTION: f32 = 0.98; #[repr(C)] -#[derive(Copy, Clone, PartialEq, Pod, Zeroable)] +#[derive(Copy, Clone, PartialEq, Default, Serialize, Deserialize)] pub struct Input { pub inp: u8, } diff --git a/src/frame_info.rs b/src/frame_info.rs index c9eec0b..41c8b0a 100644 --- a/src/frame_info.rs +++ b/src/frame_info.rs @@ -27,12 +27,7 @@ impl Default for GameState { #[derive(Debug, Copy, Clone, PartialEq)] pub(crate) struct PlayerInput where - I: Copy - + Clone - + PartialEq - + bytemuck::NoUninit - + bytemuck::CheckedBitPattern - + bytemuck::Zeroable, + I: Copy + Clone + PartialEq, { /// The frame to which this info belongs to. -1/[`NULL_FRAME`] represents an invalid frame pub frame: Frame, @@ -40,15 +35,7 @@ where pub input: I, } -impl< - I: Copy - + Clone - + PartialEq - + bytemuck::NoUninit - + bytemuck::Zeroable - + bytemuck::CheckedBitPattern, - > PlayerInput -{ +impl PlayerInput { pub(crate) fn new(frame: Frame, input: I) -> Self { Self { frame, input } } @@ -56,7 +43,7 @@ impl< pub(crate) fn blank_input(frame: Frame) -> Self { Self { frame, - input: I::zeroed(), + input: I::default(), } } @@ -72,10 +59,9 @@ impl< #[cfg(test)] mod game_input_tests { use super::*; - use bytemuck::{Pod, Zeroable}; #[repr(C)] - #[derive(Copy, Clone, PartialEq, Pod, Zeroable)] + #[derive(Copy, Clone, PartialEq, Default)] struct TestInput { inp: u8, } diff --git a/src/input_queue.rs b/src/input_queue.rs index ef9ac79..f06c72f 100644 --- a/src/input_queue.rs +++ b/src/input_queue.rs @@ -11,7 +11,7 @@ pub(crate) struct InputQueue where T: Config, { - /// The head of the queue. The newest `PlayerInput` is saved here + /// The head of the queue. The newest `PlayerInput` is saved here head: usize, /// The tail of the queue. The oldest `PlayerInput` still valid is saved here. tail: usize, @@ -250,12 +250,12 @@ mod input_queue_tests { use std::net::SocketAddr; - use bytemuck::{Pod, Zeroable}; + use serde::{Deserialize, Serialize}; use super::*; #[repr(C)] - #[derive(Copy, Clone, PartialEq, Pod, Zeroable)] + #[derive(Copy, Clone, PartialEq, Default, Serialize, Deserialize)] struct TestInput { inp: u8, } diff --git a/src/lib.rs b/src/lib.rs index dc2ebf0..427d935 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -13,6 +13,7 @@ pub use error::GgrsError; pub use network::messages::Message; pub use network::network_stats::NetworkStats; pub use network::udp_socket::UdpNonBlockingSocket; +use serde::{de::DeserializeOwned, Serialize}; pub use sessions::builder::SessionBuilder; pub use sessions::p2p_session::P2PSession; pub use sessions::p2p_spectator_session::SpectatorSession; @@ -205,12 +206,9 @@ pub trait Config: 'static + Send + Sync { /// The input type for a session. This is the only game-related data /// transmitted over the network. /// - /// Reminder: Types implementing [Pod] may not have the same byte representation - /// on platforms with different endianness. GGRS assumes that all players are - /// running with the same endianness when encoding and decoding inputs. - /// - /// [Pod]: bytemuck::Pod - type Input: Copy + Clone + PartialEq + bytemuck::Pod + bytemuck::Zeroable + Send + Sync; + /// The implementation of [Default] is used for representing "no input" for + /// a player, including when a player is disconnected. + type Input: Copy + Clone + PartialEq + Default + Serialize + DeserializeOwned + Send + Sync; /// The save state type for the session. type State: Clone + Send + Sync; @@ -242,17 +240,9 @@ pub trait Config: 'static { /// The input type for a session. This is the only game-related data /// transmitted over the network. /// - /// Reminder: Types implementing [Pod] may not have the same byte representation - /// on platforms with different endianness. GGRS assumes that all players are - /// running with the same endianness when encoding and decoding inputs. - /// - /// [Pod]: bytemuck::Pod - type Input: Copy - + Clone - + PartialEq - + bytemuck::NoUninit - + bytemuck::CheckedBitPattern - + bytemuck::Zeroable; + /// The implementation of [Default] is used for representing "no input" for + /// a player, including when a player is disconnected. + type Input: Copy + Clone + PartialEq + Default + Serialize + DeserializeOwned; /// The save state type for the session. type State; diff --git a/src/network/protocol.rs b/src/network/protocol.rs index 04bebce..4f681af 100644 --- a/src/network/protocol.rs +++ b/src/network/protocol.rs @@ -73,8 +73,9 @@ impl InputBytes { if input.frame != NULL_FRAME { frame = input.frame; } - let byte_vec = bytemuck::bytes_of(&input.input); - bytes.extend_from_slice(byte_vec); + + bincode::serialize_into(&mut bytes, &input.input) + .expect("input serialization failed"); } } Self { frame, bytes } @@ -87,8 +88,9 @@ impl InputBytes { for p in 0..num_players { let start = p * size; let end = start + size; - let input = *bytemuck::checked::try_from_bytes::(&self.bytes[start..end]) - .expect("Expected received data to be valid."); + let player_byte_slice = &self.bytes[start..end]; + let input: T::Input = + bincode::deserialize(player_byte_slice).expect("input deserialization failed"); player_inputs.push(PlayerInput::new(self.frame, input)); } player_inputs diff --git a/src/sync_layer.rs b/src/sync_layer.rs index 6fe5874..50de1ca 100644 --- a/src/sync_layer.rs +++ b/src/sync_layer.rs @@ -1,4 +1,3 @@ -use bytemuck::Zeroable; use parking_lot::{MappedMutexGuard, Mutex}; use std::ops::Deref; use std::sync::Arc; @@ -282,7 +281,7 @@ impl SyncLayer { let mut inputs = Vec::new(); for (i, con_stat) in connect_status.iter().enumerate() { if con_stat.disconnected && con_stat.last_frame < self.current_frame { - inputs.push((T::Input::zeroed(), InputStatus::Disconnected)); + inputs.push((T::Input::default(), InputStatus::Disconnected)); } else { inputs.push(self.input_queues[i].input(self.current_frame)); } @@ -380,11 +379,11 @@ impl SyncLayer { mod sync_layer_tests { use super::*; - use bytemuck::{Pod, Zeroable}; + use serde::{Deserialize, Serialize}; use std::net::SocketAddr; #[repr(C)] - #[derive(Copy, Clone, PartialEq, Pod, Zeroable)] + #[derive(Copy, Clone, PartialEq, Default, Serialize, Deserialize)] struct TestInput { inp: u8, } diff --git a/tests/stubs.rs b/tests/stubs.rs index e184309..d254da8 100644 --- a/tests/stubs.rs +++ b/tests/stubs.rs @@ -1,4 +1,5 @@ use rand::{prelude::ThreadRng, thread_rng, Rng}; +use serde::{Deserialize, Serialize}; use std::collections::hash_map::DefaultHasher; use std::hash::{Hash, Hasher}; use std::net::SocketAddr; @@ -14,10 +15,9 @@ fn calculate_hash(t: &T) -> u64 { pub struct GameStub { pub gs: StateStub, } -use bytemuck::{Pod, Zeroable}; #[repr(C)] -#[derive(Copy, Clone, PartialEq, Pod, Zeroable)] +#[derive(Copy, Clone, PartialEq, Default, Serialize, Deserialize)] pub struct StubInput { pub inp: u32, } diff --git a/tests/stubs_enum.rs b/tests/stubs_enum.rs index ed75b4c..723fa7f 100644 --- a/tests/stubs_enum.rs +++ b/tests/stubs_enum.rs @@ -3,6 +3,7 @@ use std::hash::{Hash, Hasher}; use std::net::SocketAddr; use ggrs::{Config, Frame, GameStateCell, GgrsRequest, InputStatus}; +use serde::{Deserialize, Serialize}; fn calculate_hash(t: &T) -> u64 { let mut s = DefaultHasher::new(); @@ -13,22 +14,16 @@ fn calculate_hash(t: &T) -> u64 { pub struct GameStubEnum { pub gs: StateStubEnum, } -use bytemuck::{CheckedBitPattern, NoUninit, Zeroable}; #[allow(dead_code)] #[repr(u8)] -#[derive(Copy, Clone, PartialEq, CheckedBitPattern, NoUninit)] +#[derive(Copy, Clone, PartialEq, Default, Serialize, Deserialize)] pub enum EnumInput { + #[default] Val1, Val2, } -unsafe impl Zeroable for EnumInput { - fn zeroed() -> Self { - unsafe { core::mem::zeroed() } - } -} - pub struct StubEnumConfig; impl Config for StubEnumConfig {