Merge pull request #80 from caspark/no-clone-t

feat: Remove GameStateCell's T: Clone requirement

CHANGELOG.md

@@ -4,7 +4,7 @@ In this document, all remarkable changes are listed. Not mentioned are smaller c
 
 ## Unreleased
 
-- Nothing here yet...
+- allow non-`Clone` types to be stored in `GameStateCell`.
 
 ## 0.10.2
 

src/frame_info.rs

@@ -3,7 +3,7 @@ use crate::{Frame, NULL_FRAME};
 /// Represents the game state of your game for a single frame. The `data` holds the game state, `frame` indicates the associated frame number
 /// and `checksum` can additionally be provided for use during a `SyncTestSession`.
 #[derive(Debug, Clone)]
-pub(crate) struct GameState<S: Clone> {
+pub(crate) struct GameState<S> {
     /// The frame to which this info belongs to.
     pub frame: Frame,
     /// The game state
@@ -12,7 +12,7 @@ pub(crate) struct GameState<S: Clone> {
     pub checksum: Option<u128>,
 }
 
-impl<S: Clone> Default for GameState<S> {
+impl<S> Default for GameState<S> {
     fn default() -> Self {
         Self {
             frame: NULL_FRAME,

src/lib.rs

@@ -17,7 +17,7 @@ pub use sessions::builder::SessionBuilder;
 pub use sessions::p2p_session::P2PSession;
 pub use sessions::p2p_spectator_session::SpectatorSession;
 pub use sessions::sync_test_session::SyncTestSession;
-pub use sync_layer::GameStateCell;
+pub use sync_layer::{GameStateAccessor, GameStateCell};
 
 pub(crate) mod error;
 pub(crate) mod frame_info;
@@ -255,7 +255,7 @@ pub trait Config: 'static {
         + bytemuck::Zeroable;
 
     /// The save state type for the session.
-    type State: Clone;
+    type State;
 
     /// The address type which identifies the remote clients
     type Address: Clone + PartialEq + Eq + Hash + Debug;

src/sync_layer.rs

@@ -1,5 +1,6 @@
 use bytemuck::Zeroable;
-use parking_lot::Mutex;
+use parking_lot::{MappedMutexGuard, Mutex};
+use std::ops::Deref;
 use std::sync::Arc;
 
 use crate::frame_info::{GameState, PlayerInput};
@@ -11,9 +12,9 @@ use crate::{Config, Frame, GgrsRequest, InputStatus, PlayerHandle, NULL_FRAME};
 ///
 /// [`save()`]: GameStateCell#method.save
 /// [`load()`]: GameStateCell#method.load
-pub struct GameStateCell<T: Clone>(Arc<Mutex<GameState<T>>>);
+pub struct GameStateCell<T>(Arc<Mutex<GameState<T>>>);
 
-impl<T: Clone> GameStateCell<T> {
+impl<T> GameStateCell<T> {
     /// Saves a `T` the user creates into the cell.
     pub fn save(&self, frame: Frame, data: Option<T>, checksum: Option<u128>) {
         let mut state = self.0.lock();
@@ -23,10 +24,50 @@ impl<T: Clone> GameStateCell<T> {
         state.checksum = checksum;
     }
 
-    /// Loads a `T` that the user previously saved into.
-    pub fn load(&self) -> Option<T> {
-        let state = self.0.lock();
-        state.data.clone()
+    /// Provides direct access to the `T` that the user previously saved into the cell (if there was
+    /// one previously saved), without cloning it.
+    ///
+    /// You probably want to use [load()](Self::load) instead to clone the data; this function is
+    /// useful only in niche use cases.
+    ///
+    /// # Example usage
+    ///
+    /// ```
+    /// # use ggrs::{Frame, GameStateCell};
+    /// // Setup normally performed by GGRS behind the scenes
+    /// let mut cell = GameStateCell::<MyGameState>::default();
+    /// let frame_num: Frame = 0;
+    ///
+    /// // The state of our example game will be just a String, and our game state isn't Clone
+    /// struct MyGameState { player_name: String };
+    ///
+    /// // Setup you do when GGRS requests you to save game state
+    /// {
+    ///     let game_state = MyGameState { player_name: "alex".to_owned() };
+    ///     let checksum = None;
+    ///     // (in real usage, save a checksum! We omit it here because it's not
+    ///     // relevant to this example)
+    ///     cell.save(frame_num, Some(game_state), checksum);
+    /// }
+    ///
+    /// // We can't use load() to access the game state, because it's not Clone
+    /// // println!("{}", cell.load().player_name); // compile error: Clone bound not satisfied
+    ///
+    /// // But we can still read the game state without cloning:
+    /// let game_state_accessor = cell.data().expect("should have a gamestate stored");
+    /// assert_eq!(game_state_accessor.player_name, "alex");
+    /// ```
+    ///
+    /// If you really, really need mutable access to the `T`, then consider using the aptly named
+    /// [GameStateAccessor::as_mut_dangerous()].
+    pub fn data(&self) -> Option<GameStateAccessor<'_, T>> {
+        if let Ok(mapped_data) =
+            parking_lot::MutexGuard::try_map(self.0.lock(), |state| state.data.as_mut())
+        {
+            return Some(GameStateAccessor(mapped_data));
+        } else {
+            None
+        }
     }
 
     pub(crate) fn frame(&self) -> Frame {
@@ -38,19 +79,29 @@ impl<T: Clone> GameStateCell<T> {
     }
 }
 
-impl<T: Clone> Default for GameStateCell<T> {
+impl<T: Clone> GameStateCell<T> {
+    /// Loads a `T` that the user previously saved into this cell, by cloning the `T`.
+    ///
+    /// See also [data()](Self::data) if you want a reference to the `T` without cloning it.
+    pub fn load(&self) -> Option<T> {
+        let data = self.data()?;
+        Some(data.clone())
+    }
+}
+
+impl<T> Default for GameStateCell<T> {
     fn default() -> Self {
         Self(Arc::new(Mutex::new(GameState::default())))
     }
 }
 
-impl<T: Clone> Clone for GameStateCell<T> {
+impl<T> Clone for GameStateCell<T> {
     fn clone(&self) -> Self {
         Self(self.0.clone())
     }
 }
 
-impl<T: Clone> std::fmt::Debug for GameStateCell<T> {
+impl<T> std::fmt::Debug for GameStateCell<T> {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         let inner = self.0.lock();
         f.debug_struct("GameStateCell")
@@ -60,13 +111,43 @@ impl<T: Clone> std::fmt::Debug for GameStateCell<T> {
     }
 }
 
-#[derive(Clone)]
-pub(crate) struct SavedStates<T: Clone> {
+/// A read-only accessor for the `T` that the user previously saved into a [GameStateCell].
+///
+/// You can use [deref()](Deref::deref) to access the `T` without cloning it; see
+/// [GameStateCell::data()](GameStateCell::data) for a usage example.
+///
+/// This type exists to A) hide the type of the lock guard that allows thread-safe access to the
+///  saved `T` so that it does not form part of GGRS API and B) make dangerous mutable access to the
+///  `T` very explicit (see [as_mut_dangerous()](Self::as_mut_dangerous)).
+pub struct GameStateAccessor<'c, T>(MappedMutexGuard<'c, T>);
+
+impl<'c, T> Deref for GameStateAccessor<'c, T> {
+    type Target = T;
+    fn deref(&self) -> &Self::Target {
+        &self.0
+    }
+}
+
+impl<'c, T> GameStateAccessor<'c, T> {
+    /// Get mutable access to the `T` that the user previously saved into a [GameStateCell].
+    ///
+    /// You probably do not need this! It's safer to use [Self::deref()](Deref::deref) instead;
+    /// see [GameStateCell::data()](GameStateCell::data) for a usage example.
+    ///
+    /// **Danger**: the underlying `T` must _not_ be modified in any way that affects (or may ever
+    /// in future affect) game logic. If this invariant is violated, you will almost certainly get
+    /// desyncs.
+    pub fn as_mut_dangerous(&mut self) -> &mut T {
+        &mut self.0
+    }
+}
+
+pub(crate) struct SavedStates<T> {
     pub states: Vec<GameStateCell<T>>,
     max_pred: usize,
 }
 
-impl<T: Clone> SavedStates<T> {
+impl<T> SavedStates<T> {
     fn new(max_pred: usize) -> Self {
         let mut states = Vec::with_capacity(max_pred);
         for _ in 0..max_pred {