Implement persistence for modular weapons.

This stores the components as children of the item that contains them via the DB's `parent_container_item_id` feature, and ensures that things are loaded in a good order with breadth-first search.

Squahed fixes:

- Fix some constraint violations that occurred when swapping inventory items.
- Comment out recipes for modular weapons.
- Make update_item_at_slot_using_persistence_key and is_modular more idiomatic.
- Add changelog entry.
- Document `defer_foreign_keys` usage.

CHANGELOG.md

@@ -31,6 +31,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Basic NPC interaction
 - Lights in dungeons
 - Trading system (bound to the `R` key by default, currently only works with players)
+- Support for modular weapons.
 
 ### Changed
 

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

@@ -240,13 +240,13 @@ impl ItemDef {
     }
 
     pub fn is_modular(&self) -> bool {
-        match &self.kind {
-            ItemKind::Tool(tool) => match tool.stats {
-                tool::StatKind::Direct { .. } => false,
-                tool::StatKind::Modular => true,
-            },
-            _ => false,
-        }
+        matches!(
+            &self.kind,
+            ItemKind::Tool(tool::Tool {
+                stats: tool::StatKind::Modular,
+                ..
+            })
+        )
     }
 
     #[cfg(test)]
@@ -372,25 +372,16 @@ impl Item {
             components.extend(input_components.iter().map(|comp| comp.duplicate()));
         }
 
-        let kind = inner_item.kind();
-        let item_config = if let ItemKind::Tool(_) = kind {
-            Some(Box::new(ItemConfig::from((
-                kind,
-                &*components,
-                &inner_item.ability_map,
-            ))))
-        } else {
-            None
-        };
-
-        Item {
+        let mut item = Item {
             item_id: Arc::new(AtomicCell::new(None)),
             amount: NonZeroU32::new(1).unwrap(),
             components,
             slots: vec![None; inner_item.slots as usize],
             item_def: inner_item,
-            item_config,
-        }
+            item_config: None,
+        };
+        item.update_item_config();
+        item
     }
 
     /// Creates a new instance of an `Item` from the provided asset identifier
@@ -478,6 +469,27 @@ impl Item {
         }
     }
 
+    pub fn add_component(&mut self, component: Item) {
+        // TODO: hook for typechecking (not needed atm if this is only used by DB
+        // persistence, but will definitely be needed once enhancement slots are
+        // added to prevent putting a sword into another sword)
+        self.components.push(component);
+        // adding a component changes the stats, so recalculate the ItemConfig
+        self.update_item_config();
+    }
+
+    fn update_item_config(&mut self) {
+        self.item_config = if let ItemKind::Tool(_) = self.kind() {
+            Some(Box::new(ItemConfig::from((
+                self.kind(),
+                self.components(),
+                &self.item_def.ability_map,
+            ))))
+        } else {
+            None
+        };
+    }
+
     /// Returns an iterator that drains items contained within the item's slots
     pub fn drain(&mut self) -> impl Iterator<Item = Item> + '_ {
         self.slots.iter_mut().filter_map(|x| mem::take(x))

common/src/comp/inventory/loadout.rs

