Remove panics and unwraps, part 1.

Also fixes various other miscellaneous bugs.

assets/voxygen/i18n/en/main.ron

@@ -41,6 +41,7 @@ You can create an account over at
 https://veloren.net/account/."#,
         "main.login.server_not_found": "Server not found",
         "main.login.authentication_error": "Auth error on server",
+        "main.login.internal_error": "Internal error on client (most likely, player character was deleted)",
         "main.login.failed_auth_server_url_invalid": "Failed to connect to auth server",
         "main.login.insecure_auth_scheme": "The auth Scheme HTTP is NOT supported. It's insecure! For development purposes, HTTP is allowed for 'localhost' or debug builds",
         "main.login.server_full": "Server is full",

client/src/error.rs

@@ -1,6 +1,7 @@
 use authc::AuthClientError;
 pub use network::{InitProtocolError, NetworkConnectError, NetworkError};
 use network::{ParticipantError, StreamError};
+use specs::error::Error as SpecsError;
 
 #[derive(Debug)]
 pub enum Error {
@@ -22,6 +23,11 @@ pub enum Error {
     InvalidCharacter,
     //TODO: InvalidAlias,
     Other(String),
+    SpecsErr(SpecsError),
+}
+
+impl From<SpecsError> for Error {
+    fn from(err: SpecsError) -> Self { Self::SpecsErr(err) }
 }
 
 impl From<NetworkError> for Error {

client/src/lib.rs

@@ -852,8 +852,9 @@ impl Client {
     pub fn request_remove_character(&mut self) { self.send_msg(ClientGeneral::ExitInGame); }
 
     pub fn set_view_distance(&mut self, view_distance: u32) {
-        self.view_distance = Some(view_distance.max(1).min(65));
-        self.send_msg(ClientGeneral::SetViewDistance(self.view_distance.unwrap()));
+        let view_distance = view_distance.max(1).min(65);
+        self.view_distance = Some(view_distance);
+        self.send_msg(ClientGeneral::SetViewDistance(view_distance));
     }
 
     pub fn use_slot(&mut self, slot: Slot) {
@@ -1381,13 +1382,13 @@ impl Client {
                 if let Some(client_character_state) =
                     ecs.read_storage::<comp::CharacterState>().get(entity)
                 {
-                    if last_character_states
-                        .get(entity)
-                        .map(|l| !client_character_state.same_variant(&l.0))
-                        .unwrap_or(true)
+                    if let Some(l) = last_character_states
+                        .entry(entity)
+                        .ok()
+                        .map(|l| l.or_insert_with(|| comp::Last(client_character_state.clone())))
+                        .filter(|l| !client_character_state.same_variant(&l.0))
                     {
-                        let _ = last_character_states
-                            .insert(entity, comp::Last(client_character_state.clone()));
+                        *l = comp::Last(client_character_state.clone());
                     }
                 }
             }
@@ -1815,7 +1816,22 @@ impl Client {
                     InventoryUpdateEvent::CollectFailed => {},
                     _ => {
                         // Push the updated inventory component to the client
-                        self.state.write_component(self.entity(), inventory);
+                        // FIXME: Figure out whether this error can happen under normal gameplay,
+                        // if not find a better way to handle it, if so maybe consider kicking the
+                        // client back to login?
+                        let entity = self.entity();
+                        if let Err(e) = self
+                            .state
+                            .ecs_mut()
+                            .write_storage()
+                            .insert(entity, inventory)
+                        {
+                            warn!(
+                                ?e,
+                                "Received an inventory update event for client entity, but this \
+                                 entity was not found... this may be a bug."
+                            );
+                        }
                     },
                 }
 
@@ -2353,14 +2369,12 @@ impl Drop for Client {
             trace!("no disconnect msg necessary as client wasn't registered")
         }
 
-        tokio::task::block_in_place(|| {
-            if let Err(e) = self
-                .runtime
-                .block_on(self.participant.take().unwrap().disconnect())
-            {
-                warn!(?e, "error when disconnecting, couldn't send all data");
-            }
-        });
+        self.runtime.spawn(
+            self.participant
+                .take()
+                .expect("Only set to None in Drop")
+                .disconnect(),
+        );
     }
 }
 

common/ecs/src/system.rs

@@ -77,7 +77,12 @@ impl CpuTimeline {
     fn end(&mut self) -> std::time::Duration {
         let end = Instant::now();
         self.measures.push((end, ParMode::None));
-        end.duration_since(self.measures.first().unwrap().0)
+        end.duration_since(
+            self.measures
+                .first()
+                .expect("We just pushed onto the vector.")
+                .0,
+        )
     }
 
     fn get(&self, time: Instant) -> ParMode {
@@ -181,9 +186,17 @@ pub fn gen_stats(
         // update ALL states
         for individual in individual_cores_wanted.iter() {
             let actual = (individual.1 as f32 / total_or_max) * physical_threads as f32;
-            let p = result.get_mut(individual.0).unwrap();
-            if (p.measures.last().unwrap().1 - actual).abs() > 0.0001 {
-                p.measures.push((relative_time, actual));
+            if let Some(p) = result.get_mut(individual.0) {
+                if p.measures
+                    .last()
+                    .map(|last| (last.1 - actual).abs())
+                    .unwrap_or(0.0)
+                    > 0.0001
+                {
+                    p.measures.push((relative_time, actual));
+                }
+            } else {
+                tracing::warn!("Invariant violation: keys in both hashmaps should be the same.");
             }
         }
     }

common/net/src/sync/mod.rs

@@ -10,7 +10,7 @@ pub use common::uid::{Uid, UidAllocator};
 pub use packet::{
     handle_insert, handle_interp_insert, handle_interp_modify, handle_interp_remove, handle_modify,
     handle_remove, CompPacket, CompSyncPackage, EntityPackage, EntitySyncPackage,
-    InterpolatableComponent, StatePackage,
+    InterpolatableComponent,
 };
 pub use sync_ext::WorldSyncExt;
 pub use track::UpdateTracker;

common/net/src/sync/packet.rs

@@ -104,36 +104,6 @@ pub struct EntityPackage<P: CompPacket> {
     pub comps: Vec<P>,
 }
 
-#[derive(Clone, Debug, Serialize, Deserialize)]
-pub struct StatePackage<P: CompPacket> {
-    pub entities: Vec<EntityPackage<P>>,
-}
-
-impl<P: CompPacket> Default for StatePackage<P> {
-    fn default() -> Self {
-        Self {
-            entities: Vec::new(),
-        }
-    }
-}
-
-impl<P: CompPacket> StatePackage<P> {
-    pub fn new() -> Self { Self::default() }
-
-    pub fn with_entities<C: Component + Clone + Send + Sync>(
-        mut self,
-        mut entities: Vec<EntityPackage<P>>,
-    ) -> Self {
-        self.entities.append(&mut entities);
-        self
-    }
-
-    pub fn with_entity(mut self, entry: EntityPackage<P>) -> Self {
-        self.entities.push(entry);
-        self
-    }
-}
-
 #[derive(Clone, Debug, Serialize, Deserialize)]
 pub struct EntitySyncPackage {
     pub created_entities: Vec<u64>,

common/net/src/sync/sync_ext.rs

@@ -1,7 +1,5 @@
 use super::{
-    packet::{
-        CompPacket, CompSyncPackage, CompUpdateKind, EntityPackage, EntitySyncPackage, StatePackage,
-    },
+    packet::{CompPacket, CompSyncPackage, CompUpdateKind, EntityPackage, EntitySyncPackage},
     track::UpdateTracker,
 };
 use common::{
@@ -31,7 +29,6 @@ pub trait WorldSyncExt {
         &mut self,
         entity_package: EntityPackage<P>,
     ) -> specs::Entity;
-    fn apply_state_package<P: CompPacket>(&mut self, state_package: StatePackage<P>);
     fn apply_entity_sync_package(&mut self, package: EntitySyncPackage);
     fn apply_comp_sync_package<P: CompPacket>(&mut self, package: CompSyncPackage<P>);
 }
@@ -99,19 +96,6 @@ impl WorldSyncExt for specs::World {
         }
     }
 
-    fn apply_state_package<P: CompPacket>(&mut self, state_package: StatePackage<P>) {
-        let StatePackage { entities } = state_package;
-
-        // Apply state package entities
-        for entity_package in entities {
-            self.apply_entity_package(entity_package);
-        }
-
-        // TODO: determine if this is needed
-        // Initialize entities
-        //self.maintain();
-    }
-
     fn apply_entity_sync_package(&mut self, package: EntitySyncPackage) {
         // Take ownership of the fields
         let EntitySyncPackage {

common/src/character.rs

@@ -18,7 +18,7 @@ pub struct Character {
 
 /// Data needed to render a single character item in the character list
 /// presented during character selection.
-#[derive(Clone, Debug, PartialEq, Serialize, Deserialize)]
+#[derive(Clone, Debug, Serialize, Deserialize)]
 pub struct CharacterItem {
     pub character: Character,
     pub body: comp::Body,

common/src/comp/chat.rs

@@ -44,8 +44,8 @@ impl ChatMode {
     }
 }
 
-impl Default for ChatMode {
-    fn default() -> Self { Self::World }
+impl ChatMode {
+    pub const fn default() -> Self { Self::World }
 }
 
 #[derive(Debug, Clone, Serialize, Deserialize)]

common/src/comp/inputs.rs

@@ -1,8 +1,8 @@
 use crate::depot::Id;
+use hashbrown::HashSet;
 use serde::{Deserialize, Serialize};
 use specs::{Component, DerefFlaggedStorage};
 use specs_idvs::IdvStorage;
-use std::collections::HashSet;
 use vek::geom::Aabb;
 
 #[derive(Clone, Debug, Default, PartialEq, Serialize, Deserialize)]

common/src/comp/inventory/item/mod.rs

@@ -17,16 +17,17 @@ use crate::{
     recipe::RecipeInput,
     terrain::{Block, SpriteKind},
 };
-use core::mem;
+use core::{
+    convert::TryFrom,
+    mem,
+    num::{NonZeroU32, NonZeroU64},
+    ops::Deref,
+};
 use crossbeam_utils::atomic::AtomicCell;
 use serde::{Deserialize, Serialize};
 use specs::{Component, DerefFlaggedStorage};
 use specs_idvs::IdvStorage;
-use std::{
-    num::{NonZeroU32, NonZeroU64},
-    ops::Deref,
-    sync::Arc,
-};
+use std::sync::Arc;
 use vek::Rgb;
 
 #[derive(Clone, Copy, Debug, PartialEq, Eq, Hash, Serialize, Deserialize)]
@@ -185,6 +186,12 @@ pub struct CreateDatabaseItemId {
     item_id: Arc<ItemId>,
 }*/
 
+/// NOTE: Do not call `Item::clone` without consulting the core devs!  It only
+/// exists due to being required for message serialization at the moment, and
+/// should not be used for any other purpose.
+///
+/// FIXME: Turn on a Clippy lint forbidding the use of `Item::clone` using the
+/// `disallowed_method` feature.
 #[derive(Clone, Debug, Serialize, Deserialize)]
 pub struct Item {
     /// item_id is hidden because it represents the persistent, storage entity
@@ -242,21 +249,28 @@ pub struct ItemConfig {
     pub dodge_ability: Option<CharacterAbility>,
 }
 
-impl From<(&ItemKind, &[Item], &AbilityMap, &MaterialStatManifest)> for ItemConfig {
-    fn from(
+#[derive(Debug)]
+pub enum ItemConfigError {
+    BadItemKind,
+}
+
+impl TryFrom<(&ItemKind, &[Item], &AbilityMap, &MaterialStatManifest)> for ItemConfig {
+    type Error = ItemConfigError;
+
+    fn try_from(
         (item_kind, components, map, msm): (&ItemKind, &[Item], &AbilityMap, &MaterialStatManifest),
-    ) -> Self {
+    ) -> Result<Self, Self::Error> {
         if let ItemKind::Tool(tool) = item_kind {
             let abilities = tool.get_abilities(msm, components, map);
 
-            return ItemConfig {
+            Ok(ItemConfig {
                 abilities,
                 block_ability: None,
                 dodge_ability: Some(CharacterAbility::default_roll()),
-            };
+            })
+        } else {
+            Err(ItemConfigError::BadItemKind)
         }
-
-        unimplemented!("ItemConfig is currently only supported for Tools")
     }
 }
 
@@ -314,6 +328,12 @@ impl ItemDef {
     }
 }
 
+/// NOTE: This PartialEq instance is pretty broken!  It doesn't check item
+/// amount or any child items (and, arguably, doing so should be able to ignore
+/// things like item order within the main inventory or within each bag, and
+/// possibly even coalesce amounts, though these may be more controversial).
+/// Until such time as we find an actual need for a proper PartialEq instance,
+/// please don't rely on this for anything!
 impl PartialEq for Item {
     fn eq(&self, other: &Self) -> bool {
         self.item_def.item_definition_id == other.item_def.item_definition_id
@@ -461,7 +481,20 @@ impl Item {
 
     /// Duplicates an item, creating an exact copy but with a new item ID
     pub fn duplicate(&self, msm: &MaterialStatManifest) -> Self {
-        Item::new_from_item_def(Arc::clone(&self.item_def), &self.components, msm)
+        let mut new_item =
+            Item::new_from_item_def(Arc::clone(&self.item_def), &self.components, msm);
+        new_item.set_amount(self.amount()).expect(
+            "`new_item` has the same `item_def` and as an invariant, \
+             self.set_amount(self.amount()) should always succeed.",
+        );
+        new_item.slots_mut().iter_mut().zip(self.slots()).for_each(
+            |(new_item_slot, old_item_slot)| {
+                *new_item_slot = old_item_slot
+                    .as_ref()
+                    .map(|old_item| old_item.duplicate(msm));
+            },
+        );
+        new_item
     }
 
     /// FIXME: HACK: In order to set the entity ID asynchronously, we currently
@@ -501,6 +534,7 @@ impl Item {
         let amount = u32::from(self.amount);
         self.amount = amount
             .checked_add(increase_by)
+            .filter(|&amount| amount <= self.max_amount())
             .and_then(NonZeroU32::new)
             .ok_or(OperationFailure)?;
         Ok(())
@@ -516,7 +550,7 @@ impl Item {
     }
 
     pub fn set_amount(&mut self, give_amount: u32) -> Result<(), OperationFailure> {
-        if give_amount == 1 || self.item_def.is_stackable() {
+        if give_amount <= self.max_amount() {
             self.amount = NonZeroU32::new(give_amount).ok_or(OperationFailure)?;
             Ok(())
         } else {
@@ -534,16 +568,14 @@ impl Item {
     }
 
     fn update_item_config(&mut self, msm: &MaterialStatManifest) {
-        self.item_config = if let ItemKind::Tool(_) = self.kind() {
-            Some(Box::new(ItemConfig::from((
-                self.kind(),
-                self.components(),
-                &self.item_def.ability_map,
-                msm,
-            ))))
-        } else {
-            None
-        };
+        if let Ok(item_config) = ItemConfig::try_from((
+            self.kind(),
+            self.components(),
+            &self.item_def.ability_map,
+            msm,
+        )) {
+            self.item_config = Some(Box::new(item_config));
+        }
     }
 
     /// Returns an iterator that drains items contained within the item's slots
@@ -572,6 +604,10 @@ impl Item {
 
     pub fn amount(&self) -> u32 { u32::from(self.amount) }
 
+    /// NOTE: invariant that amount() ≤ max_amount(), 1 ≤ max_amount(),
+    /// and if !self.is_stackable(), self.max_amount() = 1.
+    pub fn max_amount(&self) -> u32 { if self.is_stackable() { u32::MAX } else { 1 } }
+
     pub fn quality(&self) -> Quality { self.item_def.quality }
 
     pub fn components(&self) -> &[Item] { &self.components }

common/src/comp/inventory/loadout.rs

@@ -10,12 +10,13 @@ use serde::{Deserialize, Serialize};
 use std::ops::Range;
 use tracing::warn;
 
-#[derive(Clone, PartialEq, Debug, Serialize, Deserialize)]
+#[derive(Clone, Debug, Serialize, Deserialize)]
 pub struct Loadout {
     slots: Vec<LoadoutSlot>,
 }
 
-#[derive(Clone, PartialEq, Debug, Serialize, Deserialize)]
+/// NOTE: Please don't derive a PartialEq Instance for this; that's broken!
+#[derive(Clone, Debug, Serialize, Deserialize)]
 pub struct LoadoutSlot {
     /// The EquipSlot that this slot represents
     pub(super) equip_slot: EquipSlot,

common/src/comp/inventory/loadout_builder.rs

@@ -599,7 +599,10 @@ impl LoadoutBuilder {
                     let mut item_with_amount = |item_id: &str, amount: &mut f32| {
                         if *amount > 0.0 {
                             let mut item = Item::new_from_asset_expect(&item_id);
-                            let n = rng.gen_range(1..(amount.min(100.0) as u32).max(2));
+                            // NOTE: Conversion to and from f32 works fine because we make sure the
+                            // number we're converting is ≤ 100.
+                            let max = amount.min(100.min(item.max_amount()) as f32) as u32;
+                            let n = rng.gen_range(1..max.max(2));
                             *amount -= if item.set_amount(n).is_ok() {
                                 n as f32
                             } else {

common/src/comp/inventory/mod.rs

@@ -31,7 +31,8 @@ pub mod trade_pricing;
 pub type InvSlot = Option<Item>;
 const DEFAULT_INVENTORY_SLOTS: usize = 18;
 
-#[derive(Clone, Debug, PartialEq, Serialize, Deserialize)]
+/// NOTE: Do not add a PartialEq instance for Inventory; that's broken!
+#[derive(Clone, Debug, Serialize, Deserialize)]
 pub struct Inventory {
     loadout: Loadout,
     /// The "built-in" slots belonging to the inventory itself, all other slots
@@ -99,24 +100,61 @@ impl Inventory {
     }
 
     /// Adds a new item to the first fitting group of the inventory or starts a
-    /// new group. Returns the item again if no space was found.
-    pub fn push(&mut self, item: Item) -> Option<Item> {
-        if item.is_stackable() {
-            if let Some(slot_item) = self
-                .slots_mut()
-                .filter_map(Option::as_mut)
-                .find(|s| *s == &item)
-            {
-                return slot_item
-                    .increase_amount(item.amount())
-                    .err()
-                    .and(Some(item));
-            }
-        }
+    /// new group. Returns the item in an error if no space was found, otherwise
+    /// returns the found slot.
+    pub fn push(&mut self, item: Item) -> Result<(), Item> {
+        // First, check to make sure there's enough room for all instances of the
+        // item (note that if we find any empty slots, we can guarantee this by
+        // just filling up the whole slot, but to be nice we won't use it if we
+        // can find enough space in any combination of existing slots, and
+        // that's what we check in the `is_stackable` case).
 
-        // No existing item to stack with or item not stackable, put the item in a new
-        // slot
-        self.insert(item)
+        if item.is_stackable()
+            && self
+                .slots()
+                .filter_map(Option::as_ref)
+                .filter(|s| *s == &item)
+                .try_fold(item.amount(), |remaining, current| {
+                    remaining
+                        .checked_sub(current.max_amount() - current.amount())
+                        .filter(|&remaining| remaining > 0)
+                })
+                .is_none()
+        {
+            // We either exactly matched or had more than enough space for inserting the
+            // item into existing slots, so go do that!
+            assert!(
+                self.slots_mut()
+                    .filter_map(Option::as_mut)
+                    .filter(|s| *s == &item)
+                    .try_fold(item.amount(), |remaining, current| {
+                        // NOTE: Invariant that current.amount <= current.max_amount(), so the
+                        // subtraction is safe.
+                        let new_remaining = remaining
+                            .checked_sub(current.max_amount() - current.amount())
+                            .filter(|&remaining| remaining > 0);
+                        if new_remaining.is_some() {
+                            // Not enough capacity left to hold all the remaining items, so we fill
+                            // it as much as we can.
+                            current
+                                .set_amount(current.max_amount())
+                                .expect("max_amount() is always a valid amount.");
+                        } else {
+                            // Enough capacity to hold all the remaining items.
+                            current
+                                .increase_amount(remaining)
+                                .expect("Already checked that there is enough room.");
+                        }
+                        new_remaining
+                    })
+                    .is_none()
+            );
+            Ok(())
+        } else {
+            // No existing item to stack with or item not stackable, put the item in a new
+            // slot
+            self.insert(item)
+        }
     }
 
     /// Add a series of items to inventory, returning any which do not fit as an
@@ -125,7 +163,7 @@ impl Inventory {
         // Vec doesn't allocate for zero elements so this should be cheap
         let mut leftovers = Vec::new();
         for item in items {
-            if let Some(item) = self.push(item) {
+            if let Err(item) = self.push(item) {
                 leftovers.push(item);
             }
         }
@@ -149,7 +187,9 @@ impl Inventory {
         let mut leftovers = Vec::new();
         for item in &mut items {
             if self.contains(&item).not() {
-                self.push(item).map(|overflow| leftovers.push(overflow));
+                if let Err(overflow) = self.push(item) {
+                    leftovers.push(overflow);
+                }
             } // else drop item if it was already in
         }
         if !leftovers.is_empty() {
@@ -181,14 +221,17 @@ impl Inventory {
             }
         }
         if let Some(amount) = amount {
-            self.remove(src);
             let dstitem = self
                 .get_mut(dst)
                 .expect("self.get(dst) was Some right above this");
             dstitem
                 .increase_amount(amount)
-                .expect("already checked is_stackable");
-            true
+                .map(|_| {
+                    // Suceeded in adding the item, so remove it from `src`.
+                    self.remove(src).expect("Already verified that src was populated.");
+                })
+                // Can fail if we exceed `max_amount`
+                .is_ok()
         } else {
             false
         }
@@ -318,9 +361,11 @@ impl Inventory {
                 let mut return_item = item.duplicate(msm);
                 let returning_amount = item.amount() / 2;
                 item.decrease_amount(returning_amount).ok()?;
-                return_item
-                    .set_amount(returning_amount)
-                    .expect("Items duplicated from a stackable item must be stackable.");
+                return_item.set_amount(returning_amount).expect(
+                    "return_item.amount() = item.amount() / 2 < item.amount() (since \
+                     item.amount() ≥ 1) ≤ item.max_amount() = return_item.max_amount(), since \
+                     return_item is a duplicate of item",
+                );
                 Some(return_item)
             } else {
                 self.remove(inv_slot_id)
@@ -367,6 +412,8 @@ impl Inventory {
                     .filter(|item| item.matches_recipe_input(&*input))
                 {
                     let claim = slot_claims.entry(inv_slot_id).or_insert(0);
+                    // FIXME: Fishy, looks like it can underflow before min which can trigger an
+                    // overflow check.
                     let can_claim = (item.amount() - *claim).min(needed);
                     *claim += can_claim;
                     needed -= can_claim;
@@ -387,14 +434,15 @@ impl Inventory {
     }
 
     /// Adds a new item to the first empty slot of the inventory. Returns the
-    /// item again if no free slot was found.
-    fn insert(&mut self, item: Item) -> Option<Item> {
+    /// item again in an Err if no free slot was found, otherwise returns a
+    /// reference to the item.
+    fn insert(&mut self, item: Item) -> Result<(), Item> {
         match self.slots_mut().find(|slot| slot.is_none()) {
             Some(slot) => {
                 *slot = Some(item);
-                None
+                Ok(())
             },
-            None => Some(item),
+            None => Err(item),
         }
     }
 
@@ -495,7 +543,7 @@ impl Inventory {
     /// currently supported.
     pub fn pickup_item(&mut self, mut item: Item) -> Result<(), Item> {
         if item.is_stackable() {
-            return self.push(item).map_or(Ok(()), Err);
+            return self.push(item);
         }
 
         if self.free_slots() < item.populated_slots() + 1 {
@@ -506,11 +554,9 @@ impl Inventory {
         // itself into the inventory. We already know that there are enough free slots
         // so push will never give us an item back.
         item.drain().for_each(|item| {
-            self.push(item).unwrap_none();
+            self.push(item).unwrap();
         });
-        self.push(item).unwrap_none();
-
-        Ok(())
+        self.push(item)
     }
 
     /// Unequip an item from slot and place into inventory. Will leave the item
@@ -528,7 +574,7 @@ impl Inventory {
             .and_then(|mut unequipped_item| {
                 let unloaded_items: Vec<Item> = unequipped_item.drain().collect();
                 self.push(unequipped_item)
-                    .expect_none("Failed to push item to inventory, precondition failed?");
+                    .expect("Failed to push item to inventory, precondition failed?");
 
                 // Unload any items that were inside the equipped item into the inventory, with
                 // any that don't fit to be to be dropped on the floor by the caller
@@ -626,7 +672,7 @@ impl Inventory {
                 // inventory slot instead.
                 if let Err(returned) = self.insert_at(inv_slot_id, from_equip) {
                     self.push(returned)
-                        .expect_none("Unable to push to inventory, no slots (bug in can_swap()?)");
+                        .expect("Unable to push to inventory, no slots (bug in can_swap()?)");
                 }
 
                 items
@@ -686,7 +732,7 @@ impl Component for Inventory {
     type Storage = DerefFlaggedStorage<Self, IdvStorage<Self>>;
 }
 
-#[derive(Clone, Debug, PartialEq, Serialize, Deserialize)]
+#[derive(Clone, Debug, Serialize, Deserialize)]
 pub enum InventoryUpdateEvent {
     Init,
     Used,

common/src/comp/inventory/test.rs

@@ -13,38 +13,47 @@ lazy_static! {
 /// Attempting to push into a full inventory should return the same item.
 #[test]
 fn push_full() {
+    let msm = &MaterialStatManifest::default();
     let mut inv = Inventory {
-        slots: TEST_ITEMS.iter().map(|a| Some(a.clone())).collect(),
+        slots: TEST_ITEMS.iter().map(|a| Some(a.duplicate(msm))).collect(),
         loadout: LoadoutBuilder::new().build(),
     };
     assert_eq!(
-        inv.push(TEST_ITEMS[0].clone()).unwrap(),
-        TEST_ITEMS[0].clone()
+        inv.push(TEST_ITEMS[0].duplicate(msm)).unwrap_err(),
+        TEST_ITEMS[0].duplicate(msm)
     )
 }
 
 /// Attempting to push a series into a full inventory should return them all.
 #[test]
 fn push_all_full() {
+    let msm = &MaterialStatManifest::default();
     let mut inv = Inventory {
-        slots: TEST_ITEMS.iter().map(|a| Some(a.clone())).collect(),
+        slots: TEST_ITEMS.iter().map(|a| Some(a.duplicate(msm))).collect(),
         loadout: LoadoutBuilder::new().build(),
     };
     let Error::Full(leftovers) = inv
-        .push_all(TEST_ITEMS.iter().cloned())
+        .push_all(TEST_ITEMS.iter().map(|item| item.duplicate(msm)))
         .expect_err("Pushing into a full inventory somehow worked!");
-    assert_eq!(leftovers, TEST_ITEMS.clone())
+    assert_eq!(
+        leftovers,
+        TEST_ITEMS
+            .iter()
+            .map(|item| item.duplicate(msm))
+            .collect::<Vec<_>>()
+    )
 }
 
 /// Attempting to push uniquely into an inventory containing all the items
 /// should work fine.
 #[test]
 fn push_unique_all_full() {
+    let msm = &MaterialStatManifest::default();
     let mut inv = Inventory {
-        slots: TEST_ITEMS.iter().map(|a| Some(a.clone())).collect(),
+        slots: TEST_ITEMS.iter().map(|a| Some(a.duplicate(msm))).collect(),
         loadout: LoadoutBuilder::new().build(),
     };
-    inv.push_all_unique(TEST_ITEMS.iter().cloned())
+    inv.push_all_unique(TEST_ITEMS.iter().map(|item| item.duplicate(msm)))
         .expect("Pushing unique items into an inventory that already contains them didn't work!");
 }
 
@@ -52,11 +61,12 @@ fn push_unique_all_full() {
 /// should work fine.
 #[test]
 fn push_all_empty() {
+    let msm = &MaterialStatManifest::default();
     let mut inv = Inventory {
         slots: vec![None, None],
         loadout: LoadoutBuilder::new().build(),
     };
-    inv.push_all(TEST_ITEMS.iter().cloned())
+    inv.push_all(TEST_ITEMS.iter().map(|item| item.duplicate(msm)))
         .expect("Pushing items into an empty inventory didn't work!");
 }
 
@@ -64,22 +74,25 @@ fn push_all_empty() {
 /// should work fine.
 #[test]
 fn push_all_unique_empty() {
+    let msm = &MaterialStatManifest::default();
     let mut inv = Inventory {
         slots: vec![None, None],
         loadout: LoadoutBuilder::new().build(),
     };
-    inv.push_all_unique(TEST_ITEMS.iter().cloned()).expect(
-        "Pushing unique items into an empty inventory that didn't contain them didn't work!",
-    );
+    inv.push_all_unique(TEST_ITEMS.iter().map(|item| item.duplicate(msm)))
+        .expect(
+            "Pushing unique items into an empty inventory that didn't contain them didn't work!",
+        );
 }
 
 #[test]
 fn free_slots_minus_equipped_item_items_only_present_in_equipped_bag_slots() {
+    let msm = &MaterialStatManifest::default();
     let mut inv = Inventory::new_empty();
 
     let bag = get_test_bag(18);
     let bag1_slot = EquipSlot::Armor(ArmorSlot::Bag1);
-    inv.loadout.swap(bag1_slot, Some(bag.clone()));
+    inv.loadout.swap(bag1_slot, Some(bag.duplicate(msm)));
 
     inv.insert_at(InvSlotId::new(15, 0), bag)
         .unwrap()
@@ -94,13 +107,14 @@ fn free_slots_minus_equipped_item_items_only_present_in_equipped_bag_slots() {
 
 #[test]
 fn free_slots_minus_equipped_item() {
+    let msm = &MaterialStatManifest::default();
     let mut inv = Inventory::new_empty();
 
     let bag = get_test_bag(18);
     let bag1_slot = EquipSlot::Armor(ArmorSlot::Bag1);
-    inv.loadout.swap(bag1_slot, Some(bag.clone()));
+    inv.loadout.swap(bag1_slot, Some(bag.duplicate(msm)));
     inv.loadout
-        .swap(EquipSlot::Armor(ArmorSlot::Bag2), Some(bag.clone()));
+        .swap(EquipSlot::Armor(ArmorSlot::Bag2), Some(bag.duplicate(msm)));
 
     inv.insert_at(InvSlotId::new(16, 0), bag)
         .unwrap()
@@ -171,12 +185,13 @@ fn can_swap_equipped_bag_into_only_empty_slot_provided_by_itself_should_return_f
 
 #[test]
 fn unequip_items_both_hands() {
+    let msm = &MaterialStatManifest::default();
     let mut inv = Inventory::new_empty();
 
     let sword = Item::new_from_asset_expect("common.items.weapons.sword.steel-8");
 
-    inv.replace_loadout_item(EquipSlot::Mainhand, Some(sword.clone()));
-    inv.replace_loadout_item(EquipSlot::Offhand, Some(sword.clone()));
+    inv.replace_loadout_item(EquipSlot::Mainhand, Some(sword.duplicate(msm)));
+    inv.replace_loadout_item(EquipSlot::Offhand, Some(sword.duplicate(msm)));
 
     // Fill all inventory slots except one
     fill_inv_slots(&mut inv, 17);
@@ -200,6 +215,7 @@ fn unequip_items_both_hands() {
 
 #[test]
 fn equip_replace_already_equipped_item() {
+    let msm = &MaterialStatManifest::default();
     let boots = Item::new_from_asset_expect("common.items.testing.test_boots");
 
     let starting_sandles = Some(Item::new_from_asset_expect(
@@ -207,8 +223,11 @@ fn equip_replace_already_equipped_item() {
     ));
 
     let mut inv = Inventory::new_empty();
-    inv.push(boots.clone());
-    inv.replace_loadout_item(EquipSlot::Armor(ArmorSlot::Feet), starting_sandles.clone());
+    inv.push(boots.duplicate(msm)).unwrap();
+    inv.replace_loadout_item(
+        EquipSlot::Armor(ArmorSlot::Feet),
+        starting_sandles.as_ref().map(|item| item.duplicate(msm)),
+    );
 
     let _ = inv.equip(InvSlotId::new(0, 0));
 
@@ -219,10 +238,7 @@ fn equip_replace_already_equipped_item() {
     );
 
     // Verify inventory
-    assert_eq!(
-        inv.slots[0].as_ref().unwrap().item_definition_id(),
-        starting_sandles.unwrap().item_definition_id()
-    );
+    assert_eq!(&inv.slots[0], &starting_sandles,);
     assert_eq!(inv.populated_slots(), 1);
 }
 
@@ -274,14 +290,15 @@ fn unequip_unequipping_bag_into_its_own_slot_with_no_other_free_slots() {
 
 #[test]
 fn equip_one_bag_equipped_equip_second_bag() {
+    let msm = &MaterialStatManifest::default();
     let mut inv = Inventory::new_empty();
 
     let bag = get_test_bag(9);
     inv.loadout
-        .swap(EquipSlot::Armor(ArmorSlot::Bag1), Some(bag.clone()))
+        .swap(EquipSlot::Armor(ArmorSlot::Bag1), Some(bag.duplicate(msm)))
         .unwrap_none();
 
-    inv.push(bag);
+    inv.push(bag).unwrap();
 
     let _ = inv.equip(InvSlotId::new(0, 0));
 
@@ -298,7 +315,7 @@ fn free_after_swap_equipped_item_has_more_slots() {
         .unwrap_none();
 
     let small_bag = get_test_bag(9);
-    inv.push(small_bag);
+    inv.push(small_bag).unwrap();
 
     // Fill all remaining slots
     fill_inv_slots(&mut inv, 35);
@@ -319,10 +336,10 @@ fn free_after_swap_equipped_item_has_less_slots() {
         .unwrap_none();
 
     let small_bag = get_test_bag(18);
-    inv.push(small_bag);
+    inv.push(small_bag).unwrap();
 
     // Fill all slots except the last one
-    fill_inv_slots(&mut inv, 27);
+    fill_inv_slots(&mut inv, 26);
 
     let result = inv.free_after_swap(EquipSlot::Armor(ArmorSlot::Bag1), InvSlotId::new(0, 0));
 
@@ -352,7 +369,7 @@ fn free_after_swap_equipped_item_with_slots_swapped_with_empty_inv_slot() {
 fn free_after_swap_inv_item_with_slots_swapped_with_empty_equip_slot() {
     let mut inv = Inventory::new_empty();
 
-    inv.push(get_test_bag(9));
+    inv.push(get_test_bag(9)).unwrap();
 
     // Add 5 items to the inventory
     fill_inv_slots(&mut inv, 5);
@@ -368,7 +385,7 @@ fn free_after_swap_inv_item_without_slots_swapped_with_empty_equip_slot() {
     let mut inv = Inventory::new_empty();
 
     let boots = Item::new_from_asset_expect("common.items.testing.test_boots");
-    inv.push(boots);
+    inv.push(boots).unwrap();
 
     // Add 5 items to the inventory
     fill_inv_slots(&mut inv, 5);
@@ -380,8 +397,9 @@ fn free_after_swap_inv_item_without_slots_swapped_with_empty_equip_slot() {
 }
 
 fn fill_inv_slots(inv: &mut Inventory, items: u16) {
+    let msm = &MaterialStatManifest::default();
     let boots = Item::new_from_asset_expect("common.items.testing.test_boots");
     for _ in 0..items {
-        inv.push(boots.clone());
+        inv.push(boots.duplicate(msm)).unwrap();
     }
 }

common/src/comp/inventory/trade_pricing.rs

@@ -10,13 +10,9 @@ use lazy_static::lazy_static;
 use serde::Deserialize;
 use tracing::{info, warn};
 
-#[derive(Debug)]
-struct Entry {
-    probability: f32,
-    item: String,
-}
+type Entry = (String, f32);
 
-type Entries = Vec<(String, f32)>;
+type Entries = Vec<Entry>;
 const PRICING_DEBUG: bool = false;
 
 #[derive(Default, Debug)]
@@ -103,29 +99,29 @@ impl TradePricing {
 
     // add this much of a non-consumed crafting tool price
 
-    fn get_list(&self, good: Good) -> &Entries {
+    fn get_list(&self, good: Good) -> &[Entry] {
         match good {
             Good::Armor => &self.armor,
             Good::Tools => &self.tools,
             Good::Potions => &self.potions,
             Good::Food => &self.food,
             Good::Ingredients => &self.ingredients,
-            _ => panic!("invalid good"),
+            _ => &[],
         }
     }
 
-    fn get_list_mut(&mut self, good: Good) -> &mut Entries {
+    fn get_list_mut(&mut self, good: Good) -> &mut [Entry] {
         match good {
             Good::Armor => &mut self.armor,
             Good::Tools => &mut self.tools,
             Good::Potions => &mut self.potions,
             Good::Food => &mut self.food,
             Good::Ingredients => &mut self.ingredients,
-            _ => panic!("invalid good"),
+            _ => &mut [],
         }
     }
 
-    fn get_list_by_path(&self, name: &str) -> &Entries {
+    fn get_list_by_path(&self, name: &str) -> &[Entry] {
         match name {
             "common.items.crafting_ing.mindflayer_bag_damaged" => &self.armor,
             _ if name.starts_with("common.items.crafting_ing.") => &self.ingredients,
@@ -186,12 +182,15 @@ impl TradePricing {
                 entryvec.push((itemname.to_string(), probability));
             }
         }
-        fn sort_and_normalize(entryvec: &mut Entries, scale: f32) {
+        fn sort_and_normalize(entryvec: &mut [Entry], scale: f32) {
             if !entryvec.is_empty() {
                 entryvec.sort_by(|a, b| a.1.partial_cmp(&b.1).unwrap());
-                let rescale = scale / entryvec.last().unwrap().1;
-                for i in entryvec.iter_mut() {
-                    i.1 *= rescale;
+                if let Some((_, max_scale)) = entryvec.last() {
+                    // most common item has frequency max_scale.  avoid NaN
+                    let rescale = scale / max_scale;
+                    for i in entryvec.iter_mut() {
+                        i.1 *= rescale;
+                    }
                 }
             }
         }
@@ -228,16 +227,12 @@ impl TradePricing {
                 input: r
                     .inputs
                     .iter()
-                    .filter(|i| matches!(i.0, RecipeInput::Item(_)))
-                    .map(|i| {
-                        (
-                            if let RecipeInput::Item(it) = &i.0 {
-                                it.id().into()
-                            } else {
-                                panic!("recipe logic broken");
-                            },
-                            i.1,
-                        )
+                    .filter_map(|&(ref recipe_input, count)| {
+                        if let RecipeInput::Item(it) = recipe_input {
+                            Some((it.id().into(), count))
+                        } else {
+                            None
+                        }
                     })
                     .collect(),
             });
@@ -262,16 +257,14 @@ impl TradePricing {
         // re-look up prices and sort the vector by ascending material cost, return
         // whether first cost is finite
         fn price_sort(s: &TradePricing, vec: &mut Vec<RememberedRecipe>) -> bool {
-            if !vec.is_empty() {
-                for e in vec.iter_mut() {
-                    e.material_cost = calculate_material_cost(s, e);
-                }
-                vec.sort_by(|a, b| a.material_cost.partial_cmp(&b.material_cost).unwrap());
-                //info!(?vec);
-                vec.first().unwrap().material_cost < TradePricing::UNAVAILABLE_PRICE
-            } else {
-                false
+            for e in vec.iter_mut() {
+                e.material_cost = calculate_material_cost(s, e);
             }
+            vec.sort_by(|a, b| a.material_cost.partial_cmp(&b.material_cost).unwrap());
+            //info!(?vec);
+            vec.first()
+                .filter(|recipe| recipe.material_cost < TradePricing::UNAVAILABLE_PRICE)
+                .is_some()
         }
         // re-evaluate prices based on crafting tables
         // (start with cheap ones to avoid changing material prices after evaluation)

common/src/depot.rs

@@ -3,7 +3,6 @@ use std::{
     cmp::{Eq, Ord, Ordering, PartialEq, PartialOrd},
     fmt, hash,
     marker::PhantomData,
-    ops::{Index, IndexMut},
 };
 
 /// Type safe index into Depot
@@ -211,13 +210,3 @@ impl<T> Depot<T> {
         }
     }
 }
-
-impl<T> Index<Id<T>> for Depot<T> {
-    type Output = T;
-
-    fn index(&self, id: Id<T>) -> &Self::Output { self.get(id).unwrap() }
-}
-
-impl<T> IndexMut<Id<T>> for Depot<T> {
-    fn index_mut(&mut self, id: Id<T>) -> &mut Self::Output { self.get_mut(id).unwrap() }
-}

common/src/recipe.rs

@@ -46,7 +46,7 @@ impl Recipe {
         for i in 0..self.output.1 {
             let crafted_item =
                 Item::new_from_item_def(Arc::clone(&self.output.0), &components, msm);
-            if let Some(item) = inv.push(crafted_item) {
+            if let Err(item) = inv.push(crafted_item) {
                 return Ok(Some((item, self.output.1 - i)));
             }
         }

common/src/region.rs

@@ -108,14 +108,9 @@ impl RegionMap {
         span!(_guard, "tick", "Region::tick");
         self.tick += 1;
         // Clear events within each region
-        for i in 0..self.regions.len() {
-            self.regions
-                .get_index_mut(i)
-                .map(|(_, v)| v)
-                .unwrap()
-                .events
-                .clear();
-        }
+        self.regions.values_mut().for_each(|region| {
+            region.events.clear();
+        });
 
         // Add any untracked entities
         for (pos, id) in (&pos, &entities, !&self.tracked_entities)
@@ -130,52 +125,55 @@ impl RegionMap {
 
         let mut regions_to_remove = Vec::new();
 
-        for i in 0..self.regions.len() {
-            for (maybe_pos, _maybe_vel, id) in (
-                pos.maybe(),
-                vel.maybe(),
-                &self.regions.get_index(i).map(|(_, v)| v).unwrap().bitset,
-            )
-                .join()
-            {
-                match maybe_pos {
-                    // Switch regions for entities which need switching
-                    // TODO don't check every tick (use velocity) (and use id to stagger)
-                    // Starting parameters at v = 0 check every 100 ticks
-                    // tether_length^2 / vel^2  (with a max of every tick)
-                    Some(pos) => {
-                        let pos = pos.0.map(|e| e as i32);
-                        let current_region = self.index_key(i).unwrap();
-                        let key = Self::pos_key(pos);
-                        // Consider switching
-                        // Calculate distance outside border
-                        if key != current_region
-                            && (Vec2::<i32>::from(pos) - Self::key_pos(current_region))
-                                .map(|e| e.abs() as u32)
-                                .reduce_max()
-                                > TETHER_LENGTH
-                        {
-                            // Switch
-                            self.entities_to_move.push((i, id, pos));
-                        }
-                    },
-                    // Remove any non-existant entities (or just ones that lost their position
-                    // component) TODO: distribute this between ticks
-                    None => {
-                        // TODO: shouldn't there be a way to extract the bitset of entities with
-                        // positions directly from specs?
-                        self.entities_to_remove.push((i, id));
-                    },
+        let RegionMap {
+            entities_to_move,
+            entities_to_remove,
+            regions,
+            ..
+        } = self;
+        regions
+            .iter()
+            .enumerate()
+            .for_each(|(i, (&current_region, region_data))| {
+                for (maybe_pos, _maybe_vel, id) in
+                    (pos.maybe(), vel.maybe(), &region_data.bitset).join()
+                {
+                    match maybe_pos {
+                        // Switch regions for entities which need switching
+                        // TODO don't check every tick (use velocity) (and use id to stagger)
+                        // Starting parameters at v = 0 check every 100 ticks
+                        // tether_length^2 / vel^2  (with a max of every tick)
+                        Some(pos) => {
+                            let pos = pos.0.map(|e| e as i32);
+                            let key = Self::pos_key(pos);
+                            // Consider switching
+                            // Calculate distance outside border
+                            if key != current_region
+                                && (Vec2::<i32>::from(pos) - Self::key_pos(current_region))
+                                    .map(|e| e.abs() as u32)
+                                    .reduce_max()
+                                    > TETHER_LENGTH
+                            {
+                                // Switch
+                                entities_to_move.push((i, id, pos));
+                            }
+                        },
+                        // Remove any non-existant entities (or just ones that lost their position
+                        // component) TODO: distribute this between ticks
+                        None => {
+                            // TODO: shouldn't there be a way to extract the bitset of entities with
+                            // positions directly from specs?
+                            entities_to_remove.push((i, id));
+                        },
+                    }
                 }
-            }
 
-            // Remove region if it is empty
-            // TODO: distribute this between ticks
-            let (key, region) = self.regions.get_index(i).unwrap();
-            if region.removable() {
-                regions_to_remove.push(*key);
-            }
-        }
+                // Remove region if it is empty
+                // TODO: distribute this between ticks
+                if region_data.removable() {
+                    regions_to_remove.push(current_region);
+                }
+            });
 
         // Mutate
         // Note entity moving is outside the whole loop so that the same entity is not
@@ -268,10 +266,6 @@ impl RegionMap {
         self.regions.get_full(&key).map(|(i, _, _)| i)
     }
 
-    fn index_key(&self, index: usize) -> Option<Vec2<i32>> {
-        self.regions.get_index(index).map(|(k, _)| k).copied()
-    }
-
     /// Adds a new region
     /// Returns the index of the region in the index map
     fn insert(&mut self, key: Vec2<i32>) -> usize {

common/sys/src/character_behavior.rs

@@ -45,13 +45,16 @@ fn incorporate_update(join: &mut JoinStruct, mut state_update: StateUpdate) {
     if state_update.swap_equipped_weapons {
         let mut inventory = join.inventory.get_mut_unchecked();
         let inventory = &mut *inventory;
-        inventory
-            .swap(
-                Slot::Equip(EquipSlot::Mainhand),
-                Slot::Equip(EquipSlot::Offhand),
-            )
-            .first()
-            .unwrap_none(); // Swapping main and offhand never results in leftover items
+        assert!(
+            inventory
+                .swap(
+                    Slot::Equip(EquipSlot::Mainhand),
+                    Slot::Equip(EquipSlot::Offhand),
+                )
+                .first()
+                .is_none(),
+            "Swapping main and offhand never results in leftover items",
+        );
     }
 }
 

common/sys/src/state.rs

@@ -20,14 +20,16 @@ use common::{
 use common_base::span;
 use common_ecs::{run_now, PhysicsMetrics, SysMetrics};
 use common_net::sync::{interpolation as sync_interp, WorldSyncExt};
-use hashbrown::{HashMap, HashSet};
+use core::{convert::identity, time::Duration};
+use hashbrown::{hash_map, HashMap, HashSet};
 use rayon::{ThreadPool, ThreadPoolBuilder};
 use specs::{
+    prelude::Resource,
     shred::{Fetch, FetchMut},
     storage::{MaskedStorage as EcsMaskedStorage, Storage as EcsStorage},
     Component, DispatcherBuilder, Entity as EcsEntity, WorldExt,
 };
-use std::{sync::Arc, time::Duration};
+use std::sync::Arc;
 use vek::*;
 
 /// How much faster should an in-game day be compared to a real day?
@@ -41,18 +43,54 @@ const DAY_CYCLE_FACTOR: f64 = 24.0 * 2.0;
 /// avoid such a situation.
 const MAX_DELTA_TIME: f32 = 1.0;
 
+/// NOTE: Please don't add `Deserialize` without checking to make sure we
+/// can guarantee the invariant that every entry in `area_names` points to a
+/// valid id in `areas`.
 #[derive(Default)]
 pub struct BuildAreas {
-    pub areas: Depot<geom::Aabb<i32>>,
-    pub area_names: HashMap<String, Id<Aabb<i32>>>,
+    areas: Depot<Aabb<i32>>,
+    area_names: HashMap<String, Id<Aabb<i32>>>,
 }
 
+pub enum BuildAreaError {
+    /// This build area name is reserved by the system.
+    Reserved,
+    /// The build area name was not found.
+    NotFound,
+}
+
+/// Build area names that can only be inserted, not removed.
+const RESERVED_BUILD_AREA_NAMES: &[&str] = &["world"];
+
 impl BuildAreas {
-    pub fn new() -> Self {
-        Self {
-            areas: Depot::default(),
-            area_names: HashMap::new(),
+    pub fn areas(&self) -> &Depot<geom::Aabb<i32>> { &self.areas }
+
+    pub fn area_names(&self) -> &HashMap<String, Id<Aabb<i32>>> { &self.area_names }
+
+    /// If the area_name is already in the map, returns Err(area_name).
+    pub fn insert(&mut self, area_name: String, area: Aabb<i32>) -> Result<Id<Aabb<i32>>, String> {
+        let area_name_entry = match self.area_names.entry(area_name) {
+            hash_map::Entry::Occupied(o) => return Err(o.replace_key()),
+            hash_map::Entry::Vacant(v) => v,
+        };
+        let bb_id = self.areas.insert(area.made_valid());
+        area_name_entry.insert(bb_id);
+        Ok(bb_id)
+    }
+
+    pub fn remove(&mut self, area_name: &str) -> Result<Aabb<i32>, BuildAreaError> {
+        if RESERVED_BUILD_AREA_NAMES.contains(&area_name) {
+            return Err(BuildAreaError::Reserved);
         }
+        let bb_id = self
+            .area_names
+            .remove(area_name)
+            .ok_or(BuildAreaError::NotFound)?;
+        let area = self.areas.remove(bb_id).expect(
+            "Entries in `areas` are added before entries in `area_names` in `insert`, and that is \
+             the only exposed way to add elements to `area_names`.",
+        );
+        Ok(area)
     }
 }
 
@@ -221,7 +259,7 @@ impl State {
         ecs.insert(PlayerEntity(None));
         ecs.insert(TerrainGrid::new().unwrap());
         ecs.insert(BlockChange::default());
-        ecs.insert(BuildAreas::new());
+        ecs.insert(BuildAreas::default());
         ecs.insert(TerrainChanges::default());
         ecs.insert(EventBus::<LocalEvent>::default());
         ecs.insert(game_mode);
@@ -286,9 +324,24 @@ impl State {
         self
     }
 
-    /// Write a component attributed to a particular entity.
-    pub fn write_component<C: Component>(&mut self, entity: EcsEntity, comp: C) {
-        let _ = self.ecs.write_storage().insert(entity, comp);
+    /// Write a component attributed to a particular entity, ignoring errors.
+    ///
+    /// This should be used *only* when we can guarantee that the rest of the
+    /// code does not rely on the insert having succeeded (meaning the
+    /// entity is no longer alive!).
+    ///
+    /// Returns None if the entity was dead or there was no previous entry for
+    /// this component; otherwise, returns Some(old_component).
+    pub fn write_component_ignore_entity_dead<C: Component>(
+        &mut self,
+        entity: EcsEntity,
+        comp: C,
+    ) -> Option<C> {
+        self.ecs
+            .write_storage()
+            .insert(entity, comp)
+            .ok()
+            .and_then(identity)
     }
 
     /// Delete a component attributed to a particular entity.
@@ -306,6 +359,17 @@ impl State {
         self.ecs.read_storage().get(entity).copied()
     }
 
+    /// Given mutable access to the resource R, assuming the resource
+    /// component exists (this is already the behavior of functions like `fetch`
+    /// and `write_component_ignore_entity_dead`).  Since all of our resources
+    /// are generated up front, any failure here is definitely a code bug.
+    pub fn mut_resource<R: Resource>(&mut self) -> &mut R {
+        self.ecs.get_mut::<R>().expect(
+            "Tried to fetch an invalid resource even though all our resources should be known at \
+             compile time.",
+        )
+    }
+
     /// Get a read-only reference to the storage of a particular component type.
     pub fn read_storage<C: Component>(&self) -> EcsStorage<C, Fetch<EcsMaskedStorage<C>>> {
         self.ecs.read_storage::<C>()
@@ -355,16 +419,16 @@ impl State {
     }
 
     /// Set a block in this state's terrain.
-    pub fn set_block(&mut self, pos: Vec3<i32>, block: Block) {
+    pub fn set_block(&self, pos: Vec3<i32>, block: Block) {
         self.ecs.write_resource::<BlockChange>().set(pos, block);
     }
 
     /// Check if the block at given position `pos` has already been modified
     /// this tick.
-    pub fn can_set_block(&mut self, pos: Vec3<i32>) -> bool {
+    pub fn can_set_block(&self, pos: Vec3<i32>) -> bool {
         !self
             .ecs
-            .write_resource::<BlockChange>()
+            .read_resource::<BlockChange>()
             .blocks
             .contains_key(&pos)
     }

server/src/character_creator.rs

@@ -41,10 +41,14 @@ pub fn create_character(
     let mut inventory = Inventory::new_with_loadout(loadout);
 
     // Default items for new characters
-    inventory.push(Item::new_from_asset_expect(
-        "common.items.consumable.potion_minor",
-    ));
-    inventory.push(Item::new_from_asset_expect("common.items.food.cheese"));
+    inventory
+        .push(Item::new_from_asset_expect(
+            "common.items.consumable.potion_minor",
+        ))
+        .expect("Inventory has at least 2 slots left!");
+    inventory
+        .push(Item::new_from_asset_expect("common.items.food.cheese"))
+        .expect("Inventory has at least 1 slot left!");
 
     let waypoint = None;
 

server/src/connection_handler.rs

@@ -142,9 +142,16 @@ impl ConnectionHandler {
 
 impl Drop for ConnectionHandler {
     fn drop(&mut self) {
-        let _ = self.stop_sender.take().unwrap().send(());
+        let _ = self
+            .stop_sender
+            .take()
+            .expect("`stop_sender` is private, initialized as `Some`, and only updated in Drop")
+            .send(());
         trace!("aborting ConnectionHandler");
-        self.thread_handle.take().unwrap().abort();
+        self.thread_handle
+            .take()
+            .expect("`thread_handle` is private, initialized as `Some`, and only updated in Drop")
+            .abort();
         trace!("aborted ConnectionHandler!");
     }
 }

server/src/events/information.rs

@@ -35,15 +35,13 @@ pub fn handle_site_info(server: &Server, entity: EcsEntity, id: u64) {
                 .economy
                 .labor_values
                 .iter()
-                .filter(|a| a.1.is_some())
-                .map(|(g, a)| (g, a.unwrap()))
+                .filter_map(|(g, a)| a.map(|a| (g, a)))
                 .collect(),
             values: site
                 .economy
                 .values
                 .iter()
-                .filter(|a| a.1.is_some())
-                .map(|(g, a)| (g, a.unwrap()))
+                .filter_map(|(g, a)| a.map(|a| (g, a)))
                 .collect(),
             labors: site.economy.labors.iter().map(|(_, a)| (*a)).collect(),
             last_exports: site

server/src/events/interaction.rs

@@ -77,6 +77,7 @@ pub fn handle_npc_interaction(server: &mut Server, interactor: EcsEntity, npc_en
     }
 }
 
+/// FIXME: Make mounting more robust, avoid bidirectional links.
 pub fn handle_mount(server: &mut Server, mounter: EcsEntity, mountee: EcsEntity) {
     let state = server.state_mut();
 
@@ -101,8 +102,14 @@ pub fn handle_mount(server: &mut Server, mounter: EcsEntity, mountee: EcsEntity)
                 state.ecs().uid_from_entity(mounter),
                 state.ecs().uid_from_entity(mountee),
             ) {
-                state.write_component(mountee, comp::MountState::MountedBy(mounter_uid));
-                state.write_component(mounter, comp::Mounting(mountee_uid));
+                // We know the entities must exist to be able to look up their UIDs, so these
+                // are guaranteed to work; hence we can ignore possible errors
+                // here.
+                state.write_component_ignore_entity_dead(
+                    mountee,
+                    comp::MountState::MountedBy(mounter_uid),
+                );
+                state.write_component_ignore_entity_dead(mounter, comp::Mounting(mountee_uid));
             }
         }
     }
@@ -126,8 +133,11 @@ pub fn handle_unmount(server: &mut Server, mounter: EcsEntity) {
 }
 
 #[allow(clippy::nonminimal_bool)] // TODO: Pending review in #587
-pub fn handle_possess(server: &Server, possessor_uid: Uid, possesse_uid: Uid) {
-    let ecs = &server.state.ecs();
+/// FIXME: This code is dangerous and needs to be refactored.  We can't just
+/// comment it out, but it needs to be fixed for a variety of reasons.  Get rid
+/// of this ASAP!
+pub fn handle_possess(server: &mut Server, possessor_uid: Uid, possesse_uid: Uid) {
+    let ecs = server.state.ecs();
     if let (Some(possessor), Some(possesse)) = (
         ecs.entity_from_uid(possessor_uid.into()),
         ecs.entity_from_uid(possesse_uid.into()),
@@ -231,18 +241,21 @@ pub fn handle_possess(server: &Server, possessor_uid: Uid, possesse_uid: Uid) {
         let mut inventories = ecs.write_storage::<Inventory>();
         let mut inventory = inventories
             .entry(possesse)
-            .expect("Could not read inventory component while possessing")
+            .expect("Nobody has &mut World, so there's no way to delete an entity.")
             .or_insert(Inventory::new_empty());
 
         let debug_item = comp::Item::new_from_asset_expect("common.items.debug.admin_stick");
         if let item::ItemKind::Tool(_) = debug_item.kind() {
-            inventory
-                .swap(
-                    Slot::Equip(EquipSlot::Mainhand),
-                    Slot::Equip(EquipSlot::Offhand),
-                )
-                .first()
-                .unwrap_none(); // Swapping main and offhand never results in leftover items
+            assert!(
+                inventory
+                    .swap(
+                        Slot::Equip(EquipSlot::Mainhand),
+                        Slot::Equip(EquipSlot::Offhand),
+                    )
+                    .first()
+                    .is_none(),
+                "Swapping main and offhand never results in leftover items",
+            );
 
             inventory.replace_loadout_item(EquipSlot::Mainhand, Some(debug_item));
         }

server/src/events/inventory_manip.rs

@@ -80,78 +80,101 @@ pub fn handle_inventory(server: &mut Server, entity: EcsEntity, manip: comp::Inv
         })
     };
 
+    let mut inventories = state.ecs().write_storage::<comp::Inventory>();
+    let mut inventory = if let Some(inventory) = inventories.get_mut(entity) {
+        inventory
+    } else {
+        error!(
+            ?entity,
+            "Can't manipulate inventory, entity doesn't have one"
+        );
+        return;
+    };
+
     match manip {
         comp::InventoryManip::Pickup(uid) => {
-            let picked_up_item: Option<comp::Item>;
-            let item_entity = if let (Some((item, item_entity)), Some(mut inv)) = (
-                state
-                    .ecs()
-                    .entity_from_uid(uid.into())
-                    .and_then(|item_entity| {
-                        state
-                            .ecs()
-                            .write_storage::<comp::Item>()
-                            .get_mut(item_entity)
-                            .map(|item| (item.clone(), item_entity))
-                    }),
-                state
-                    .ecs()
-                    .write_storage::<comp::Inventory>()
-                    .get_mut(entity),
-            ) {
-                picked_up_item = Some(item.clone());
-
-                let entity_cylinder = get_cylinder(state, entity);
-                if !within_pickup_range(entity_cylinder, || get_cylinder(state, item_entity)) {
-                    debug!(
-                        ?entity_cylinder,
-                        "Failed to pick up item as not within range, Uid: {}", uid
-                    );
-                    return;
-                };
-
-                // Grab the health from the entity and check if the entity is dead.
-                let healths = state.ecs().read_storage::<comp::Health>();
-                if let Some(entity_health) = healths.get(entity) {
-                    if entity_health.is_dead {
-                        debug!("Failed to pick up item as the entity is dead");
-                        return; // If dead, don't continue
-                    }
-                }
-
-                // First try to equip the picked up item
-                if let Err(returned_item) = inv.try_equip(item) {
-                    // If we couldn't equip it (no empty slot for it or unequippable) then attempt
-                    // to add the item to the entity's inventory
-                    match inv.pickup_item(returned_item) {
-                        Ok(_) => Some(item_entity),
-                        Err(_) => None, // Inventory was full
-                    }
-                } else {
-                    Some(item_entity)
-                }
+            let item_entity = if let Some(item_entity) = state.ecs().entity_from_uid(uid.into()) {
+                item_entity
             } else {
-                // Item entity/component could not be found - most likely because the entity
+                // Item entity could not be found - most likely because the entity
                 // attempted to pick up the same item very quickly before its deletion of the
                 // world from the first pickup attempt was processed.
-                debug!("Failed to get entity/component for item Uid: {}", uid);
+                debug!("Failed to get entity for item Uid: {}", uid);
+                return;
+            };
+            let entity_cylinder = get_cylinder(state, entity);
+
+            // FIXME: Raycast so we can't pick up items through walls.
+            if !within_pickup_range(entity_cylinder, || get_cylinder(state, item_entity)) {
+                debug!(
+                    ?entity_cylinder,
+                    "Failed to pick up item as not within range, Uid: {}", uid
+                );
+                return;
+            }
+
+            // Grab the health from the entity and check if the entity is dead.
+            let healths = state.ecs().read_storage::<comp::Health>();
+            if let Some(entity_health) = healths.get(entity) {
+                if entity_health.is_dead {
+                    debug!("Failed to pick up item as the entity is dead");
+                    return; // If dead, don't continue
+                }
+            }
+            drop(healths);
+
+            // First, we remove the item, assuming picking it up will succeed (we do this to
+            // avoid cloning the item, as we should not call Item::clone and it
+            // may be removed!).
+            let mut item_storage = state.ecs().write_storage::<comp::Item>();
+            let item = if let Some(item) = item_storage.remove(item_entity) {
+                item
+            } else {
+                // Item component could not be found - most likely because the entity
+                // attempted to pick up the same item very quickly before its deletion of the
+                // world from the first pickup attempt was processed.
+                debug!("Failed to delete item component for entity, Uid: {}", uid);
                 return;
             };
 
-            let event = if let Some(item_entity) = item_entity {
-                if let Err(err) = state.delete_entity_recorded(item_entity) {
-                    // If this occurs it means the item was duped as it's been pushed to the
-                    // entity's inventory but also left on the ground
-                    panic!("Failed to delete picked up item entity: {:?}", err);
-                }
-                comp::InventoryUpdate::new(comp::InventoryUpdateEvent::Collected(
-                    picked_up_item.unwrap(),
-                ))
-            } else {
-                comp::InventoryUpdate::new(comp::InventoryUpdateEvent::CollectFailed)
+            // NOTE: We dup the item for message purposes.
+            let item_msg = item.duplicate(&state.ecs().read_resource::<MaterialStatManifest>());
+
+            // Next, we try to equip the picked up item
+            let event = match inventory.try_equip(item).or_else(|returned_item| {
+                // If we couldn't equip it (no empty slot for it or unequippable) then attempt
+                // to add the item to the entity's inventory
+                inventory.pickup_item(returned_item)
+            }) {
+                Err(returned_item) => {
+                    // Inventory was full, so we need to put back the item (note that we know there
+                    // was no old item component for this entity).
+                    item_storage.insert(entity, returned_item).expect(
+                        "We know item_entity exists since we just successfully removed its Item \
+                         component.",
+                    );
+                    drop(item_storage);
+                    drop(inventories);
+                    comp::InventoryUpdate::new(comp::InventoryUpdateEvent::CollectFailed)
+                },
+                Ok(_) => {
+                    // We succeeded in picking up the item, so we may now delete its old entity
+                    // entirely.
+                    drop(item_storage);
+                    drop(inventories);
+                    state.delete_entity_recorded(item_entity).expect(
+                        "We knew item_entity existed since we just successfully removed its Item \
+                         component.",
+                    );
+                    comp::InventoryUpdate::new(comp::InventoryUpdateEvent::Collected(item_msg))
+                },
             };
 
-            state.write_component(entity, event);
+            state
+                .ecs()
+                .write_storage()
+                .insert(entity, event)
+                .expect("We know entity exists since we got its inventory.");
         },
         comp::InventoryManip::Collect(pos) => {
             let block = state.terrain().get(pos).ok().copied();
@@ -174,34 +197,31 @@ pub fn handle_inventory(server: &mut Server, entity: EcsEntity, manip: comp::Inv
                     };
 
                     if let Some(item) = comp::Item::try_reclaim_from_block(block) {
-                        let (event, item_was_added) = if let Some(mut inv) = state
-                            .ecs()
-                            .write_storage::<comp::Inventory>()
-                            .get_mut(entity)
-                        {
-                            match inv.push(item.clone()) {
-                                None => (
-                                    Some(comp::InventoryUpdate::new(
-                                        comp::InventoryUpdateEvent::Collected(item),
-                                    )),
-                                    true,
-                                ),
-                                Some(_) => (
-                                    Some(comp::InventoryUpdate::new(
-                                        comp::InventoryUpdateEvent::CollectFailed,
-                                    )),
-                                    false,
-                                ),
-                            }
-                        } else {
-                            debug!(
-                                "Can't add item to inventory: entity has no inventory ({:?})",
-                                entity
-                            );
-                            (None, false)
+                        // NOTE: We dup the item for message purposes.
+                        let item_msg =
+                            item.duplicate(&state.ecs().read_resource::<MaterialStatManifest>());
+                        let (event, item_was_added) = match inventory.push(item) {
+                            Ok(_) => (
+                                Some(comp::InventoryUpdate::new(
+                                    comp::InventoryUpdateEvent::Collected(item_msg),
+                                )),
+                                true,
+                            ),
+                            // The item we created was in some sense "fake" so it's safe to
+                            // drop it.
+                            Err(_) => (
+                                Some(comp::InventoryUpdate::new(
+                                    comp::InventoryUpdateEvent::CollectFailed,
+                                )),
+                                false,
+                            ),
                         };
                         if let Some(event) = event {
-                            state.write_component(entity, event);
+                            state
+                                .ecs()
+                                .write_storage()
+                                .insert(entity, event)
+                                .expect("We know entity exists since we got its inventory.");
                             if item_was_added {
                                 // we made sure earlier the block was not already modified this tick
                                 state.set_block(pos, block.into_vacant())
@@ -222,19 +242,9 @@ pub fn handle_inventory(server: &mut Server, entity: EcsEntity, manip: comp::Inv
                     );
                 }
             }
+            drop(inventories);
         },
         comp::InventoryManip::Use(slot) => {
-            let mut inventories = state.ecs().write_storage::<comp::Inventory>();
-            let mut inventory = if let Some(inventory) = inventories.get_mut(entity) {
-                inventory
-            } else {
-                error!(
-                    ?entity,
-                    "Can't manipulate inventory, entity doesn't have one"
-                );
-                return;
-            };
-
             let mut maybe_effect = None;
 
             let event = match slot {
@@ -380,7 +390,9 @@ pub fn handle_inventory(server: &mut Server, entity: EcsEntity, manip: comp::Inv
                                 Some(comp::InventoryUpdateEvent::Used)
                             },
                             _ => {
-                                inventory.insert_or_stack_at(slot, item).unwrap();
+                                inventory.insert_or_stack_at(slot, item).expect(
+                                    "slot was just vacated of item, so it definitely fits there.",
+                                );
                                 None
                             },
                         }
@@ -420,55 +432,51 @@ pub fn handle_inventory(server: &mut Server, entity: EcsEntity, manip: comp::Inv
                 }
             }
             if let Some(event) = event {
-                state.write_component(entity, comp::InventoryUpdate::new(event));
+                state
+                    .ecs()
+                    .write_storage()
+                    .insert(entity, comp::InventoryUpdate::new(event))
+                    .expect("We know entity exists since we got its inventory.");
             }
         },
         comp::InventoryManip::Swap(a, b) => {
             let ecs = state.ecs();
 
             if let Some(pos) = ecs.read_storage::<comp::Pos>().get(entity) {
-                if let Some(mut inventory) = ecs.write_storage::<comp::Inventory>().get_mut(entity)
-                {
-                    let mut merged_stacks = false;
+                let mut merged_stacks = false;
 
-                    // If both slots have items and we're attemping to drag from one stack
-                    // into another, stack the items.
-                    if let (Slot::Inventory(slot_a), Slot::Inventory(slot_b)) = (a, b) {
-                        merged_stacks |= inventory.merge_stack_into(slot_a, slot_b);
-                    }
+                // If both slots have items and we're attemping to drag from one stack
+                // into another, stack the items.
+                if let (Slot::Inventory(slot_a), Slot::Inventory(slot_b)) = (a, b) {
+                    merged_stacks |= inventory.merge_stack_into(slot_a, slot_b);
+                }
 
-                    // If the stacks weren't mergable carry out a swap.
-                    if !merged_stacks {
-                        dropped_items.extend(inventory.swap(a, b).into_iter().map(|x| {
-                            (
-                                *pos,
-                                state
-                                    .read_component_copied::<comp::Ori>(entity)
-                                    .unwrap_or_default(),
-                                x,
-                            )
-                        }));
-                    }
+                // If the stacks weren't mergable carry out a swap.
+                if !merged_stacks {
+                    dropped_items.extend(inventory.swap(a, b).into_iter().map(|x| {
+                        (
+                            *pos,
+                            state
+                                .read_component_copied::<comp::Ori>(entity)
+                                .unwrap_or_default(),
+                            x,
+                        )
+                    }));
                 }
             }
+            drop(inventories);
 
-            state.write_component(
-                entity,
-                comp::InventoryUpdate::new(comp::InventoryUpdateEvent::Swapped),
-            );
+            state
+                .ecs()
+                .write_storage()
+                .insert(
+                    entity,
+                    comp::InventoryUpdate::new(comp::InventoryUpdateEvent::Swapped),
+                )
+                .expect("We know entity exists since we got its inventory.");
         },
         comp::InventoryManip::SplitSwap(slot, target) => {
             let msm = state.ecs().read_resource::<MaterialStatManifest>();
-            let mut inventories = state.ecs().write_storage::<comp::Inventory>();
-            let mut inventory = if let Some(inventory) = inventories.get_mut(entity) {
-                inventory
-            } else {
-                error!(
-                    ?entity,
-                    "Can't manipulate inventory, entity doesn't have one"
-                );
-                return;
-            };
 
             // If both slots have items and we're attemping to split from one stack
             // into another, ensure that they are the same type of item. If they are
@@ -496,27 +504,22 @@ pub fn handle_inventory(server: &mut Server, entity: EcsEntity, manip: comp::Inv
                     inventory.insert_or_stack_at(target, item).ok();
                 }
             }
-            drop(inventory);
-            drop(inventories);
             drop(msm);
 
-            state.write_component(
-                entity,
-                comp::InventoryUpdate::new(comp::InventoryUpdateEvent::Swapped),
-            );
+            state
+                .ecs()
+                .write_storage()
+                .insert(
+                    entity,
+                    comp::InventoryUpdate::new(comp::InventoryUpdateEvent::Swapped),
+                )
+                .expect("We know entity exists since we got its inventory.");
+            drop(inventories);
         },
         comp::InventoryManip::Drop(slot) => {
             let item = match slot {
-                Slot::Inventory(slot) => state
-                    .ecs()
-                    .write_storage::<comp::Inventory>()
-                    .get_mut(entity)
-                    .and_then(|mut inv| inv.remove(slot)),
-                Slot::Equip(slot) => state
-                    .ecs()
-                    .write_storage::<comp::Inventory>()
-                    .get_mut(entity)
-                    .and_then(|mut inv| inv.replace_loadout_item(slot, None)),
+                Slot::Inventory(slot) => inventory.remove(slot),
+                Slot::Equip(slot) => inventory.replace_loadout_item(slot, None),
             };
 
             // FIXME: We should really require the drop and write to be atomic!
@@ -532,19 +535,20 @@ pub fn handle_inventory(server: &mut Server, entity: EcsEntity, manip: comp::Inv
                     item,
                 ));
             }
-            state.write_component(
-                entity,
-                comp::InventoryUpdate::new(comp::InventoryUpdateEvent::Dropped),
-            );
+            state
+                .ecs()
+                .write_storage()
+                .insert(
+                    entity,
+                    comp::InventoryUpdate::new(comp::InventoryUpdateEvent::Dropped),
+                )
+                .expect("We know entity exists since we got its inventory.");
+            drop(inventories);
         },
         comp::InventoryManip::SplitDrop(slot) => {
             let msm = state.ecs().read_resource::<MaterialStatManifest>();
             let item = match slot {
-                Slot::Inventory(slot) => state
-                    .ecs()
-                    .write_storage::<comp::Inventory>()
-                    .get_mut(entity)
-                    .and_then(|mut inv| inv.take_half(slot, &msm)),
+                Slot::Inventory(slot) => inventory.take_half(slot, &msm),
                 Slot::Equip(_) => None,
             };
 
@@ -562,47 +566,48 @@ pub fn handle_inventory(server: &mut Server, entity: EcsEntity, manip: comp::Inv
                 ));
             }
             drop(msm);
-            state.write_component(
-                entity,
-                comp::InventoryUpdate::new(comp::InventoryUpdateEvent::Dropped),
-            );
+            state
+                .ecs()
+                .write_storage()
+                .insert(
+                    entity,
+                    comp::InventoryUpdate::new(comp::InventoryUpdateEvent::Dropped),
+                )
+                .expect("We know entity exists since we got its inventory.");
+            drop(inventories);
         },
         comp::InventoryManip::CraftRecipe(recipe) => {
-            if let Some(mut inv) = state
-                .ecs()
-                .write_storage::<comp::Inventory>()
-                .get_mut(entity)
-            {
-                let recipe_book = default_recipe_book().read();
-                let craft_result = recipe_book.get(&recipe).and_then(|r| {
-                    r.perform(
-                        &mut inv,
-                        &state.ecs().read_resource::<item::MaterialStatManifest>(),
-                    )
-                    .ok()
-                });
+            let recipe_book = default_recipe_book().read();
+            let craft_result = recipe_book.get(&recipe).and_then(|r| {
+                r.perform(
+                    &mut inventory,
+                    &state.ecs().read_resource::<item::MaterialStatManifest>(),
+                )
+                .ok()
+            });
+            drop(inventories);
 
-                // FIXME: We should really require the drop and write to be atomic!
-                if craft_result.is_some() {
-                    let _ = state.ecs().write_storage().insert(
-                        entity,
-                        comp::InventoryUpdate::new(comp::InventoryUpdateEvent::Craft),
-                    );
-                }
+            // FIXME: We should really require the drop and write to be atomic!
+            if craft_result.is_some() {
+                let _ = state.ecs().write_storage().insert(
+                    entity,
+                    comp::InventoryUpdate::new(comp::InventoryUpdateEvent::Craft),
+                );
+            }
 
-                // Drop the item if there wasn't enough space
-                if let Some(Some((item, amount))) = craft_result {
-                    for _ in 0..amount {
-                        dropped_items.push((
-                            state
-                                .read_component_copied::<comp::Pos>(entity)
-                                .unwrap_or_default(),
-                            state
-                                .read_component_copied::<comp::Ori>(entity)
-                                .unwrap_or_default(),
-                            item.clone(),
-                        ));
-                    }
+            // Drop the item if there wasn't enough space
+            if let Some(Some((item, amount))) = craft_result {
+                let msm = state.ecs().read_resource::<MaterialStatManifest>();
+                for _ in 0..amount {
+                    dropped_items.push((
+                        state
+                            .read_component_copied::<comp::Pos>(entity)
+                            .unwrap_or_default(),
+                        state
+                            .read_component_copied::<comp::Ori>(entity)
+                            .unwrap_or_default(),
+                        item.duplicate(&msm),
+                    ));
                 }
             }
         },

server/src/events/mod.rs

@@ -124,7 +124,7 @@ impl Server {
                 ServerEvent::Mount(mounter, mountee) => handle_mount(self, mounter, mountee),
                 ServerEvent::Unmount(mounter) => handle_unmount(self, mounter),
                 ServerEvent::Possess(possessor_uid, possesse_uid) => {
-                    handle_possess(&self, possessor_uid, possesse_uid)
+                    handle_possess(self, possessor_uid, possesse_uid)
                 },
                 ServerEvent::InitCharacterData {
                     entity,

server/src/events/player.rs

@@ -101,28 +101,37 @@ pub fn handle_client_disconnect(server: &mut Server, entity: EcsEntity) -> Event
         .write_storage::<Client>()
         .get_mut(entity)
     {
-        let participant = client.participant.take().unwrap();
-        let pid = participant.remote_pid();
-        server.runtime.spawn(
-            async {
-                let now = std::time::Instant::now();
-                debug!("Start handle disconnect of client");
-                if let Err(e) = participant.disconnect().await {
-                    debug!(
-                        ?e,
-                        "Error when disconnecting client, maybe the pipe already broke"
-                    );
-                };
-                trace!("finished disconnect");
-                let elapsed = now.elapsed();
-                if elapsed.as_millis() > 100 {
-                    warn!(?elapsed, "disconnecting took quite long");
-                } else {
-                    debug!(?elapsed, "disconnecting took");
+        // NOTE: There are not and likely willl not be a way to safeguard against
+        // receiving multiple `ServerEvent::ClientDisconnect` messages in a tick
+        // intended for the same player, so the `None` case here is *not* a bug
+        // and we should not log it as a potential issue.
+        if let Some(participant) = client.participant.take() {
+            let pid = participant.remote_pid();
+            server.runtime.spawn(
+                async {
+                    let now = std::time::Instant::now();
+                    debug!("Start handle disconnect of client");
+                    if let Err(e) = participant.disconnect().await {
+                        debug!(
+                            ?e,
+                            "Error when disconnecting client, maybe the pipe already broke"
+                        );
+                    };
+                    trace!("finished disconnect");
+                    let elapsed = now.elapsed();
+                    if elapsed.as_millis() > 100 {
+                        warn!(?elapsed, "disconnecting took quite long");
+                    } else {
+                        debug!(?elapsed, "disconnecting took");
+                    }
                 }
-            }
-            .instrument(tracing::debug_span!("client_disconnect", ?pid, ?entity)),
-        );
+                .instrument(tracing::debug_span!(
+                    "client_disconnect",
+                    ?pid,
+                    ?entity
+                )),
+            );
+        }
     }
 
     let state = server.state_mut();

server/src/events/trade.rs

@@ -166,7 +166,7 @@ fn commit_trade(ecs: &specs::World, trade: &PendingTrade) -> TradeResult {
             None => return TradeResult::Declined,
         }
     }
-    let mut inventories = ecs.write_component::<Inventory>();
+    let mut inventories = ecs.write_storage::<Inventory>();
     for entity in entities.iter() {
         if inventories.get_mut(*entity).is_none() {
             return TradeResult::Declined;

server/src/lib.rs

@@ -86,7 +86,6 @@ use persistence::{
     character_loader::{CharacterLoader, CharacterLoaderResponseKind},
     character_updater::CharacterUpdater,
 };
-use plugin_api::Uid;
 use prometheus::Registry;
 use prometheus_hyper::Server as PrometheusServer;
 use specs::{join::Join, Builder, Entity as EcsEntity, SystemData, WorldExt};
@@ -337,10 +336,9 @@ impl Server {
                 max: Vec3::new(world_size.x, world_size.y, 32767),
             }
             .made_valid();
-            let world_aabb_id = build_areas.areas.insert(world_aabb);
             build_areas
-                .area_names
-                .insert("world".to_string(), world_aabb_id);
+                .insert("world".to_string(), world_aabb)
+                .expect("The initial insert should always work.");
         }
 
         // Insert the world into the ECS (todo: Maybe not an Arc?)
@@ -748,11 +746,18 @@ impl Server {
             .ecs()
             .read_storage::<Client>()
             .get(entity)
-            .unwrap()
+            .expect(
+                "We just created this entity with a Client component using build(), and we have \
+                 &mut access to the ecs so it can't have been deleted yet.",
+            )
             .send(ServerInit::GameSync {
                 // Send client their entity
                 entity_package: TrackedComps::fetch(&self.state.ecs())
-                    .create_entity_package(entity, None, None, None),
+                    .create_entity_package(entity, None, None, None)
+                    .expect(
+                        "We just created this entity as marked() (using create_entity_synced) so \
+                         it definitely has a uid",
+                    ),
                 time_of_day: *self.state.ecs().read_resource(),
                 max_group_size: self.settings().max_player_group_size,
                 client_timeout: self.settings().client_timeout,
@@ -840,22 +845,24 @@ impl Server {
                     uid_allocator: &self.state.ecs().read_resource::<UidAllocator>().into(),
                     player: self.state.ecs().read_component().into(),
                 };
+                let uid = if let Some(uid) = ecs_world.uid.get(entity).copied() {
+                    uid
+                } else {
+                    self.notify_client(
+                        entity,
+                        ServerGeneral::server_msg(
+                            comp::ChatType::CommandError,
+                            "Can't get player UUID (player may be disconnected?)",
+                        ),
+                    );
+                    return;
+                };
                 let rs = plugin_manager.execute_event(
                     &ecs_world,
                     &plugin_api::event::ChatCommandEvent {
                         command: kwd.clone(),
                         command_args: args.split(' ').map(|x| x.to_owned()).collect(),
-                        player: plugin_api::event::Player {
-                            id: plugin_api::Uid(
-                                (self
-                                    .state
-                                    .ecs()
-                                    .read_storage::<Uid>()
-                                    .get(entity)
-                                    .expect("Can't get player UUID [This should never appen]"))
-                                .0,
-                            ),
-                        },
+                        player: plugin_api::event::Player { id: uid },
                     },
                 );
                 match rs {
@@ -976,8 +983,7 @@ impl Server {
                 .map(|(e, _)| e)
         }) {
             // Remove admin component if the player is ingame
-            let _ = self
-                .state
+            self.state
                 .ecs()
                 .write_storage::<comp::Admin>()
                 .remove(entity);

server/src/login_provider.rs

@@ -21,7 +21,7 @@ fn derive_uuid(username: &str) -> Uuid {
         state = state.wrapping_mul(309485009821345068724781371);
     }
 
-    Uuid::from_slice(&state.to_be_bytes()).unwrap()
+    Uuid::from_u128(state)
 }
 
 /// derive Uuid for "singleplayer" is a pub fn
@@ -107,26 +107,35 @@ impl LoginProvider {
         match pending.pending_r.try_recv() {
             Ok(Err(e)) => Some(Err(e)),
             Ok(Ok((username, uuid))) => {
-                if let Some(ban_record) = banlist.get(&uuid) {
-                    // Pull reason string out of ban record and send a copy of it
-                    return Some(Err(RegisterError::Banned(ban_record.reason.clone())));
+                // Hardcoded admins can always log in.
+                let is_admin = admins.contains(&uuid);
+                if !is_admin {
+                    if let Some(ban_record) = banlist.get(&uuid) {
+                        // Pull reason string out of ban record and send a copy of it
+                        return Some(Err(RegisterError::Banned(ban_record.reason.clone())));
+                    }
+
+                    // non-admins can only join if the whitelist is empty (everyone can join)
+                    // or his name is in the whitelist
+                    if !whitelist.is_empty() && !whitelist.contains(&uuid) {
+                        return Some(Err(RegisterError::NotOnWhitelist));
+                    }
                 }
 
-                // user can only join if he is admin, the whitelist is empty (everyone can join)
-                // or his name is in the whitelist
-                if !whitelist.is_empty() && !whitelist.contains(&uuid) && !admins.contains(&uuid) {
-                    return Some(Err(RegisterError::NotOnWhitelist));
-                }
                 #[cfg(feature = "plugins")]
                 {
+                    // Plugin player join hooks execute for all players, but are only allowed to
+                    // filter non-admins.
                     match plugin_manager.execute_event(&world, &PlayerJoinEvent {
                         player_name: username.clone(),
                         player_id: *uuid.as_bytes(),
                     }) {
                         Ok(e) => {
-                            for i in e.into_iter() {
-                                if let PlayerJoinResult::Kick(a) = i {
-                                    return Some(Err(RegisterError::Kicked(a)));
+                            if !is_admin {
+                                for i in e.into_iter() {
+                                    if let PlayerJoinResult::Kick(a) = i {
+                                        return Some(Err(RegisterError::Kicked(a)));
+                                    }
                                 }
                             }
                         },

server/src/persistence/character/conversions.rs

@@ -253,6 +253,9 @@ pub fn convert_inventory_from_database_items(
 
         // Stack Size
         if db_item.stack_size == 1 || item.is_stackable() {
+            // FIXME: On failure, collect the set of items that don't fit and return them
+            // (to be dropped next to the player) as this could be the result of
+            // a change in the max amount for that item.
             item.set_amount(u32::try_from(db_item.stack_size).map_err(|_| {
                 Error::ConversionError(format!(
                     "Invalid item stack size for stackable={}: {}",
@@ -280,6 +283,11 @@ pub fn convert_inventory_from_database_items(
             let insert_res = inventory.insert_at(slot, item).map_err(|_| {
                 // If this happens there were too many items in the database for the current
                 // inventory size
+                //
+                // FIXME: On failure, collect the set of items that don't fit and return them
+                // (to be dropped next to the player) as this could be the
+                // result of a change in the slot capacity for an equipped bag
+                // (or a change in the inventory size).
                 Error::ConversionError(format!(
                     "Error inserting item into inventory, position: {:?}",
                     slot

server/src/state_ext.rs

@@ -361,37 +361,40 @@ impl StateExt for State {
     fn initialize_character_data(&mut self, entity: EcsEntity, character_id: CharacterId) {
         let spawn_point = self.ecs().read_resource::<SpawnPoint>().0;
 
-        self.write_component(entity, comp::Controller::default());
-        self.write_component(entity, comp::Pos(spawn_point));
-        self.write_component(entity, comp::Vel(Vec3::zero()));
-        self.write_component(entity, comp::Ori::default());
-        self.write_component(entity, comp::Collider::Box {
-            radius: 0.4,
-            z_min: 0.0,
-            z_max: 1.75,
-        });
-        self.write_component(entity, comp::Gravity(1.0));
-        self.write_component(entity, comp::CharacterState::default());
-        self.write_component(
-            entity,
-            comp::Alignment::Owned(self.read_component_copied(entity).unwrap()),
-        );
-        self.write_component(entity, comp::Buffs::default());
-        self.write_component(entity, comp::Auras::default());
-        self.write_component(entity, comp::Combo::default());
+        if let Some(player_uid) = self.read_component_copied::<Uid>(entity) {
+            // NOTE: By fetching the player_uid, we validated that the entity exists, and we
+            // call nothing that can delete it in any of the subsequent
+            // commands, so we can assume that all of these calls succeed,
+            // justifying ignoring the result of insertion.
+            self.write_component_ignore_entity_dead(entity, comp::Controller::default());
+            self.write_component_ignore_entity_dead(entity, comp::Pos(spawn_point));
+            self.write_component_ignore_entity_dead(entity, comp::Vel(Vec3::zero()));
+            self.write_component_ignore_entity_dead(entity, comp::Ori::default());
+            self.write_component_ignore_entity_dead(entity, comp::Collider::Box {
+                radius: 0.4,
+                z_min: 0.0,
+                z_max: 1.75,
+            });
+            self.write_component_ignore_entity_dead(entity, comp::Gravity(1.0));
+            self.write_component_ignore_entity_dead(entity, comp::CharacterState::default());
+            self.write_component_ignore_entity_dead(entity, comp::Alignment::Owned(player_uid));
+            self.write_component_ignore_entity_dead(entity, comp::Buffs::default());
+            self.write_component_ignore_entity_dead(entity, comp::Auras::default());
+            self.write_component_ignore_entity_dead(entity, comp::Combo::default());
 
-        // Make sure physics components are updated
-        self.write_component(entity, comp::ForceUpdate);
+            // Make sure physics components are updated
+            self.write_component_ignore_entity_dead(entity, comp::ForceUpdate);
 
-        const INITIAL_VD: u32 = 5; //will be changed after login
-        self.write_component(
-            entity,
-            Presence::new(INITIAL_VD, PresenceKind::Character(character_id)),
-        );
+            const INITIAL_VD: u32 = 5; //will be changed after login
+            self.write_component_ignore_entity_dead(
+                entity,
+                Presence::new(INITIAL_VD, PresenceKind::Character(character_id)),
+            );
 
-        // Tell the client its request was successful.
-        if let Some(client) = self.ecs().read_storage::<Client>().get(entity) {
-            client.send_fallible(ServerGeneral::CharacterSuccess);
+            // Tell the client its request was successful.
+            if let Some(client) = self.ecs().read_storage::<Client>().get(entity) {
+                client.send_fallible(ServerGeneral::CharacterSuccess);
+            }
         }
     }
 
@@ -406,12 +409,16 @@ impl StateExt for State {
                 }),
             ));
 
-            self.write_component(entity, comp::Collider::Box {
+            // NOTE: By fetching the player_uid, we validated that the entity exists, and we
+            // call nothing that can delete it in any of the subsequent
+            // commands, so we can assume that all of these calls succeed,
+            // justifying ignoring the result of insertion.
+            self.write_component_ignore_entity_dead(entity, comp::Collider::Box {
                 radius: body.radius(),
                 z_min: 0.0,
                 z_max: body.height(),
             });
-            self.write_component(entity, body);
+            self.write_component_ignore_entity_dead(entity, body);
             let (health_level, energy_level) = (
                 stats
                     .skill_set
@@ -424,21 +431,21 @@ impl StateExt for State {
                     .unwrap_or(None)
                     .unwrap_or(0),
             );
-            self.write_component(entity, comp::Health::new(body, health_level));
-            self.write_component(entity, comp::Energy::new(body, energy_level));
-            self.write_component(entity, comp::Poise::new(body));
-            self.write_component(entity, stats);
-            self.write_component(entity, inventory);
-            self.write_component(
+            self.write_component_ignore_entity_dead(entity, comp::Health::new(body, health_level));
+            self.write_component_ignore_entity_dead(entity, comp::Energy::new(body, energy_level));
+            self.write_component_ignore_entity_dead(entity, comp::Poise::new(body));
+            self.write_component_ignore_entity_dead(entity, stats);
+            self.write_component_ignore_entity_dead(entity, inventory);
+            self.write_component_ignore_entity_dead(
                 entity,
                 comp::InventoryUpdate::new(comp::InventoryUpdateEvent::default()),
             );
 
             if let Some(waypoint) = waypoint {
-                self.write_component(entity, waypoint);
-                self.write_component(entity, comp::Pos(waypoint.get_pos()));
-                self.write_component(entity, comp::Vel(Vec3::zero()));
-                self.write_component(entity, comp::ForceUpdate);
+                self.write_component_ignore_entity_dead(entity, waypoint);
+                self.write_component_ignore_entity_dead(entity, comp::Pos(waypoint.get_pos()));
+                self.write_component_ignore_entity_dead(entity, comp::Vel(Vec3::zero()));
+                self.write_component_ignore_entity_dead(entity, comp::ForceUpdate);
             }
         }
     }
@@ -585,8 +592,8 @@ impl StateExt for State {
         )
             .join()
         {
-            if lazy_msg.is_none() {
-                lazy_msg = Some(client.prepare(msg.take().unwrap()));
+            if let Some(msg) = msg.take() {
+                lazy_msg = Some(client.prepare(msg));
             }
             lazy_msg.as_ref().map(|ref msg| client.send_prepared(&msg));
         }
@@ -602,8 +609,8 @@ impl StateExt for State {
         )
             .join()
         {
-            if lazy_msg.is_none() {
-                lazy_msg = Some(client.prepare(msg.take().unwrap()));
+            if let Some(msg) = msg.take() {
+                lazy_msg = Some(client.prepare(msg));
             }
             lazy_msg.as_ref().map(|ref msg| client.send_prepared(&msg));
         }

server/src/sys/entity_sync.rs

@@ -15,6 +15,7 @@ use common::{
 };
 use common_ecs::{Job, Origin, Phase, System};
 use common_net::{msg::ServerGeneral, sync::CompSyncPackage};
+use itertools::Either;
 use specs::{Entities, Join, Read, ReadExpect, ReadStorage, Write, WriteStorage};
 use vek::*;
 
@@ -127,18 +128,19 @@ impl<'a> System<'a> for Sys {
                             continue;
                         }
                         let entity = entities.entity(*id);
-                        if let Some((_uid, pos, vel, ori)) = uids.get(entity).and_then(|uid| {
-                            positions.get(entity).map(|pos| {
-                                (uid, pos, velocities.get(entity), orientations.get(entity))
-                            })
-                        }) {
-                            let create_msg =
-                                ServerGeneral::CreateEntity(tracked_comps.create_entity_package(
+                        if let Some(pkg) = positions
+                            .get(entity)
+                            .map(|pos| (pos, velocities.get(entity), orientations.get(entity)))
+                            .and_then(|(pos, vel, ori)| {
+                                tracked_comps.create_entity_package(
                                     entity,
                                     Some(*pos),
                                     vel.copied(),
                                     ori.copied(),
-                                ));
+                                )
+                            })
+                        {
+                            let create_msg = ServerGeneral::CreateEntity(pkg);
                             for (client, regions, client_entity, _) in &mut subscribers {
                                 if maybe_key
                                     .as_ref()
@@ -178,38 +180,35 @@ impl<'a> System<'a> for Sys {
                     .take_deleted_in_region(key)
                     .unwrap_or_default(),
             );
-            let mut entity_sync_package = Some(entity_sync_package);
-            let mut comp_sync_package = Some(comp_sync_package);
-            let mut entity_sync_lazymsg = None;
-            let mut comp_sync_lazymsg = None;
-            subscribers.iter_mut().for_each(move |(client, _, _, _)| {
-                if entity_sync_lazymsg.is_none() {
-                    entity_sync_lazymsg = Some(client.prepare(ServerGeneral::EntitySync(
-                        entity_sync_package.take().unwrap(),
-                    )));
-                    comp_sync_lazymsg = Some(
-                        client.prepare(ServerGeneral::CompSync(comp_sync_package.take().unwrap())),
-                    );
-                }
-                entity_sync_lazymsg
-                    .as_ref()
-                    .map(|msg| client.send_prepared(&msg));
-                comp_sync_lazymsg
-                    .as_ref()
-                    .map(|msg| client.send_prepared(&msg));
-            });
+            // We lazily initializethe the synchronization messages in case there are no
+            // clients.
+            let mut entity_comp_sync = Either::Left((entity_sync_package, comp_sync_package));
+            for (client, _, _, _) in &mut subscribers {
+                let msg =
+                    entity_comp_sync.right_or_else(|(entity_sync_package, comp_sync_package)| {
+                        (
+                            client.prepare(ServerGeneral::EntitySync(entity_sync_package)),
+                            client.prepare(ServerGeneral::CompSync(comp_sync_package)),
+                        )
+                    });
+                // We don't care much about stream errors here since they could just represent
+                // network disconnection, which is handled elsewhere.
+                let _ = client.send_prepared(&msg.0);
+                let _ = client.send_prepared(&msg.1);
+                entity_comp_sync = Either::Right(msg);
+            }
 
             for (client, _, client_entity, client_pos) in &mut subscribers {
                 let mut comp_sync_package = CompSyncPackage::new();
 
-                for (_, entity, &uid, &pos, vel, ori, force_update, collider) in (
+                for (_, entity, &uid, (&pos, last_pos), vel, ori, force_update, collider) in (
                     region.entities(),
                     &entities,
                     &uids,
-                    &positions,
-                    velocities.maybe(),
-                    orientations.maybe(),
-                    force_updates.maybe(),
+                    (&positions, last_pos.mask().maybe()),
+                    (&velocities, last_vel.mask().maybe()).maybe(),
+                    (&orientations, last_vel.mask().maybe()).maybe(),
+                    force_updates.mask().maybe(),
                     colliders.maybe(),
                 )
                     .join()
@@ -246,45 +245,48 @@ impl<'a> System<'a> for Sys {
                         }
                     };
 
-                    if last_pos.get(entity).is_none() {
+                    if last_pos.is_none() {
                         comp_sync_package.comp_inserted(uid, pos);
                     } else if send_now {
                         comp_sync_package.comp_modified(uid, pos);
                     }
 
-                    vel.map(|v| {
-                        if last_vel.get(entity).is_none() {
+                    if let Some((v, last_vel)) = vel {
+                        if last_vel.is_none() {
                             comp_sync_package.comp_inserted(uid, *v);
                         } else if send_now {
                             comp_sync_package.comp_modified(uid, *v);
                         }
-                    });
+                    }
 
-                    ori.map(|o| {
-                        if last_ori.get(entity).is_none() {
+                    if let Some((o, last_ori)) = ori {
+                        if last_ori.is_none() {
                             comp_sync_package.comp_inserted(uid, *o);
                         } else if send_now {
                             comp_sync_package.comp_modified(uid, *o);
                         }
-                    });
+                    }
                 }
 
                 client.send_fallible(ServerGeneral::CompSync(comp_sync_package));
             }
 
             // Update the last physics components for each entity
-            for (_, entity, &pos, vel, ori) in (
+            for (_, _, &pos, vel, ori, last_pos, last_vel, last_ori) in (
                 region.entities(),
                 &entities,
                 &positions,
                 velocities.maybe(),
                 orientations.maybe(),
+                last_pos.entries(),
+                last_vel.entries(),
+                last_ori.entries(),
             )
                 .join()
             {
-                let _ = last_pos.insert(entity, Last(pos));
-                vel.map(|v| last_vel.insert(entity, Last(*v)));
-                ori.map(|o| last_ori.insert(entity, Last(*o)));
+                last_pos.replace(Last(pos));
+                vel.and_then(|&v| last_vel.replace(Last(v)));
+                ori.and_then(|&o| last_ori.replace(Last(o)));
             }
         }
 
@@ -347,10 +349,12 @@ impl<'a> System<'a> for Sys {
         // system?)
         let mut tof_lazymsg = None;
         for client in (&clients).join() {
-            if tof_lazymsg.is_none() {
-                tof_lazymsg = Some(client.prepare(ServerGeneral::TimeOfDay(*time_of_day)));
-            }
-            tof_lazymsg.as_ref().map(|msg| client.send_prepared(&msg));
+            let msg = tof_lazymsg
+                .unwrap_or_else(|| client.prepare(ServerGeneral::TimeOfDay(*time_of_day)));
+            // We don't care much about stream errors here since they could just represent
+            // network disconnection, which is handled elsewhere.
+            let _ = client.send_prepared(&msg);
+            tof_lazymsg = Some(msg);
         }
     }
 }

server/src/sys/msg/character_screen.rs

@@ -17,7 +17,6 @@ impl Sys {
     #[allow(clippy::too_many_arguments)]
     fn handle_client_character_screen_msg(
         server_emitter: &mut common::event::Emitter<'_, ServerEvent>,
-        new_chat_msgs: &mut Vec<(Option<specs::Entity>, UnresolvedChatMsg)>,
         entity: specs::Entity,
         client: &Client,
         character_loader: &ReadExpect<'_, CharacterLoader>,
@@ -68,7 +67,7 @@ impl Sys {
 
                         if !client.login_msg_sent.load(Ordering::Relaxed) {
                             if let Some(player_uid) = uids.get(entity) {
-                                new_chat_msgs.push((None, UnresolvedChatMsg {
+                                server_emitter.emit(ServerEvent::Chat(UnresolvedChatMsg {
                                     chat_type: ChatType::Online(*player_uid),
                                     message: "".to_string(),
                                 }));
@@ -155,13 +154,11 @@ impl<'a> System<'a> for Sys {
         ): Self::SystemData,
     ) {
         let mut server_emitter = server_event_bus.emitter();
-        let mut new_chat_msgs = Vec::new();
 
         for (entity, client) in (&entities, &clients).join() {
             let _ = super::try_recv_all(client, 1, |client, msg| {
                 Self::handle_client_character_screen_msg(
                     &mut server_emitter,
-                    &mut new_chat_msgs,
                     entity,
                     client,
                     &character_loader,
@@ -174,19 +171,5 @@ impl<'a> System<'a> for Sys {
                 )
             });
         }
-
-        // Handle new chat messages.
-        for (entity, msg) in new_chat_msgs {
-            // Handle chat commands.
-            if msg.message.starts_with('/') {
-                if let (Some(entity), true) = (entity, msg.message.len() > 1) {
-                    let argv = String::from(&msg.message[1..]);
-                    server_emitter.emit(ServerEvent::ChatCmd(entity, argv));
-                }
-            } else {
-                // Send chat message
-                server_emitter.emit(ServerEvent::Chat(msg));
-            }
-        }
     }
 }

server/src/sys/msg/general.rs

@@ -1,6 +1,6 @@
 use crate::{client::Client, metrics::PlayerMetrics};
 use common::{
-    comp::{ChatMode, Player, UnresolvedChatMsg},
+    comp::{ChatMode, Player},
     event::{EventBus, ServerEvent},
     resources::Time,
     uid::Uid,
@@ -18,7 +18,6 @@ impl Sys {
     #[allow(clippy::unnecessary_wraps)]
     fn handle_general_msg(
         server_emitter: &mut common::event::Emitter<'_, ServerEvent>,
-        new_chat_msgs: &mut Vec<(Option<specs::Entity>, UnresolvedChatMsg)>,
         entity: specs::Entity,
         client: &Client,
         player: Option<&Player>,
@@ -32,10 +31,17 @@ impl Sys {
                 if player.is_some() {
                     match validate_chat_msg(&message) {
                         Ok(()) => {
-                            if let Some(from) = uids.get(entity) {
-                                let mode = chat_modes.get(entity).cloned().unwrap_or_default();
-                                let msg = mode.new_message(*from, message);
-                                new_chat_msgs.push((Some(entity), msg));
+                            if let Some(message) = message.strip_prefix('/') {
+                                if !message.is_empty() {
+                                    let argv = String::from(message);
+                                    server_emitter.emit(ServerEvent::ChatCmd(entity, argv));
+                                }
+                            } else if let Some(from) = uids.get(entity) {
+                                const CHAT_MODE_DEFAULT: &ChatMode = &ChatMode::default();
+                                let mode = chat_modes.get(entity).unwrap_or(CHAT_MODE_DEFAULT);
+                                // Send chat message
+                                server_emitter
+                                    .emit(ServerEvent::Chat(mode.new_message(*from, message)));
                             } else {
                                 error!("Could not send message. Missing player uid");
                             }
@@ -49,7 +55,7 @@ impl Sys {
                 }
             },
             ClientGeneral::Terminate => {
-                debug!(?entity, "Client send message to termitate session");
+                debug!(?entity, "Client send message to terminate session");
                 player_metrics
                     .clients_disconnected
                     .with_label_values(&["gracefully"])
@@ -97,13 +103,11 @@ impl<'a> System<'a> for Sys {
         ): Self::SystemData,
     ) {
         let mut server_emitter = server_event_bus.emitter();
-        let mut new_chat_msgs = Vec::new();
 
         for (entity, client, player) in (&entities, &clients, (&players).maybe()).join() {
             let res = super::try_recv_all(client, 3, |client, msg| {
                 Self::handle_general_msg(
                     &mut server_emitter,
-                    &mut new_chat_msgs,
                     entity,
                     client,
                     player,
@@ -119,19 +123,5 @@ impl<'a> System<'a> for Sys {
                 *client.last_ping.lock().unwrap() = time.0
             }
         }
-
-        // Handle new chat messages.
-        for (entity, msg) in new_chat_msgs {
-            // Handle chat commands.
-            if msg.message.starts_with('/') {
-                if let (Some(entity), true) = (entity, msg.message.len() > 1) {
-                    let argv = String::from(&msg.message[1..]);
-                    server_emitter.emit(ServerEvent::ChatCmd(entity, argv));
-                }
-            } else {
-                // Send chat message
-                server_emitter.emit(ServerEvent::Chat(msg));
-            }
-        }
     }
 }

server/src/sys/msg/in_game.rs

@@ -107,7 +107,7 @@ impl Sys {
                     if comp_can_build.enabled {
                         for area in comp_can_build.build_areas.iter() {
                             if let Some(block) = build_areas
-                                .areas
+                                .areas()
                                 .get(*area)
                                 // TODO: Make this an exclusive check on the upper bound of the AABB
                                 // Vek defaults to inclusive which is not optimal
@@ -125,7 +125,7 @@ impl Sys {
                     if comp_can_build.enabled {
                         for area in comp_can_build.build_areas.iter() {
                             if build_areas
-                                .areas
+                                .areas()
                                 .get(*area)
                                 // TODO: Make this an exclusive check on the upper bound of the AABB
                                 // Vek defaults to inclusive which is not optimal

server/src/sys/sentinel.rs

@@ -1,8 +1,8 @@
 use common::{
     comp::{
-        Auras, BeamSegment, Body, Buffs, CanBuild, CharacterState, Collider, Combo, Energy,
-        Gravity, Group, Health, Inventory, Item, LightEmitter, Mass, MountState, Mounting, Ori,
-        Player, Poise, Pos, Scale, Shockwave, Stats, Sticky, Vel,
+        item::MaterialStatManifest, Auras, BeamSegment, Body, Buffs, CanBuild, CharacterState,
+        Collider, Combo, Energy, Gravity, Group, Health, Inventory, Item, LightEmitter, Mass,
+        MountState, Mounting, Ori, Player, Poise, Pos, Scale, Shockwave, Stats, Sticky, Vel,
     },
     uid::Uid,
 };
@@ -63,6 +63,7 @@ pub struct TrackedComps<'a> {
     pub character_state: ReadStorage<'a, CharacterState>,
     pub shockwave: ReadStorage<'a, Shockwave>,
     pub beam_segment: ReadStorage<'a, BeamSegment>,
+    pub msm: ReadExpect<'a, MaterialStatManifest>,
 }
 impl<'a> TrackedComps<'a> {
     pub fn create_entity_package(
@@ -71,13 +72,8 @@ impl<'a> TrackedComps<'a> {
         pos: Option<Pos>,
         vel: Option<Vel>,
         ori: Option<Ori>,
-    ) -> EntityPackage<EcsCompPacket> {
-        let uid = self
-            .uid
-            .get(entity)
-            .copied()
-            .expect("No uid to create an entity package")
-            .0;
+    ) -> Option<EntityPackage<EcsCompPacket>> {
+        let uid = self.uid.get(entity).copied()?.0;
         let mut comps = Vec::new();
         self.body.get(entity).copied().map(|c| comps.push(c.into()));
         self.player
@@ -120,7 +116,10 @@ impl<'a> TrackedComps<'a> {
             .get(entity)
             .copied()
             .map(|c| comps.push(c.into()));
-        self.item.get(entity).cloned().map(|c| comps.push(c.into()));
+        self.item
+            .get(entity)
+            .map(|item| item.duplicate(&self.msm))
+            .map(|c| comps.push(c.into()));
         self.scale
             .get(entity)
             .copied()
@@ -171,7 +170,7 @@ impl<'a> TrackedComps<'a> {
         vel.map(|c| comps.push(c.into()));
         ori.map(|c| comps.push(c.into()));
 
-        EntityPackage { uid, comps }
+        Some(EntityPackage { uid, comps })
     }
 }
 #[derive(SystemData)]

server/src/sys/subscription.rs

@@ -179,7 +179,7 @@ impl<'a> System<'a> for Sys {
                     // already within the set of subscribed regions
                     if subscription.regions.insert(key) {
                         if let Some(region) = region_map.get(key) {
-                            for (pos, vel, ori, _, entity) in (
+                            (
                                 &positions,
                                 velocities.maybe(),
                                 orientations.maybe(),
@@ -188,18 +188,19 @@ impl<'a> System<'a> for Sys {
                             )
                                 .join()
                                 .filter(|(_, _, _, _, e)| *e != client_entity)
-                            {
-                                // Send message to create entity and tracked components and physics
-                                // components
-                                client.send_fallible(ServerGeneral::CreateEntity(
+                                .filter_map(|(pos, vel, ori, _, entity)| {
                                     tracked_comps.create_entity_package(
                                         entity,
                                         Some(*pos),
                                         vel.copied(),
                                         ori.copied(),
-                                    ),
-                                ));
-                            }
+                                    )
+                                })
+                                .for_each(|msg| {
+                                    // Send message to create entity and tracked components and
+                                    // physics components
+                                    client.send_fallible(ServerGeneral::CreateEntity(msg));
+                                })
                         }
                     }
                 }
@@ -228,39 +229,40 @@ pub fn initialize_region_subscription(world: &World, entity: specs::Entity) {
         let tracked_comps = TrackedComps::fetch(world);
         for key in &regions {
             if let Some(region) = region_map.get(*key) {
-                for (pos, vel, ori, _, entity) in (
+                (
                     &world.read_storage::<Pos>(), // We assume all these entities have a position
                     world.read_storage::<Vel>().maybe(),
                     world.read_storage::<Ori>().maybe(),
                     region.entities(),
                     &world.entities(),
                 )
-                    .join()
-                    // Don't send client its own components because we do that below
-                    .filter(|t| t.4 != entity)
-                {
+                .join()
+                // Don't send client its own components because we do that below
+                .filter(|t| t.4 != entity)
+                .filter_map(|(pos, vel, ori, _, entity)|
+                    tracked_comps.create_entity_package(
+                        entity,
+                        Some(*pos),
+                        vel.copied(),
+                        ori.copied(),
+                    )
+                )
+                .for_each(|msg| {
                     // Send message to create entity and tracked components and physics components
-                    client.send_fallible(ServerGeneral::CreateEntity(
-                        tracked_comps.create_entity_package(
-                            entity,
-                            Some(*pos),
-                            vel.copied(),
-                            ori.copied(),
-                        ),
-                    ));
-                }
+                    client.send_fallible(ServerGeneral::CreateEntity(msg));
+                });
             }
         }
         // If client position was modified it might not be updated in the region system
         // so we send its components here
-        client.send_fallible(ServerGeneral::CreateEntity(
-            tracked_comps.create_entity_package(
-                entity,
-                Some(*client_pos),
-                world.read_storage().get(entity).copied(),
-                world.read_storage().get(entity).copied(),
-            ),
-        ));
+        if let Some(pkg) = tracked_comps.create_entity_package(
+            entity,
+            Some(*client_pos),
+            world.read_storage().get(entity).copied(),
+            world.read_storage().get(entity).copied(),
+        ) {
+            client.send_fallible(ServerGeneral::CreateEntity(pkg));
+        }
 
         if let Err(e) = world.write_storage().insert(entity, RegionSubscription {
             fuzzy_chunk,

voxygen/src/menu/main/mod.rs

@@ -126,6 +126,11 @@ impl PlayState for MainMenuState {
                             localized_strings.get("main.login.server_not_found").into()
                         },
                         InitError::ClientError(err) => match err {
+                            client::Error::SpecsErr(e) => format!(
+                                "{}: {}",
+                                localized_strings.get("main.login.internal_error"),
+                                e
+                            ),
                             client::Error::AuthErr(e) => format!(
                                 "{}: {}",
                                 localized_strings.get("main.login.authentication_error"),

voxygen/src/mesh/greedy.rs

@@ -11,7 +11,7 @@ type TodoRect = (
     Vec3<i32>,
 );
 
-pub struct GreedyConfig<D, FL, FG, FC, FO, FS, FP, FT> {
+pub struct GreedyConfig<D, FL, FG, FO, FS, FP, FT> {
     pub data: D,
     /// The minimum position to mesh, in the coordinate system used
     /// for queries against the volume.
@@ -39,9 +39,6 @@ pub struct GreedyConfig<D, FL, FG, FC, FO, FS, FP, FT> {
     /// Given a position, return the glow information for the voxel at that
     /// position (i.e: additional non-sun light).
     pub get_glow: FG,
-    /// Given a position, return the color information for the voxel at that
-    /// position.
-    pub get_color: FC,
     /// Given a position, return the opacity information for the voxel at that
     /// position. Currently, we don't support real translucent lighting, so the
     /// value should either be `false` (for opaque blocks) or `true`
@@ -63,8 +60,8 @@ pub struct GreedyConfig<D, FL, FG, FC, FO, FS, FP, FT> {
     /// world space, the normal facing out frmo the rectangle in world
     /// space, and meta information common to every voxel in this rectangle.
     pub push_quad: FP,
-    /// Create a texel (in the texture atlas) that corresponds to a face with
-    /// the given properties.
+    /// Given a position and the lighting information for a face at that
+    /// position, return the texel for the face at that position.
     pub make_face_texel: FT,
 }
 
@@ -146,17 +143,16 @@ impl<'a> GreedyMesh<'a> {
     /// Returns an estimate of the bounds of the current meshed model.
     ///
     /// For more information on the config parameter, see [GreedyConfig].
-    pub fn push<M: PartialEq, D: 'a, FL, FG, FC, FO, FS, FP, FT>(
+    pub fn push<M: PartialEq, D: 'a, FL, FG, FO, FS, FP, FT>(
         &mut self,
-        config: GreedyConfig<D, FL, FG, FC, FO, FS, FP, FT>,
+        config: GreedyConfig<D, FL, FG, FO, FS, FP, FT>,
     ) where
         FL: for<'r> FnMut(&'r mut D, Vec3<i32>) -> f32 + 'a,
         FG: for<'r> FnMut(&'r mut D, Vec3<i32>) -> f32 + 'a,
-        FC: for<'r> FnMut(&'r mut D, Vec3<i32>) -> Rgb<u8> + 'a,
         FO: for<'r> FnMut(&'r mut D, Vec3<i32>) -> bool + 'a,
         FS: for<'r> FnMut(&'r mut D, Vec3<i32>, Vec3<i32>, Vec2<Vec3<i32>>) -> Option<(bool, M)>,
         FP: FnMut(Vec2<u16>, Vec2<Vec2<u16>>, Vec3<f32>, Vec2<Vec3<f32>>, Vec3<f32>, &M),
-        FT: for<'r> FnMut(&'r mut D, Vec3<i32>, u8, u8, Rgb<u8>) -> <<ColLightFmt as gfx::format::Formatted>::Surface as gfx::format::SurfaceTyped>::DataType + 'a,
+        FT: for<'r> FnMut(&'r mut D, Vec3<i32>, u8, u8) -> <<ColLightFmt as gfx::format::Formatted>::Surface as gfx::format::SurfaceTyped>::DataType + 'a,
     {
         span!(_guard, "push", "GreedyMesh::push");
         let cont = greedy_mesh(
@@ -194,7 +190,7 @@ impl<'a> GreedyMesh<'a> {
     pub fn max_size(&self) -> guillotiere::Size { self.max_size }
 }
 
-fn greedy_mesh<'a, M: PartialEq, D: 'a, FL, FG, FC, FO, FS, FP, FT>(
+fn greedy_mesh<'a, M: PartialEq, D: 'a, FL, FG, FO, FS, FP, FT>(
     atlas: &mut guillotiere::SimpleAtlasAllocator,
     col_lights_size: &mut Vec2<u16>,
     max_size: guillotiere::Size,
@@ -205,21 +201,19 @@ fn greedy_mesh<'a, M: PartialEq, D: 'a, FL, FG, FC, FO, FS, FP, FT>(
         greedy_size_cross,
         get_light,
         get_glow,
-        get_color,
         get_opacity,
         mut should_draw,
         mut push_quad,
         make_face_texel,
-    }: GreedyConfig<D, FL, FG, FC, FO, FS, FP, FT>,
+    }: GreedyConfig<D, FL, FG, FO, FS, FP, FT>,
 ) -> Box<SuspendedMesh<'a>>
 where
     FL: for<'r> FnMut(&'r mut D, Vec3<i32>) -> f32 + 'a,
     FG: for<'r> FnMut(&'r mut D, Vec3<i32>) -> f32 + 'a,
-    FC: for<'r> FnMut(&'r mut D, Vec3<i32>) -> Rgb<u8> + 'a,
     FO: for<'r> FnMut(&'r mut D, Vec3<i32>) -> bool + 'a,
     FS: for<'r> FnMut(&'r mut D, Vec3<i32>, Vec3<i32>, Vec2<Vec3<i32>>) -> Option<(bool, M)>,
     FP: FnMut(Vec2<u16>, Vec2<Vec2<u16>>, Vec3<f32>, Vec2<Vec3<f32>>, Vec3<f32>, &M),
-    FT: for<'r> FnMut(&'r mut D, Vec3<i32>, u8, u8, Rgb<u8>) -> <<ColLightFmt as gfx::format::Formatted>::Surface as gfx::format::SurfaceTyped>::DataType + 'a,
+    FT: for<'r> FnMut(&'r mut D, Vec3<i32>, u8, u8) -> <<ColLightFmt as gfx::format::Formatted>::Surface as gfx::format::SurfaceTyped>::DataType + 'a,
 {
     span!(_guard, "greedy_mesh");
     // TODO: Collect information to see if we can choose a good value here.
@@ -357,7 +351,6 @@ where
             draw_delta,
             get_light,
             get_glow,
-            get_color,
             get_opacity,
             make_face_texel,
         );
@@ -513,9 +506,8 @@ fn draw_col_lights<D>(
     draw_delta: Vec3<i32>,
     mut get_light: impl FnMut(&mut D, Vec3<i32>) -> f32,
     mut get_glow: impl FnMut(&mut D, Vec3<i32>) -> f32,
-    mut get_color: impl FnMut(&mut D, Vec3<i32>) -> Rgb<u8>,
     mut get_opacity: impl FnMut(&mut D, Vec3<i32>) -> bool,
-    mut make_face_texel: impl FnMut(&mut D, Vec3<i32>, u8, u8, Rgb<u8>) -> <<ColLightFmt as gfx::format::Formatted>::Surface as gfx::format::SurfaceTyped>::DataType,
+    mut make_face_texel: impl FnMut(&mut D, Vec3<i32>, u8, u8) -> <<ColLightFmt as gfx::format::Formatted>::Surface as gfx::format::SurfaceTyped>::DataType,
 ) {
     todo_rects.into_iter().for_each(|(pos, uv, rect, delta)| {
         // NOTE: Conversions are safe because width, height, and offset must be
@@ -589,10 +581,9 @@ fn draw_col_lights<D>(
                             0.0
                         })
                         / 4.0;
-                    let col = get_color(data, pos);
                     let light = (darkness * 31.5) as u8;
                     let glow = (glowiness * 31.5) as u8;
-                    *col_light = make_face_texel(data, pos, light, glow, col);
+                    *col_light = make_face_texel(data, pos, light, glow);
                 });
         });
     });

voxygen/src/mesh/segment.rs

@@ -78,12 +78,6 @@ where
             }
         };
         let get_glow = |_vol: &mut V, _pos: Vec3<i32>| 0.0;
-        let get_color = |vol: &mut V, pos: Vec3<i32>| {
-            vol.get(pos)
-                .ok()
-                .and_then(|vox| vox.get_color())
-                .unwrap_or(Rgb::zero())
-        };
         let get_opacity =
             |vol: &mut V, pos: Vec3<i32>| vol.get(pos).map(|vox| vox.is_empty()).unwrap_or(true);
         let should_draw = |vol: &mut V, pos: Vec3<i32>, delta: Vec3<i32>, uv| {
@@ -102,7 +96,6 @@ where
             greedy_size_cross,
             get_light,
             get_glow,
-            get_color,
             get_opacity,
             should_draw,
             push_quad: |atlas_origin, dim, origin, draw_dim, norm, meta: &()| {
@@ -116,11 +109,12 @@ where
                     |atlas_pos, pos, norm, &_meta| create_opaque(atlas_pos, pos, norm),
                 ));
             },
-            make_face_texel: |vol: &mut V, pos, light, _, col| {
-                let (glowy, shiny) = vol
-                    .get(pos)
+            make_face_texel: |vol: &mut V, pos, light, _| {
+                let cell = vol.get(pos).ok();
+                let (glowy, shiny) = cell
                     .map(|c| (c.is_glowy(), c.is_shiny()))
                     .unwrap_or_default();
+                let col = cell.and_then(|vox| vox.get_color()).unwrap_or(Rgb::zero());
                 TerrainVertex::make_col_light_figure(light, glowy, shiny, col)
             },
         });
@@ -212,7 +206,6 @@ where
             greedy_size_cross,
             get_light,
             get_glow,
-            get_color,
             get_opacity,
             should_draw,
             push_quad: |atlas_origin, dim, origin, draw_dim, norm, meta: &bool| {
@@ -226,8 +219,8 @@ where
                     |atlas_pos, pos, norm, &meta| create_opaque(atlas_pos, pos, norm, meta),
                 ));
             },
-            make_face_texel: |_: &mut V, _, light, glow, col| {
-                TerrainVertex::make_col_light(light, glow, col)
+            make_face_texel: move |vol: &mut V, pos, light, glow| {
+                TerrainVertex::make_col_light(light, glow, get_color(vol, pos))
             },
         });
 
@@ -311,7 +304,6 @@ where
             greedy_size_cross,
             get_light,
             get_glow,
-            get_color,
             get_opacity,
             should_draw,
             push_quad: |atlas_origin, dim, origin, draw_dim, norm, meta: &()| {
@@ -325,8 +317,8 @@ where
                     |atlas_pos, pos, norm, &_meta| create_opaque(atlas_pos, pos, norm),
                 ));
             },
-            make_face_texel: |_: &mut V, _, light, glow, col| {
-                TerrainVertex::make_col_light(light, glow, col)
+            make_face_texel: move |vol: &mut V, pos, light, glow| {
+                TerrainVertex::make_col_light(light, glow, get_color(vol, pos))
             },
         });
 

voxygen/src/mesh/terrain.rs

@@ -412,7 +412,6 @@ impl<'a, V: RectRasterableVol<Vox = Block> + ReadVol + Debug + 'static>
             greedy_size_cross,
             get_light,
             get_glow,
-            get_color,
             get_opacity,
             should_draw,
             push_quad: |atlas_origin, dim, origin, draw_dim, norm, meta: &FaceKind| match meta {
@@ -439,8 +438,8 @@ impl<'a, V: RectRasterableVol<Vox = Block> + ReadVol + Debug + 'static>
                     ));
                 },
             },
-            make_face_texel: |_: &mut (), _, light, glow, col| {
-                TerrainVertex::make_col_light(light, glow, col)
+            make_face_texel: |data: &mut (), pos, light, glow| {
+                TerrainVertex::make_col_light(light, glow, get_color(data, pos))
             },
         });