From: Ian Jackson <ijackson@chiark.greenend.org.uk>
Date: Sun, 27 Sep 2020 00:19:55 +0000 (+0100)
Subject: Fix game saving
X-Git-Tag: otter-0.2.0~874
X-Git-Url: https://www.chiark.greenend.org.uk/ucgi/~ianmdlvl/git?a=commitdiff_plain;h=edcd7ad936d578b33ff838bfc260f4467f79c657;p=otter.git

Fix game saving

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
---

diff --git a/src/cmdlistener.rs b/src/cmdlistener.rs
index 5ec81846..6267bcf9 100644
--- a/src/cmdlistener.rs
+++ b/src/cmdlistener.rs
@@ -68,7 +68,7 @@ fn execute(cs: &mut CommandStream, cmd: MgmtCommand) -> MgmtResponse {
         scoped_name : name,
       };
 
-      let gref = Instance::new(name, gs, Default::default())?;
+      let gref = Instance::new(name, gs)?;
       let mut ig = gref.lock()?;
 
       execute_for_game(cs, &mut ig, insns, MgmtGameUpdateMode::Bulk)
@@ -223,7 +223,9 @@ fn execute_game_insn(cs: &CommandStream,
     }
 
     DeletePiece(piece) => {
-      let p = ig.pieces.remove(piece).ok_or(ME::PieceNotFound)?;
+      let modperm = ig.modify_pieces();
+      let p = ig.pieces.as_mut(modperm)
+        .remove(piece).ok_or(ME::PieceNotFound)?;
       let gs = &mut ig.gs;
       let pc = gs.pieces.remove(piece);
       let desc_html = p.describe_html(Some(Default::default()));
@@ -238,6 +240,7 @@ fn execute_game_insn(cs: &CommandStream,
     },
 
     AddPieces(PiecesSpec{ pos,posd,count,face,info }) => {
+      let modperm = ig.modify_pieces();
       let ig = &mut **ig;
       let gs = &mut ig.gs;
       let count = count.unwrap_or(1);
@@ -262,7 +265,7 @@ fn execute_game_insn(cs: &CommandStream,
           throw!(SpecError::PosOffTable);
         }
         let piece = gs.pieces.insert(pc);
-        ig.pieces.insert(piece, p);
+        ig.pieces.as_mut(modperm).insert(piece, p);
         updates.push((piece, PieceUpdateOp::Insert(())));
         pos[0] += posd[0];
         pos[1] += posd[1];
diff --git a/src/global.rs b/src/global.rs
index a296d2db..f6986e08 100644
--- a/src/global.rs
+++ b/src/global.rs
@@ -20,8 +20,6 @@ const GAME_SAVE_LAG : Duration = Duration::from_millis(500);
 #[repr(transparent)]
 pub struct RawTokenVal(str);
 
-pub type PiecesLoaded = SecondarySlotMap<PieceId,Box<dyn Piece>>;
-
 // ---------- public data structure ----------
 
 #[derive(Debug,Serialize,Deserialize)]
@@ -44,6 +42,13 @@ pub struct Instance {
   pub tokens_clients : TokenRegistry<ClientId>,
 }
 
+#[derive(Debug,Serialize,Deserialize)]
+#[serde(transparent)]
+pub struct PiecesLoaded (ActualPiecesLoaded);
+pub type ActualPiecesLoaded = SecondarySlotMap<PieceId,Box<dyn Piece>>;
+#[derive(Copy,Clone,Debug)]
+pub struct ModifyingPieces(());
+
 #[derive(Debug,Clone,Deserialize,Serialize)]
 #[derive(Eq,PartialEq,Ord,PartialOrd,Hash)]
 pub enum ManagementScope {
@@ -174,6 +179,7 @@ pub struct Global {
 pub struct InstanceContainer {
   live : bool,
   game_dirty : bool,
+  access_dirty : bool,
   g : Instance,
 }
 
@@ -238,13 +244,14 @@ impl Instance {
   /// Returns `None` if a game with this name already exists
   #[allow(clippy::new_ret_no_self)]
   #[throws(MgmtError)]
-  pub fn new(name: InstanceName, gs: GameState, pieces: PiecesLoaded)
+  pub fn new(name: InstanceName, gs: GameState)
              -> InstanceRef {
     let name = Arc::new(name);
 
     let g = Instance {
       name : name.clone(),
-      gs, pieces,
+      gs,
+      pieces : PiecesLoaded(Default::default()),
       clients : Default::default(),
       updates : Default::default(),
       tokens_players : Default::default(),
@@ -254,6 +261,7 @@ impl Instance {
     let cont = InstanceContainer {
       live : true,
       game_dirty : false,
+      access_dirty : false,
       g,
     };
 
@@ -530,6 +538,17 @@ impl InstanceGuard<'_> {
     out
   }
 
+  pub fn modify_pieces(&mut self) -> ModifyingPieces {
+    self.save_game_and_access_later();
+    // want this to be borrowed from self, so that we tie it properly
+    // to the same game.  But in practice we don't expect to write
+    // bugs where we get different games mixed up.  Borrowing self
+    // from the caller's pov is troublesome beczuse ultimately the
+    // caller will need to manipulate various fields of Instance (so
+    // we mustn't have a borrow of it).
+    ModifyingPieces(())
+  }
+
   fn token_register<Id:AccessId>(
     &mut self,
     token: RawToken,
@@ -554,6 +573,7 @@ impl InstanceGuard<'_> {
       !remove
     });
   }
+
 }
 
 // ---------- save/load ----------
@@ -636,6 +656,9 @@ impl InstanceGuard<'_> {
 
   #[throws(InternalError)]
   pub fn save_game_now(&mut self) {
+    if self.c.access_dirty {
+      self.save_access_now()?;
+    }
     self.save_something("g-", |s,w| {
       rmp_serde::encode::write_named(w, &s.c.g.gs)
     })?;
@@ -660,6 +683,7 @@ impl InstanceGuard<'_> {
       let isa = InstanceSaveAccesses { pieces, tokens_players };
       rmp_serde::encode::write_named(w, &isa)
     })?;
+    self.c.access_dirty = false;
     info!("saved accesses for {:?}", &self.name);
   }
 
@@ -688,7 +712,7 @@ impl InstanceGuard<'_> {
         })().context(lockfile).context("lock global save area")?);
       }
     }
