Consolidate Vocabulary

[ChrisRenfrow]

I'm submitting this as a draft so we can discuss how to properly handle these changes as I go.

---

For the time being I've taken a selection of the word generators from each of the existing name generators and added them as public functions to world/vocabulary.rs which is then declared as a module in world/mod.rs. But something I feel like is within the scope for this set of changes is making it easier/more-consistent for name-generators to make their own specialized word-generators, that is, the things that aren't so general that they should be included in vocabulary.rs. This should also DRY up some of the word-generators already defined within vocabulary.rs promoting readability and streamlining future adjustments.

What I have in mind for this is to implement a word-generator struct in which is initialized with an array of words and provides a single method that accepts something that implements Rng which, on invocation, spits out one of the words in the selection. This is simple enough as it's basically just centralizing the behavior defined in each of the existing word-generators.

If for the moment we just need to have the vocabulary in one centralized place to continue, I'm more than happy to propagate this change to the existing name-generators and then submit for review. Otherwise I'd be happy to take a little bit more time to make this addition a little "smarter."

[ChrisRenfrow]

@MikkelPaulson Is there a way to disable CI on drafts? Doing so might generate less noise, but if that would disrupt preferred workflows feel free to ignore this.

[MikkelPaulson]

Interesting. I was unconvinced, so I did it. There's not that much boilerplate from a LoC standpoint, but it's true that it has less complexity/cognitive overhead to do something like:

diff --git a/core/src/world/vocabulary.rs b/core/src/world/vocabulary.rs
index b1e64c75..dea1b720 100644
--- a/core/src/world/vocabulary.rs
+++ b/core/src/world/vocabulary.rs
@@ -1,4 +1,4 @@
-use rand::Rng;
+use rand::prelude::*;

 pub fn adjective(rng: &mut impl Rng) -> &'static str {
     #[rustfmt::skip]
@@ -104,12 +104,38 @@ pub fn land_animal(rng: &mut impl Rng) -> &'static str {
     LAND_ANIMALS[rng.gen_range(0..LAND_ANIMALS.len())]
 }

+#[rustfmt::skip]
 pub fn coastal_animal(rng: &mut impl Rng) -> &'static str {
-    #[rustfmt::skip]
-    const COASTAL_ANIMALS: &[&str] = &[
-        "Cormorant", "Crab", "Dolphin", "Herring", "Mermaid", "Octopus", "Osprey", "Otter",
-        "Pelican", "Perch", "Salmon", "Seagull", "Seal", "Shark", "Starfish", "Squid", "Whale",
-        "Whelk",
-    ];
-    COASTAL_ANIMALS[rng.gen_range(0..COASTAL_ANIMALS.len())]
+    ListGenerator(&["Cormorant", "Crab", "Dolphin", "Herring", "Mermaid", "Octopus", "Osprey",
+        "Otter", "Pelican", "Perch", "Salmon", "Seagull", "Seal", "Shark", "Starfish", "Squid",
+        "Whale", "Whelk"])
+    .gen(rng)
+}
+
+pub struct ListGenerator(pub &'static [&'static str]);
+
+impl ListGenerator {
+    pub fn gen(&self, rng: &mut impl Rng) -> &'static str {
+        self.0[rng.gen_range(0..self.0.len())]
+    }
+}
+
+#[cfg(test)]
+mod test {
+    use super::*;
+
+    #[test]
+    fn list_generator_test() {
+        let mut rng = SmallRng::seed_from_u64(0);
+
+        assert_eq!(
+            ["Pelican", "Pelican", "Whale", "Pelican", "Otter"]
+                .iter()
+                .map(|s| s.to_string())
+                .collect::<Vec<_>>(),
+            (0..5)
+                .map(|_| coastal_animal(&mut rng))
+                .collect::<Vec<&'static str>>(),
+        );
+    }
 }

I wish we didn't need the #[rustfmt::skip] everywhere, but it does get awfully verbose if you let it splat all of your vocabulary onto its own line.

@MikkelPaulson Is there a way to disable CI on drafts? Doing so might generate less noise, but if that would disrupt preferred workflows feel free to ignore this.

I don't think so. At best you might be able to make the CI run pass without error on a draft PR, but since test runs are associated with commits in the git history, I'm not sure that's a good idea, since you'd end up with failing commits marked as passing.

[ChrisRenfrow]

Oh that looks great! I wouldn't have thought to use a tuple struct for this but it makes sense here.

And yeah, you're right, though it's not a lot of boilerplate, a little bit of abstraction can add up to great effect over time.

No problem re: disabling CI for drafts. I'll probably use drafts more sparingly or be mindful of the draft status and push less frequently (<2/d) so less CI chatter is generated.

Anyway, I'll go ahead and use what you've presented here as is and start converting each existing name generator to use this new pattern. I've taken a note to do a better job of convincing you/others on the validity of my claims going forward. 😃 Thank you!

[ChrisRenfrow]

Re using rustfmt::skip, what do you think about enabling #![feature(stmt_expr_attributes)] in lib.rs? This would make it so that we can keep the skip focused to just the list initialization and return statement rather than having to include the entire function block here and elsewhere.

Like so:

-#[rustfmt::skip]
 pub fn coastal_animal(rng: &mut impl Rng) -> &'static str {
