Correct localisation of numeric values, use coordinate conversions in rtsim

client/i18n/src/lib.rs

@@ -19,11 +19,11 @@ use std::{borrow::Cow, io};
 use assets::{source::DirEntry, AssetExt, AssetGuard, AssetHandle, ReloadWatcher, SharedString};
 use tracing::warn;
 // Re-export because I don't like prefix
-use common::comp::Content;
+use common::comp::{Content, LocalizationArg};
 use common_assets as assets;
 
 // Re-export for argument creation
-pub use fluent::fluent_args;
+pub use fluent::{fluent_args, FluentValue};
 pub use fluent_bundle::FluentArgs;
 
 /// The reference language, aka the more up-to-date localization data.
@@ -332,7 +332,7 @@ impl LocalizationGuard {
             })
     }
 
-    /// Localize the given context.
+    /// Localize the given content.
     pub fn get_content(&self, content: &Content) -> String {
         match content {
             Content::Plain(text) => text.clone(),
@@ -342,7 +342,14 @@ impl LocalizationGuard {
                     *seed,
                     &args
                         .iter()
-                        .map(|(k, content)| (k, self.get_content(content)))
+                        .map(|(k, arg)| {
+                            (k, match arg {
+                                LocalizationArg::Content(content) => {
+                                    FluentValue::String(self.get_content(content).into())
+                                },
+                                LocalizationArg::Nat(n) => FluentValue::from(n),
+                            })
+                        })
                         .collect(),
                 )
                 .into_owned(),

common/src/comp/chat.rs

@@ -199,20 +199,56 @@ pub enum Content {
         /// deterministic (but pseudorandom) localised output
         seed: u16,
         /// i18n arguments
-        args: HashMap<String, Content>,
+        args: HashMap<String, LocalizationArg>,
     },
 }
 
-// TODO: Remove impl and make use of `Plain` explicit (to discourage it)
+// TODO: Remove impl and make use of `Plain(...)` explicit (to discourage it)
 impl From<String> for Content {
     fn from(text: String) -> Self { Self::Plain(text) }
 }
 
-// TODO: Remove impl and make use of `Plain` explicit (to discourage it)
+// TODO: Remove impl and make use of `Plain(...)` explicit (to discourage it)
 impl<'a> From<&'a str> for Content {
     fn from(text: &'a str) -> Self { Self::Plain(text.to_string()) }
 }
 
+/// A localisation argument for localised content (see [`Content::Localized`]).
+#[derive(Debug, Clone, Serialize, Deserialize)]
+pub enum LocalizationArg {
+    /// The localisation argument is itself a section of content.
+    ///
+    /// Note that this allows [`Content`] to recursively refer to itself. It may
+    /// be tempting to decide to parameterise everything, having dialogue
+    /// generated with a compact tree. "It's simpler!", you might say. False.
+    /// Over-parameterisation is an anti-pattern that hurts translators. Where
+    /// possible, prefer fewer levels of nesting unless doing so would
+    /// result in an intractably larger number of combinations. See [here](https://github.com/projectfluent/fluent/wiki/Good-Practices-for-Developers#prefer-wet-over-dry) for the
+    /// guidance provided by the docs for `fluent`, the localisation library
+    /// used by clients.
+    Content(Content),
+    /// The localisation argument is a natural number
+    Nat(u64),
+}
+
+// TODO: Remove impl and make use of `Content(Plain(...))` explicit (to
+// discourage it)
+impl From<String> for LocalizationArg {
+    fn from(text: String) -> Self { Self::Content(Content::Plain(text)) }
+}
+
+// TODO: Remove impl and make use of `Content(Plain(...))` explicit (to
+// discourage it)
+impl<'a> From<&'a str> for LocalizationArg {
+    fn from(text: &'a str) -> Self { Self::Content(Content::Plain(text.to_string())) }
+}
+
+// TODO: Remove impl and make use of `Content(Plain(...))` explicit (to
+// discourage it)
+impl From<u64> for LocalizationArg {
+    fn from(n: u64) -> Self { Self::Nat(n) }
+}
+
 impl Content {
     pub fn localized(key: impl ToString) -> Self {
         Self::Localized {
@@ -222,7 +258,7 @@ impl Content {
         }
     }
 
-    pub fn localized_with_args<'a, A: Into<Content>>(
+    pub fn localized_with_args<'a, A: Into<LocalizationArg>>(
         key: impl ToString,
         args: impl IntoIterator<Item = (&'a str, A)>,
     ) -> Self {
@@ -341,6 +377,8 @@ impl<G> GenericChatMsg<G> {
 
     pub fn content(&self) -> &Content { &self.content }
 
+    pub fn into_content(self) -> Content { self.content }
+
     pub fn set_content(&mut self, content: Content) { self.content = content; }
 }
 

common/src/comp/mod.rs

@@ -75,8 +75,8 @@ pub use self::{
     },
     character_state::{CharacterActivity, CharacterState, StateUpdate},
     chat::{
-        ChatMode, ChatMsg, ChatType, Content, Faction, SpeechBubble, SpeechBubbleType,
+        ChatMode, ChatMsg, ChatType, Content, Faction, LocalizationArg, SpeechBubble,
-        UnresolvedChatMsg,
+        SpeechBubbleType, UnresolvedChatMsg,
     },
     combo::Combo,
     controller::{

common/src/terrain/mod.rs

@@ -84,7 +84,9 @@ pub trait CoordinateConversions {
 
 impl CoordinateConversions for Vec2<i32> {
     #[inline]
-    fn wpos_to_cpos(&self) -> Self { self.map2(TerrainChunkSize::RECT_SIZE, |e, sz| e / sz as i32) }
+    fn wpos_to_cpos(&self) -> Self {
+        self.map2(TerrainChunkSize::RECT_SIZE, |e, sz| e.div_euclid(sz as i32))
+    }
 
     #[inline]
     fn cpos_to_wpos(&self) -> Self { self.map2(TerrainChunkSize::RECT_SIZE, |e, sz| e * sz as i32) }

rtsim/src/data/npc.rs

@@ -12,8 +12,7 @@ use common::{
         Actor, ChunkResource, FactionId, NpcAction, NpcActivity, Personality, SiteId, VehicleId,
     },
     store::Id,
-    terrain::TerrainChunkSize,
+    terrain::CoordinateConversions,
-    vol::RectVolSize,
 };
 use hashbrown::{HashMap, HashSet};
 use rand::prelude::*;
@@ -349,12 +348,7 @@ impl Npcs {
         wpos: Vec3<f32>,
         radius: f32,
     ) -> impl Iterator<Item = Actor> + '_ {
-        let chunk_pos = wpos
+        let chunk_pos = wpos.xy().as_().wpos_to_cpos();
-            .xy()
-            .as_::<i32>()
-            .map2(TerrainChunkSize::RECT_SIZE.as_::<i32>(), |e, sz| {
-                e.div_euclid(sz)
-            });
         let r_sqr = radius * radius;
         LOCALITY
             .into_iter()

rtsim/src/rule/npc_ai.rs

@@ -16,9 +16,8 @@ use common::{
     rtsim::{ChunkResource, Profession, SiteId},
     spiral::Spiral2d,
     store::Id,
-    terrain::{SiteKindMeta, TerrainChunkSize},
+    terrain::{CoordinateConversions, SiteKindMeta, TerrainChunkSize},
     time::DayPeriod,
-    vol::RectVolSize,
 };
 use fxhash::FxHasher64;
 use itertools::{Either, Itertools};
@@ -333,7 +332,7 @@ where
         let wpos_site = |wpos: Vec2<f32>| {
             ctx.world
                 .sim()
-                .get(wpos.as_::<i32>() / TerrainChunkSize::RECT_SIZE.as_())
+                .get(wpos.as_().wpos_to_cpos())
                 .and_then(|chunk| chunk.sites.first().copied())
         };
 
@@ -568,7 +567,7 @@ fn hunt_animals() -> impl Action {
 }
 
 fn find_forest(ctx: &mut NpcCtx) -> Option<Vec2<f32>> {
-    let chunk_pos = ctx.npc.wpos.xy().as_() / TerrainChunkSize::RECT_SIZE.as_();
+    let chunk_pos = ctx.npc.wpos.xy().as_().wpos_to_cpos();
     Spiral2d::new()
         .skip(ctx.rng.gen_range(1..=8))
         .take(49)
@@ -801,8 +800,7 @@ fn chunk_path(
                 .filter_map(|p| Some((p, chunk_height(p)?)))
         },
         |(p0, h0), (p1, h1)| {
-            let diff =
+            let diff = (p0 - p1).as_().cpos_to_wpos().with_z((h0 - h1) as f32);
-                ((p0 - p1).as_() * TerrainChunkSize::RECT_SIZE.as_()).with_z((h0 - h1) as f32);
 
             diff.magnitude_squared()
         },
@@ -841,9 +839,8 @@ fn pilot() -> impl Action {
             })
             .choose(&mut ctx.rng);
         if let Some((_id, site)) = site {
-            let start_chunk =
+            let start_chunk = ctx.npc.wpos.xy().as_().wpos_to_cpos();
-                ctx.npc.wpos.xy().as_::<i32>() / TerrainChunkSize::RECT_SIZE.as_::<i32>();
+            let end_chunk = site.wpos.wpos_to_cpos();
-            let end_chunk = site.wpos / TerrainChunkSize::RECT_SIZE.as_::<i32>();
             chunk_path(start_chunk, end_chunk, |chunk| {
                 ctx.world
                     .sim()
@@ -861,7 +858,7 @@ fn pilot() -> impl Action {
 fn captain() -> impl Action {
     // For now just randomly travel the sea
     now(|ctx| {
-        let chunk = ctx.npc.wpos.xy().as_::<i32>() / TerrainChunkSize::RECT_SIZE.as_::<i32>();
+        let chunk = ctx.npc.wpos.xy().as_().wpos_to_cpos();
         if let Some(chunk) = NEIGHBORS
             .into_iter()
             .map(|neighbor| chunk + neighbor)

rtsim/src/rule/simulate_npcs.rs

@@ -6,8 +6,7 @@ use crate::{
 use common::{
     comp::{self, Body},
     rtsim::{Actor, NpcAction, NpcActivity, Personality},
-    terrain::TerrainChunkSize,
+    terrain::CoordinateConversions,
-    vol::RectVolSize,
 };
 use rand::prelude::*;
 use rand_chacha::ChaChaRng;
@@ -180,11 +179,7 @@ fn on_tick(ctx: EventCtx<SimulateNpcs, OnTick>) {
                                 | common::comp::ship::Body::AirBalloon => true,
                                 common::comp::ship::Body::SailBoat
                                 | common::comp::ship::Body::Galleon => {
-                                    let chunk_pos =
+                                    let chunk_pos = wpos.xy().as_().wpos_to_cpos();
-                                        wpos.xy().as_::<i32>().map2(
-                                            TerrainChunkSize::RECT_SIZE.as_::<i32>(),
-                                            |e, sz| e.div_euclid(sz),
-                                        );
                                     ctx.world
                                         .sim()
                                         .get(chunk_pos)

rtsim/src/rule/sync_npcs.rs

@@ -2,7 +2,7 @@ use crate::{
     event::{EventCtx, OnDeath, OnSetup, OnTick},
     RtState, Rule, RuleError,
 };
-use common::{grid::Grid, rtsim::Actor, terrain::TerrainChunkSize, vol::RectVolSize};
+use common::{grid::Grid, rtsim::Actor, terrain::CoordinateConversions};
 
 pub struct SyncNpcs;
 
@@ -45,7 +45,7 @@ fn on_tick(ctx: EventCtx<SyncNpcs, OnTick>) {
     let data = &mut *ctx.state.data_mut();
     // Update vehicle grid cells
     for (vehicle_id, vehicle) in data.npcs.vehicles.iter_mut() {
-        let chunk_pos = vehicle.wpos.xy().as_::<i32>() / TerrainChunkSize::RECT_SIZE.as_::<i32>();
+        let chunk_pos = vehicle.wpos.xy().as_().wpos_to_cpos();
         if vehicle.chunk_pos != Some(chunk_pos) {
             if let Some(cell) = vehicle
                 .chunk_pos
@@ -66,14 +66,7 @@ fn on_tick(ctx: EventCtx<SyncNpcs, OnTick>) {
         npc.current_site = ctx
             .world
             .sim()
-            .get(
+            .get(npc.wpos.xy().as_().wpos_to_cpos())
-                npc.wpos
-                    .xy()
-                    .as_::<i32>()
-                    .map2(TerrainChunkSize::RECT_SIZE.as_::<i32>(), |e, sz| {
-                        e.div_euclid(sz)
-                    }),
-            )
             .and_then(|chunk| {
                 chunk
                     .sites
@@ -99,13 +92,7 @@ fn on_tick(ctx: EventCtx<SyncNpcs, OnTick>) {
         }
 
         // Update the NPC's grid cell
-        let chunk_pos = npc
+        let chunk_pos = npc.wpos.xy().as_().wpos_to_cpos();
-            .wpos
-            .xy()
-            .as_::<i32>()
-            .map2(TerrainChunkSize::RECT_SIZE.as_::<i32>(), |e, sz| {
-                e.div_euclid(sz)
-            });
         if npc.chunk_pos != Some(chunk_pos) {
             if let Some(cell) = npc
                 .chunk_pos

server/agent/src/action_nodes.rs

@@ -1515,6 +1515,8 @@ impl<'a> AgentData<'a> {
         }
     }
 
+    // TODO: Pass a localisation key instead of `Content` to avoid allocating if
+    // we're not permitted to speak.
     pub fn chat_npc_if_allowed_to_speak(
         &self,
         msg: Content,

server/src/rtsim/mod.rs

@@ -201,7 +201,10 @@ impl RtSim {
         debug!("Saving rtsim data...");
 
         // Create the save thread if it doesn't already exist
-        // TODO: Use the slow job pool eventually
+        // We're not using the slow job pool here for two reasons:
+        // 1) The thread is mostly blocked on IO, not compute
+        // 2) We need to synchronise saves to ensure monotonicity, which slow jobs
+        // aren't designed to allow
         let (tx, _) = self.save_thread.get_or_insert_with(|| {
             trace!("Starting rtsim data save thread...");
             let (tx, rx) = unbounded();

server/src/rtsim/tick.rs

@@ -9,9 +9,8 @@ use common::{
     resources::{DeltaTime, Time, TimeOfDay},
     rtsim::{Actor, RtSimEntity, RtSimVehicle},
     slowjob::SlowJobPool,
-    terrain::{CoordinateConversions, TerrainChunkSize},
+    terrain::CoordinateConversions,
     trade::{Good, SiteInformation},
-    vol::RectVolSize,
     LoadoutBuilder,
 };
 use common_ecs::{Job, Origin, Phase, System};
@@ -236,13 +235,7 @@ impl<'a> System<'a> for Sys {
             data.npcs.character_map.clear();
             for (presence, wpos) in (&presences, &positions).join() {
                 if let PresenceKind::Character(character) = &presence.kind {
-                    let chunk_pos = wpos
+                    let chunk_pos = wpos.0.xy().as_().wpos_to_cpos();
-                        .0
-                        .xy()
-                        .as_::<i32>()
-                        .map2(TerrainChunkSize::RECT_SIZE.as_::<i32>(), |e, sz| {
-                            e.div_euclid(sz)
-                        });
                     data.npcs
                         .character_map
                         .entry(chunk_pos)

server/src/state_ext.rs

@@ -874,7 +874,7 @@ impl StateExt for State {
                     } else {
                         self.notify_players(ServerGeneral::server_msg(
                             comp::ChatType::Kill(kill_source.clone(), *uid),
-                            msg.content().clone(),
+                            msg.into_content(),
                         ))
                     }
                 },

voxygen/src/session/mod.rs

@@ -384,8 +384,7 @@ impl SessionState {
                         .new_message(ChatType::CommandError.into_msg(match time {
                             0 => Content::localized("hud-chat-goodbye"),
                             _ => Content::localized_with_args("hud-chat-connection_lost", [(
-                                "time",
+                                "time", time,
-                                time.to_string(),
                             )]),
                         }));
                 },
@@ -988,7 +987,7 @@ impl PlayState for SessionState {
                                                     self.hud.new_message(ChatType::Meta.into_msg(
                                                         Content::localized_with_args(
                                                             "hud-trade-invite_sent",
-                                                            [("playername", name.as_str())],
+                                                            [("playername", name)],
                                                         ),
                                                     ));
                                                     client.send_invite(uid, InviteKind::Trade)