-    let InstanceSaveAccesses::<String,PiecesLoaded>
+    let InstanceSaveAccesses::<String,ActualPiecesLoaded>
     { mut tokens_players, mut pieces } = Self::load_something(&name, "a-")
       .or_else(|e| {
         if let InternalError::Anyhow(ae) = &e {
@@ -723,7 +747,8 @@ impl InstanceGuard<'_> {
     );
 
     let g = Instance {
-      gs, pieces, updates,
+      gs, updates,
+      pieces: PiecesLoaded(pieces),
       name: name.clone(),
       clients : Default::default(),
       tokens_clients : Default::default(),
@@ -732,6 +757,7 @@ impl InstanceGuard<'_> {
     let cont = InstanceContainer {
       live: true,
       game_dirty: false,
+      access_dirty: false,
       g,
     };
     let gref = InstanceRef(Arc::new(Mutex::new(cont)));
@@ -870,6 +896,18 @@ pub fn record_token<Id : AccessId> (
   token
 }
 
+// ========== instance pieces data access ==========
+
+impl PiecesLoaded {
+  pub fn get(&self, piece: PieceId) -> Option<&Box<dyn Piece>> {
+    self.0.get(piece)
+  }
+
+  pub fn as_mut(&mut self, _: ModifyingPieces) -> &mut ActualPiecesLoaded {
+    &mut self.0
+  }
+}
+
 // ========== background maintenance ==========
 
 // ---------- delayed game save ----------
@@ -880,6 +918,12 @@ impl InstanceGuard<'_> {
     GLOBAL.dirty.lock().unwrap().push_back(self.gref.clone());
     self.c.game_dirty = true;
   }
+
+  pub fn save_game_and_access_later(&mut self) {
+    if self.c.access_dirty { return }
+    self.save_game_later();
+    self.c.access_dirty = true;
+  }
 }
 
 pub fn game_flush_task() {
diff --git a/src/updates.rs b/src/updates.rs
index 2d9ce6da..e4e78006 100644
--- a/src/updates.rs
+++ b/src/updates.rs
@@ -300,9 +300,9 @@ impl<'r> PrepareUpdatesBuffer<'r> {
 
     let (update, piece) = match (
       gs.pieces.byid_mut(piece),
-      self.g.pieces.byid(piece),
+      self.g.pieces.get(piece),
     ) {
-      (Ok(pc), Ok(p)) => {
+      (Ok(pc), Some(p)) => {
         gs.max_z.update_max(pc.zlevel.z);
 
         if self.by_client != pc.lastclient {