+    #[rustfmt::skip]
     ListGenerator(&[
         "Cormorant", "Crab", "Dolphin", "Herring", "Mermaid", "Octopus", "Osprey", "Otter",
         "Pelican", "Perch", "Salmon", "Seagull", "Seal", "Shark", "Starfish", "Squid", "Whale",

[ChrisRenfrow]

Alright, I think this is ready for review! The probabilities changed a little during the process of centralizing the generators. Most notably within any_animal there is now a 50% chance of triggering either coastal_animal or land_animal, which is inflated from when all animals were in a single list including the minority of coastal animals. This seems most notable in shrine's test output.

diff --git a/core/src/world/place/building/religious/shrine.rs b/core/src/world/place/building/religious/shrine.rs
index 8e8a0db..cb753d9 100644
--- a/core/src/world/place/building/religious/shrine.rs
+++ b/core/src/world/place/building/religious/shrine.rs
@@ -125,26 +125,26 @@ mod test {

         assert_eq!(
             [
-                "Shrine of the Wolf",
-                "Shrine of Courage",
-                "Shrine of Foul Decay",
+                "Shrine of the Pelican",
+                "Place Where the Weasels Drown",
+                "The Gold Pillar",
                 "The Singing Cave",
                 "The Fading Basin",
-                "The Exalted Gate",
+                "The Grey Gate",
                 "The Creeping Shrine",
-                "The Alabaster Shrine",
-                "Pillar of the Five Deer",
-                "The Pale Pagoda",
-                "The Immortal Shrine",
-                "Place Where the Tigers Sing",
-                "Tree of Shining Textiles",
-                "The Hunting Shrine",
+                "The Red Shrine",
+                "Pillar of the Five Camels",
+                "The Wasted Pagoda",
+                "Shrine of the Empress",
+                "The Singing Shrine",
+                "Place Where the Otters Shine",
+                "The Orange Tree",
                 "The Creeping Shrine",
-                "Shrine of the Thirty-Six Snakes",
-                "Shrine of the Iron Deer",
-                "The Spirit Altar",
-                "Shrine of Forgiveness",
-                "The Dark Shrine"
+                "Shrine of the Thirty-Six Sharks",
+                "Shrine of the Wild Seagull",
+                "Pillars of the Five Otters",
+                "Shrine of Laughing Forgiveness",
+                "The Thirsty Pillar"
             ]
             .iter()
             .map(|s| s.to_string())

I'm not sure if this is something to address here/now, but I just wanted to make sure it was acknowledged.

[MikkelPaulson]

Alright, I think this is ready for review! The probabilities changed a little during the process of centralizing the generators. Most notably within any_animal there is now a 50% chance of triggering either coastal_animal or land_animal, which is inflated from when all animals were in a single list including the minority of coastal animals. This seems most notable in shrine's test output.

I'm not sure if this is something to address here/now, but I just wanted to make sure it was acknowledged.

Yeah, I feel like that's going to be a problem. It doesn't need to be exact, but if you have six types of flying_animal, you don't want them to have the same weight as the 30 types of land_animal. I think the solution will be to move the data into constants outside of the functions, and then use a weighted distribution:

use rand::prelude::*;
use rand::distributions::WeightedIndex;

const LAND_ANIMALS: &[&str] = &["Alpaca", "Boar", "Camel"][..];
const SEA_ANIMALS: &[&str] = &["Dolphin", "Eel", "Fish"][..];
const AIR_ANIMALS: &[&str] = &["Gull", "Hawk"][..];

pub fn animal(rng: &mut impl Rng) -> &'static str {
    let weights = [LAND_ANIMALS.len(), SEA_ANIMALS.len(), AIR_ANIMALS.len()];
    let dist = WeightedIndex::new(&weights).unwrap();

    match dist.sample(&mut rng) {
        0 => land_animal(rng),
        1 => sea_animal(rng),
        2 => air_animal(rng),
        _ => unreachable!(),
    }
}

pub fn air_animal(rng: &mut impl Rng) -> &'static str {
    ListGenerator(AIR_ANIMALS).gen(rng)
}

(Doing this off the cuff, mind you, so it probably won't compile.)

[ChrisRenfrow]

Ooh, nice! I didn't know about rand's WeightedIndex but that sounds like exactly what we need here. I'll see about integrating it with any_animal today.

[MikkelPaulson]

I actually wrote another implementation using subslices as a proposed alternative, but it was objectively worse in every way, so I trashed it.

[ChrisRenfrow]

Looking a lot better now!

diff --git a/core/src/world/place/building/religious/shrine.rs b/core/src/world/place/building/religious/shrine.rs
index cb753d9..31688bb 100644
--- a/core/src/world/place/building/religious/shrine.rs
+++ b/core/src/world/place/building/religious/shrine.rs
@@ -137,14 +137,14 @@ mod test {
                 "The Wasted Pagoda",
                 "Shrine of the Empress",
                 "The Singing Shrine",
-                "Place Where the Otters Shine",
+                "Place Where the Unicorns Weep",
+                "Gate of the Emperor",
                 "The Orange Tree",
                 "The Creeping Shrine",
-                "Shrine of the Thirty-Six Sharks",
-                "Shrine of the Wild Seagull",
-                "Pillars of the Five Otters",
-                "Shrine of Laughing Forgiveness",
-                "The Thirsty Pillar"
+                "Gate of the Thirty-Six Rams",
+                "Shrine of the Wild Cat",
+                "The Wasted Altar",
+                "Shrine of Forgiveness"
             ]
             .iter()
             .map(|s| s.to_string())

[MikkelPaulson]

Test build:

https://1c2e168f.initiativesh.pages.dev/

[MikkelPaulson]

Updated test build: https://e6ec05ad.initiativesh.pages.dev/

Looks good to merge on your :+1:.

[ChrisRenfrow]

LGTM