@@ -46,6 +46,7 @@ pub(super) struct LoadoutSlotId {
 
 pub enum LoadoutError {
     InvalidPersistenceKey,
+    NoParentAtSlot,
 }
 
 impl Loadout {
@@ -134,6 +135,25 @@ impl Loadout {
         }
     }
 
+    pub fn update_item_at_slot_using_persistence_key<F: FnOnce(&mut Item)>(
+        &mut self,
+        persistence_key: &str,
+        f: F,
+    ) -> Result<(), LoadoutError> {
+        self.slots
+            .iter_mut()
+            .find(|loadout_slot| loadout_slot.persistence_key == persistence_key)
+            .map_or(Err(LoadoutError::InvalidPersistenceKey), |loadout_slot| {
+                loadout_slot
+                    .slot
+                    .as_mut()
+                    .map_or(Err(LoadoutError::NoParentAtSlot), |item| {
+                        f(item);
+                        Ok(())
+                    })
+            })
+    }
+
     /// Swaps the contents of two loadout slots
     pub(super) fn swap_slots(&mut self, equip_slot_a: EquipSlot, equip_slot_b: EquipSlot) {
         if self.slot(equip_slot_b).is_none() || self.slot(equip_slot_b).is_none() {

common/src/comp/inventory/mod.rs

@@ -350,7 +350,7 @@ impl Inventory {
         }
     }
 
-    fn slot_mut(&mut self, inv_slot_id: InvSlotId) -> Option<&mut InvSlot> {
+    pub fn slot_mut(&mut self, inv_slot_id: InvSlotId) -> Option<&mut InvSlot> {
         match SlotId::from(inv_slot_id) {
             SlotId::Inventory(slot_idx) => self.slots.get_mut(slot_idx),
             SlotId::Loadout(loadout_slot_id) => self.loadout.inv_slot_mut(loadout_slot_id),

common/src/recipe.rs

@@ -119,7 +119,13 @@ impl assets::Compound for RecipeBook {
         }
 
         let mut raw = cache.load::<RawRecipeBook>(specifier)?.read().clone();
+
+        // Avoid showing purple-question-box recipes until the assets are added
+        // (the `if false` is needed because commenting out the call will add a warning
+        // that there are no other uses of append_modular_recipes)
+        if false {
             modular::append_modular_recipes(&mut raw);
+        }
 
         let recipes = raw
             .0

server/src/persistence/character.rs

@@ -27,7 +27,7 @@ use crate::{
 use common::character::{CharacterId, CharacterItem, MAX_CHARACTERS_PER_PLAYER};
 use core::ops::Range;
 use diesel::{prelude::*, sql_query, sql_types::BigInt};
-use std::sync::Arc;
+use std::{collections::VecDeque, sync::Arc};
 use tracing::{error, trace, warn};
 
 /// Private module for very tightly coupled database conversion methods.  In
@@ -50,6 +50,26 @@ struct CharacterContainers {
     loadout_container_id: EntityId,
 }
 
+/// BFS the inventory/loadout to ensure that each is topologically sorted in the
+/// sense required by convert_inventory_from_database_items to support recursive
+/// items
+pub fn load_items_bfs(connection: VelorenTransaction, root: i64) -> Result<Vec<Item>, Error> {
+    use schema::item::dsl::*;
+    let mut items = Vec::new();
+    let mut queue = VecDeque::new();
+    queue.push_front(root);
+    while let Some(id) = queue.pop_front() {
+        let frontier = item
+            .filter(parent_container_item_id.eq(id))
+            .load::<Item>(&*connection)?;
+        for i in frontier.iter() {
+            queue.push_back(i.item_id);
+        }
+        items.extend(frontier);
+    }
+    Ok(items)
+}
+
 /// Load stored data for a character.
 ///
 /// After first logging in, and after a character is selected, we fetch this
@@ -59,19 +79,12 @@ pub fn load_character_data(
     char_id: CharacterId,
     connection: VelorenTransaction,
 ) -> CharacterDataResult {
-    use schema::{body::dsl::*, character::dsl::*, item::dsl::*, skill_group::dsl::*};
+    use schema::{body::dsl::*, character::dsl::*, skill_group::dsl::*};
 
     let character_containers = get_pseudo_containers(connection, char_id)?;
 
-    // TODO: Make inventory and loadout item loading work with recursive items when
-    // container items are supported
-    let inventory_items = item
-        .filter(parent_container_item_id.eq(character_containers.inventory_container_id))
-        .load::<Item>(&*connection)?;
-
-    let loadout_items = item
-        .filter(parent_container_item_id.eq(character_containers.loadout_container_id))
-        .load::<Item>(&*connection)?;
+    let inventory_items = load_items_bfs(connection, character_containers.inventory_container_id)?;
+    let loadout_items = load_items_bfs(connection, character_containers.loadout_container_id)?;
 
     let character_data = character
         .filter(
@@ -109,7 +122,12 @@ pub fn load_character_data(
     Ok((
         convert_body_from_database(&char_body)?,
         convert_stats_from_database(character_data.alias, &skill_data, &skill_group_data),
-        convert_inventory_from_database_items(&inventory_items, &loadout_items)?,
+        convert_inventory_from_database_items(
+            character_containers.inventory_container_id,
+            &inventory_items,
+            character_containers.loadout_container_id,
+            &loadout_items,
+        )?,
         char_waypoint,
     ))
 }
@@ -125,7 +143,7 @@ pub fn load_character_list(
     player_uuid_: &str,
     connection: VelorenTransaction,
 ) -> CharacterListResult {
-    use schema::{body::dsl::*, character::dsl::*, item::dsl::*};
+    use schema::{body::dsl::*, character::dsl::*};
 
     let result = character
         .filter(player_uuid.eq(player_uuid_))
@@ -149,13 +167,10 @@ pub fn load_character_list(
                 LOADOUT_PSEUDO_CONTAINER_POSITION,
             )?;
 
-            // TODO: Make work with recursive items if containers are ever supported as part
-            // of a loadout
-            let loadout_items = item
-                .filter(parent_container_item_id.eq(loadout_container_id))
-                .load::<Item>(&*connection)?;
+            let loadout_items = load_items_bfs(connection, loadout_container_id)?;
 
-            let loadout = convert_loadout_from_database_items(&loadout_items)?;
+            let loadout =
+                convert_loadout_from_database_items(loadout_container_id, &loadout_items)?;
 
             Ok(CharacterItem {
                 character: char,
@@ -276,7 +291,7 @@ pub fn create_character(
     let mut inserts = Vec::new();
 
     get_new_entity_ids(connection, |mut next_id| {
-        let (inserts_, _deletes) = convert_items_to_database_items(
+        let inserts_ = convert_items_to_database_items(
             loadout_container_id,
             &inventory,
             inventory_container_id,
@@ -541,7 +556,7 @@ pub fn update(
     // First, get all the entity IDs for any new items, and identify which slots to
     // upsert and which ones to delete.
     get_new_entity_ids(connection, |mut next_id| {
-        let (upserts_, _deletes) = convert_items_to_database_items(
+        let upserts_ = convert_items_to_database_items(
             pseudo_containers.loadout_container_id,
             &inventory,
             pseudo_containers.inventory_container_id,
@@ -553,9 +568,17 @@ pub fn update(
 
     // Next, delete any slots we aren't upserting.
     trace!("Deleting items for character_id {}", char_id);
-    let existing_items = parent_container_item_id
-        .eq(pseudo_containers.inventory_container_id)
-        .or(parent_container_item_id.eq(pseudo_containers.loadout_container_id));
+    let mut existing_item_ids: Vec<i64> = vec![
+        pseudo_containers.inventory_container_id,
+        pseudo_containers.loadout_container_id,
+    ];
+    for it in load_items_bfs(connection, pseudo_containers.inventory_container_id)? {
+        existing_item_ids.push(it.item_id);
+    }
+    for it in load_items_bfs(connection, pseudo_containers.loadout_container_id)? {
+        existing_item_ids.push(it.item_id);
+    }
+    let existing_items = parent_container_item_id.eq_any(existing_item_ids);
     let non_upserted_items = item_id.ne_all(
         upserts
             .iter()
@@ -573,7 +596,13 @@ pub fn update(
     if expected_upsert_count > 0 {
         let (upserted_items, upserted_comps_): (Vec<_>, Vec<_>) = upserts
             .into_iter()
-            .map(|model_pair| (model_pair.model, model_pair.comp))
+            .map(|model_pair| {
+                debug_assert_eq!(
+                    model_pair.model.item_id,
+                    model_pair.comp.load().unwrap().get() as i64
+                );
+                (model_pair.model, model_pair.comp)
+            })
             .unzip();
         upserted_comps = upserted_comps_;
         trace!(
@@ -582,9 +611,17 @@ pub fn update(
             char_id
         );
 
+        // When moving inventory items around, foreign key constraints on
+        // `parent_container_item_id` can be temporarily violated by one upsert, but
+        // restored by another upsert. Deferred constraints allow SQLite to check this
+        // when committing the transaction. The `defer_foreign_keys` pragma treats the
+        // foreign key constraints as deferred for the next transaction (it turns itself
+        // off at the commit boundary). https://sqlite.org/foreignkeys.html#fk_deferred
+        connection.execute("PRAGMA defer_foreign_keys = ON;")?;
         let upsert_count = diesel::replace_into(item)
             .values(&upserted_items)
             .execute(&*connection)?;
+        trace!("upsert_count: {}", upsert_count);
         if upsert_count != expected_upsert_count {
             return Err(Error::OtherError(format!(
                 "Expected upsertions={}, actual={}, for char_id {}--unsafe to continue \

server/src/persistence/character/conversions.rs

@@ -21,16 +21,17 @@ use common::{
 };
 use core::{convert::TryFrom, num::NonZeroU64};
 use hashbrown::HashMap;
-use itertools::{Either, Itertools};
-use std::sync::Arc;
+use std::{collections::VecDeque, sync::Arc};
 
+#[derive(Debug)]
 pub struct ItemModelPair {
     pub comp: Arc<common::comp::item::ItemId>,
     pub model: Item,
 }
 
-/// The left vector contains all item rows to upsert; the right-hand vector
-/// contains all item rows to delete (by parent ID and position).
+/// Returns a vector that contains all item rows to upsert; parent is
+/// responsible for deleting items from the same owner that aren't affirmatively
+/// kept by this.
 ///
 /// NOTE: This method does not yet handle persisting nested items within
 /// inventories. Although loadout items do store items inside them this does
@@ -41,7 +42,7 @@ pub fn convert_items_to_database_items(
     inventory: &Inventory,
     inventory_container_id: EntityId,
     next_id: &mut i64,
-) -> (Vec<ItemModelPair>, Vec<(EntityId, String)>) {
+) -> Vec<ItemModelPair> {
     let loadout = inventory
         .loadout_items_with_persistence_key()
         .map(|(slot, item)| (slot.to_string(), item, loadout_container_id));
@@ -55,24 +56,27 @@ pub fn convert_items_to_database_items(
         )
     });
 
+    // Use Breadth-first search to recurse into containers/modular weapons to store
+    // their parts
+    let mut bfs_queue: VecDeque<_> = inventory.chain(loadout).collect();
+    let mut upserts = Vec::new();
+    let mut depth = HashMap::new();
+    depth.insert(inventory_container_id, 0);
+    depth.insert(loadout_container_id, 0);
+    while let Some((position, item, parent_container_item_id)) = bfs_queue.pop_front() {
         // Construct new items.
-    inventory.chain(loadout)
-        .partition_map(|(position, item, parent_container_item_id)| {
         if let Some(item) = item {
             // Try using the next available id in the sequence as the default for new items.
-                let new_item_id = NonZeroU64::new(u64::try_from(*next_id)
-                    .expect("We are willing to crash if the next entity id overflows \
-                (or is otherwise negative).")).expect("next_id should not be zero, either");
+            let new_item_id = NonZeroU64::new(u64::try_from(*next_id).expect(
+                "We are willing to crash if the next entity id overflows (or is otherwise \
+                 negative).",
+            ))
+            .expect("next_id should not be zero, either");
 
-                let comp = item.get_item_id_for_database();
-                Either::Left(ItemModelPair {
-                    model: Item {
-                        item_definition_id: item.item_definition_id().to_owned(),
-                        position,
-                        parent_container_item_id,
             // Fast (kinda) path: acquire read for the common case where an id has
             // already been assigned.
-                        item_id: comp.load()
+            let comp = item.get_item_id_for_database();
+            let item_id = comp.load()
                 // First, we filter out "impossible" entity IDs--IDs that are larger
                 // than the maximum sequence value (next_id).  This is important
                 // because we update the item ID atomically, *before* we know whether
@@ -137,7 +141,24 @@ pub fn convert_items_to_database_items(
                                 .expect("We always choose legal EntityIds as item ids")
                         },
                     }
-                            }),
+                });
+
+            depth.insert(item_id, depth[&parent_container_item_id] + 1);
+
+            for (i, component) in item.components().iter().enumerate() {
+                // recursive items' children have the same position as their parents, and since
+                // they occur afterwards in the topological sort of the parent graph (which
+                // should still always be a tree, even with recursive items), we
+                // have enough information to put them back into their parents on load
+                bfs_queue.push_back((format!("component_{}", i), Some(component), item_id));
+            }
+
+            let upsert = ItemModelPair {
+                model: Item {
+                    item_definition_id: item.item_definition_id().to_owned(),
+                    position,
+                    parent_container_item_id,
+                    item_id,
                     stack_size: if item.is_stackable() {
                         item.amount() as i32
                     } else {
@@ -147,11 +168,13 @@ pub fn convert_items_to_database_items(
                 // Continue to remember the atomic, in case we detect an error later and want
                 // to roll back to preserve liveness.
                 comp,
-                })
-            } else {
-                Either::Right((parent_container_item_id, position))
+            };
+            upserts.push(upsert);
         }
-        })
+    }
+    upserts.sort_by_key(|pair| (depth[&pair.model.item_id], pair.model.item_id));
+    tracing::debug!("upserts: {:#?}", upserts);
+    upserts
 }
 
 pub fn convert_body_to_database_json(body: &CompBody) -> Result<String, Error> {
@@ -192,23 +215,30 @@ pub fn convert_waypoint_from_database_json(position: &str) -> Result<Waypoint, E
     Ok(Waypoint::new(character_position.waypoint, Time(0.0)))
 }
 
+/// Properly-recursive items (currently modular weapons) occupy the same
+/// inventory slot as their parent. The caller is responsible for ensuring that
+/// inventory_items and loadout_items are topologically sorted (i.e. forall i,
+/// `items[i].parent_container_item_id == x` implies exists j < i satisfying
+/// `items[j].item_id == x`)
 pub fn convert_inventory_from_database_items(
+    inventory_container_id: i64,
     inventory_items: &[Item],
+    loadout_container_id: i64,
     loadout_items: &[Item],
 ) -> Result<Inventory, Error> {
     // Loadout items must be loaded before inventory items since loadout items
     // provide inventory slots. Since items stored inside loadout items actually
     // have their parent_container_item_id as the loadout pseudo-container we rely
     // on populating the loadout items first, and then inserting the items into the
-    // inventory at the correct position. When we want to support items inside the
-    // player's inventory containing other items (such as "right click to
-    // unwrap" gifts perhaps) then we will need to refactor inventory/loadout
-    // persistence to traverse the tree of items and load them from the root
-    // down.
-    let loadout = convert_loadout_from_database_items(loadout_items)?;
+    // inventory at the correct position.
+    //
+    let loadout = convert_loadout_from_database_items(loadout_container_id, loadout_items)?;
     let mut inventory = Inventory::new_with_loadout(loadout);
+    let mut item_indices = HashMap::new();
+
+    for (i, db_item) in inventory_items.iter().enumerate() {
+        item_indices.insert(db_item.item_id, i);
 
-    for db_item in inventory_items.iter() {
         let mut item = get_item_from_asset(db_item.item_definition_id.as_str())?;
 
         // NOTE: Since this is freshly loaded, the atomic is *unique.*
@@ -234,13 +264,17 @@ pub fn convert_inventory_from_database_items(
         // Insert item into inventory
 
         // Slot position
-        let slot: InvSlotId = serde_json::from_str(&db_item.position).map_err(|_| {
+        let slot = |s: &str| {
+            serde_json::from_str::<InvSlotId>(s).map_err(|_| {
                 Error::ConversionError(format!(
                     "Failed to parse item position: {:?}",
                     &db_item.position
                 ))
-        })?;
+            })
+        };
 
+        if db_item.parent_container_item_id == inventory_container_id {
+            let slot = slot(&db_item.position)?;
             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
@@ -258,31 +292,71 @@ pub fn convert_inventory_from_database_items(
                     "Inserted an item into the same slot twice".to_string(),
                 ));
             }
+        } else if let Some(&j) = item_indices.get(&db_item.parent_container_item_id) {
+            if let Some(Some(parent)) = inventory.slot_mut(slot(&inventory_items[j].position)?) {
+                parent.add_component(item);
+            } else {
+                return Err(Error::ConversionError(format!(
+                    "Parent slot {} for component {} was empty even though it occurred earlier in \
+                     the loop?",
+                    db_item.parent_container_item_id, db_item.item_id
+                )));
+            }
+        } else {
+            return Err(Error::ConversionError(format!(
+                "Couldn't find parent item {} before item {} in inventory",
+                db_item.parent_container_item_id, db_item.item_id
+            )));
+        }
     }
 
     Ok(inventory)
 }
 
-pub fn convert_loadout_from_database_items(database_items: &[Item]) -> Result<Loadout, Error> {
+pub fn convert_loadout_from_database_items(
+    loadout_container_id: i64,
+    database_items: &[Item],
+) -> Result<Loadout, Error> {
     let loadout_builder = LoadoutBuilder::new();
     let mut loadout = loadout_builder.build();
+    let mut item_indices = HashMap::new();
+
+    for (i, db_item) in database_items.iter().enumerate() {
+        item_indices.insert(db_item.item_id, i);
 
-    for db_item in database_items.iter() {
         let item = get_item_from_asset(db_item.item_definition_id.as_str())?;
+
         // NOTE: item id is currently *unique*, so we can store the ID safely.
         let comp = item.get_item_id_for_database();
         comp.store(Some(NonZeroU64::try_from(db_item.item_id as u64).map_err(
             |_| Error::ConversionError("Item with zero item_id".to_owned()),
         )?));
 
+        let convert_error = |err| match err {
+            LoadoutError::InvalidPersistenceKey => {
+                Error::ConversionError(format!("Invalid persistence key: {}", &db_item.position))
+            },
+            LoadoutError::NoParentAtSlot => {
+                Error::ConversionError(format!("No parent item at slot: {}", &db_item.position))
+            },
+        };
+
+        if db_item.parent_container_item_id == loadout_container_id {
             loadout
                 .set_item_at_slot_using_persistence_key(&db_item.position, item)
-            .map_err(|err| match err {
-                LoadoutError::InvalidPersistenceKey => Error::ConversionError(format!(
-                    "Invalid persistence key: {}",
-                    &db_item.position
-                )),
-            })?;
+                .map_err(convert_error)?;
+        } else if let Some(&j) = item_indices.get(&db_item.parent_container_item_id) {
+            loadout
+                .update_item_at_slot_using_persistence_key(&database_items[j].position, |parent| {
+                    parent.add_component(item);
+                })
+                .map_err(convert_error)?;
+        } else {
+            return Err(Error::ConversionError(format!(
+                "Couldn't find parent item {} before item {} in loadout",
+                db_item.parent_container_item_id, db_item.item_id
+            )));
+        }
     }
 
     Ok(loadout)