From 1274dc43c8b96ee5b7f408702a59410b74253086 Mon Sep 17 00:00:00 2001
From: Maxicarlos08 <maxicarlos08@gmail.com>
Date: Sat, 19 Aug 2023 12:43:48 +0200
Subject: [PATCH] document and cleanup StateExt::position_mut

---
 server/src/cmd.rs                        | 27 ++++++++++++++++--------
 server/src/events/entity_manipulation.rs |  2 +-
 server/src/state_ext.rs                  | 22 +++++++++++--------
 3 files changed, 32 insertions(+), 19 deletions(-)

diff --git a/server/src/cmd.rs b/server/src/cmd.rs
index 6c707dd2d9..6072e3cd3a 100644
--- a/server/src/cmd.rs
+++ b/server/src/cmd.rs
@@ -777,9 +777,10 @@ fn handle_jump(
     {
         server
             .state
-            .position_mut(target, dismount_volume, |current_pos| {
+            .position_mut(target, dismount_volume.unwrap_or(true), |current_pos| {
                 current_pos.0 += Vec3::new(x, y, z)
             })
+            .map_err(ToString::to_string)
     } else {
         Err(action.help_string())
     }
@@ -796,9 +797,10 @@ fn handle_goto(
     {
         server
             .state
-            .position_mut(target, dismount_volume, |current_pos| {
+            .position_mut(target, dismount_volume.unwrap_or(true), |current_pos| {
                 current_pos.0 = Vec3::new(x, y, z)
             })
+            .map_err(ToString::to_string)
     } else {
         Err(action.help_string())
     }
@@ -833,9 +835,10 @@ fn handle_site(
 
         server
             .state
-            .position_mut(target, dismount_volume, |current_pos| {
+            .position_mut(target, dismount_volume.unwrap_or(true), |current_pos| {
                 current_pos.0 = site_pos
             })
+            .map_err(ToString::to_string)
     } else {
         Err(action.help_string())
     }
@@ -860,9 +863,10 @@ fn handle_respawn(
 
     server
         .state
-        .position_mut(target, Some(true), |current_pos| {
+        .position_mut(target, true, |current_pos| {
             current_pos.0 = waypoint;
         })
+        .map_err(ToString::to_string)
 }
 
 fn handle_kill(
@@ -1234,9 +1238,10 @@ fn handle_tp(
     let player_pos = position(server, player, "player")?;
     server
         .state
-        .position_mut(target, dismount_volume, |target_pos| {
+        .position_mut(target, dismount_volume.unwrap_or(true), |target_pos| {
             *target_pos = player_pos
         })
+        .map_err(ToString::to_string)
 }
 
 fn handle_rtsim_tp(
@@ -1266,9 +1271,10 @@ fn handle_rtsim_tp(
     };
     server
         .state
-        .position_mut(target, dismount_volume, |target_pos| {
+        .position_mut(target, dismount_volume.unwrap_or(true), |target_pos| {
             target_pos.0 = pos;
         })
+        .map_err(ToString::to_string)
 }
 
 fn handle_rtsim_info(
@@ -4040,9 +4046,12 @@ fn handle_location(
     if let Some(name) = parse_cmd_args!(args, String) {
         let loc = server.state.ecs().read_resource::<Locations>().get(&name);
         match loc {
-            Ok(loc) => server.state.position_mut(target, Some(true), |target_pos| {
-                target_pos.0 = loc;
-            }),
+            Ok(loc) => server
+                .state
+                .position_mut(target, true, |target_pos| {
+                    target_pos.0 = loc;
+                })
+                .map_err(ToString::to_string),
             Err(e) => Err(e.to_string()),
         }
     } else {
diff --git a/server/src/events/entity_manipulation.rs b/server/src/events/entity_manipulation.rs
index f21acf829a..6d6f7b8ee9 100644
--- a/server/src/events/entity_manipulation.rs
+++ b/server/src/events/entity_manipulation.rs
@@ -1661,7 +1661,7 @@ pub fn handle_remove_light_emitter(server: &mut Server, entity: EcsEntity) {
 pub fn handle_teleport_to_position(server: &mut Server, entity: EcsEntity, position: Vec3<f32>) {
     if let Err(error) = server
         .state
-        .position_mut(entity, Some(true), |pos| pos.0 = position)
+        .position_mut(entity, true, |pos| pos.0 = position)
     {
         warn!("Failed to teleport entity: {error}");
     }
diff --git a/server/src/state_ext.rs b/server/src/state_ext.rs
index 225027f64d..58e8087c6c 100644
--- a/server/src/state_ext.rs
+++ b/server/src/state_ext.rs
@@ -158,12 +158,18 @@ pub trait StateExt {
     ) -> Result<(), specs::error::WrongGeneration>;
     /// Get the given entity as an [`Actor`], if it is one.
     fn entity_as_actor(&self, entity: EcsEntity) -> Option<Actor>;
+    /// Mutate the position of an entity or, if the entity is mounted, the
+    /// mount.
+    ///
+    /// If `dismount_volume` is `true`, an entity mounted on a volume entity
+    /// (such as an airship) will be dismounted to avoid teleporting the volume
+    /// entity.
     fn position_mut<T>(
         &mut self,
         entity: EcsEntity,
-        dismount_volume: Option<bool>,
+        dismount_volume: bool,
         f: impl for<'a> FnOnce(&'a mut comp::Pos) -> T,
-    ) -> Result<T, String>;
+    ) -> Result<T, &'static str>;
 }
 
 impl StateExt for State {
@@ -1247,10 +1253,10 @@ impl StateExt for State {
     fn position_mut<T>(
         &mut self,
         entity: EcsEntity,
-        dismount_volume: Option<bool>,
+        dismount_volume: bool,
         f: impl for<'a> FnOnce(&'a mut comp::Pos) -> T,
-    ) -> Result<T, String> {
-        if dismount_volume.unwrap_or(true) {
+    ) -> Result<T, &'static str> {
+        if dismount_volume {
             self.ecs().write_storage::<Is<VolumeRider>>().remove(entity);
         }
 
@@ -1268,9 +1274,7 @@ impl StateExt for State {
                     .get(entity)
                     .and_then(|volume_rider| {
                         Some(match volume_rider.pos.kind {
-                            common::mounting::Volume::Terrain => {
-                                Err("Tried to move the world.".to_string())
-                            },
+                            common::mounting::Volume::Terrain => Err("Tried to move the world."),
                             common::mounting::Volume::Entity(uid) => {
                                 Ok(self.ecs().read_resource::<IdMaps>().uid_entity(uid)?)
                             },
@@ -1290,7 +1294,7 @@ impl StateExt for State {
                 maybe_pos = Some(pos.0);
                 res
             })
-            .ok_or_else(|| "Cannot get position for entity!".to_string());
+            .ok_or("Cannot get position for entity!");
 
         if let Some(pos) = maybe_pos {
             if self