From 5bacf526ad91817616c1b365780affd0d1f0ccfa Mon Sep 17 00:00:00 2001 From: Sam Date: Mon, 15 Nov 2021 01:05:50 -0500 Subject: [PATCH] Began addressing initial review of modular weapons. --- common/src/comp/inventory/item/modular.rs | 75 ++++++++----------- common/src/comp/inventory/item/tool.rs | 4 +- common/src/comp/inventory/loadout.rs | 14 +--- common/src/comp/inventory/loadout_builder.rs | 1 - common/src/comp/inventory/mod.rs | 7 +- common/src/comp/inventory/trade_pricing.rs | 4 +- common/src/recipe.rs | 22 ++++-- common/src/states/utils.rs | 2 +- server/src/events/inventory_manip.rs | 2 +- .../src/persistence/character/conversions.rs | 9 +-- voxygen/src/hud/crafting.rs | 37 ++++----- voxygen/src/hud/loot_scroller.rs | 2 +- voxygen/src/ui/widgets/item_tooltip.rs | 2 +- 13 files changed, 77 insertions(+), 104 deletions(-) diff --git a/common/src/comp/inventory/item/modular.rs b/common/src/comp/inventory/item/modular.rs index f2a7bae5c0..65cf856496 100644 --- a/common/src/comp/inventory/item/modular.rs +++ b/common/src/comp/inventory/item/modular.rs @@ -78,7 +78,7 @@ fn initialize_modular_assets() -> HashMap { let mut itemdefs = HashMap::new(); for &toolkind in &SUPPORTED_TOOLKINDS { let (identifier, item) = make_weapon_def(toolkind); - itemdefs.insert(identifier.clone(), item); + itemdefs.insert(identifier, item); } itemdefs } @@ -100,6 +100,19 @@ pub(super) fn synthesize_modular_asset(specifier: &str) -> Option { /// the damage component used and {Material} is from the material the damage /// component is created from. pub(super) fn modular_name<'a>(item: &'a Item, arg1: &'a str) -> Cow<'a, str> { + // Closure to get material name from an item + let material_name = |component: &Item| { + component + .components() + .iter() + .filter_map(|comp| match comp.kind() { + ItemKind::Ingredient { descriptor, .. } => Some(descriptor.to_owned()), + _ => None, + }) + .last() + .unwrap_or_else(|| "Modular".to_owned()) + }; + match item.kind() { ItemKind::Tool(tool) => { let main_components = item.components().iter().filter(|comp| { @@ -109,21 +122,7 @@ pub(super) fn modular_name<'a>(item: &'a Item, arg1: &'a str) -> Cow<'a, str> { }); // Last fine as there should only ever be one damage component on a weapon let (material_name, weapon_name) = if let Some(component) = main_components.last() { - let materials = - component - .components() - .iter() - .filter_map(|comp| match comp.kind() { - ItemKind::Ingredient { .. } => Some(comp.kind()), - _ => None, - }); - // TODO: Better handle multiple materials - let material_name = - if let Some(ItemKind::Ingredient { descriptor, .. }) = materials.last() { - descriptor - } else { - "Modular" - }; + let material_name = material_name(component); let weapon_name = if let ItemKind::ModularComponent(ModularComponent { weapon_name, .. }) = component.kind() @@ -134,32 +133,17 @@ pub(super) fn modular_name<'a>(item: &'a Item, arg1: &'a str) -> Cow<'a, str> { }; (material_name, weapon_name) } else { - ("Modular", tool.kind.identifier_name()) + ("Modular".to_owned(), tool.kind.identifier_name()) }; Cow::Owned(format!("{} {}", material_name, weapon_name)) }, - ItemKind::ModularComponent(comp) => { - match comp.modkind { - ModularComponentKind::Damage => { - let materials = item - .components() - .iter() - .filter_map(|comp| match comp.kind() { - ItemKind::Ingredient { .. } => Some(comp.kind()), - _ => None, - }); - // TODO: Better handle multiple materials - let material_name = - if let Some(ItemKind::Ingredient { descriptor, .. }) = materials.last() { - descriptor - } else { - "Modular" - }; - Cow::Owned(format!("{} {}", material_name, arg1)) - }, - ModularComponentKind::Held => Cow::Borrowed(arg1), - } + ItemKind::ModularComponent(comp) => match comp.modkind { + ModularComponentKind::Damage => { + let material_name = material_name(item); + Cow::Owned(format!("{} {}", material_name, arg1)) + }, + ModularComponentKind::Held => Cow::Borrowed(arg1), }, _ => Cow::Borrowed("Modular Item"), } @@ -168,12 +152,12 @@ pub(super) fn modular_name<'a>(item: &'a Item, arg1: &'a str) -> Cow<'a, str> { pub(super) fn resolve_quality(item: &Item) -> super::Quality { item.components .iter() - .fold(super::Quality::Common, |a, b| a.max(b.quality())) + .fold(super::Quality::Low, |a, b| a.max(b.quality())) } /// Returns directory that contains components for a particular combination of /// toolkind and modular component kind -fn make_mod_comp_dir_def(tool: ToolKind, mod_kind: ModularComponentKind) -> String { +fn make_mod_comp_dir_spec(tool: ToolKind, mod_kind: ModularComponentKind) -> String { const MOD_COMP_DIR_PREFIX: &str = "common.items.crafting_ing.modular"; format!( "{}.{}.{}", @@ -263,7 +247,7 @@ pub fn random_weapon(tool: ToolKind, material: super::Material, hands: Option ModularWeaponKey { @@ -329,6 +314,8 @@ pub fn weapon_to_key(mod_weap: &dyn ItemDesc) -> ModularWeaponKey { (main_comp.to_owned(), material.to_owned(), hands) } +/// This is used as a key to uniquely identify the modular weapon in asset +/// manifests in voxygen (Main component, material) pub type ModularWeaponComponentKey = (String, String); pub enum ModularWeaponComponentKeyError { diff --git a/common/src/comp/inventory/item/tool.rs b/common/src/comp/inventory/item/tool.rs index 68df435a1f..ff932632d2 100644 --- a/common/src/comp/inventory/item/tool.rs +++ b/common/src/comp/inventory/item/tool.rs @@ -144,7 +144,7 @@ impl Stats { } } - pub fn oned() -> Stats { + pub fn one() -> Stats { Stats { equip_time_secs: 1.0, power: 1.0, @@ -254,7 +254,7 @@ impl StatKind { pub fn resolve_stats(&self, msm: &MaterialStatManifest, components: &[Item]) -> Stats { let mut stats = match self { StatKind::Direct(stats) => *stats, - StatKind::Modular => Stats::oned(), + StatKind::Modular => Stats::one(), }; let mut multipliers: Vec = Vec::new(); for item in components.iter() { diff --git a/common/src/comp/inventory/loadout.rs b/common/src/comp/inventory/loadout.rs index 35098ed364..ecbfd78b4c 100644 --- a/common/src/comp/inventory/loadout.rs +++ b/common/src/comp/inventory/loadout.rs @@ -151,15 +151,6 @@ impl Loadout { .as_mut() .ok_or(LoadoutError::NoParentAtSlot) }) - // .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 @@ -208,9 +199,8 @@ impl Loadout { .or_else(|| first.map(|x| x.equip_slot)) } - /// Returns all items currently equipped that an item of the given ItemKind - /// could replace - pub(super) fn equipped_items_of_kind<'a>( + /// Returns all items currently equipped that an item could replace + pub(super) fn equipped_items_replaceable_by<'a>( &'a self, item: &'a Item, ) -> impl Iterator { diff --git a/common/src/comp/inventory/loadout_builder.rs b/common/src/comp/inventory/loadout_builder.rs index 10f5d159df..b3f3179f86 100644 --- a/common/src/comp/inventory/loadout_builder.rs +++ b/common/src/comp/inventory/loadout_builder.rs @@ -169,7 +169,6 @@ impl Hands { hands, material, tool, equip_slot ); } - std::mem::drop(item); }, } } diff --git a/common/src/comp/inventory/mod.rs b/common/src/comp/inventory/mod.rs index 6b130ce782..f0ddeb3da2 100644 --- a/common/src/comp/inventory/mod.rs +++ b/common/src/comp/inventory/mod.rs @@ -775,8 +775,11 @@ impl Inventory { true } - pub fn equipped_items_of_kind<'a>(&'a self, item: &'a Item) -> impl Iterator { - self.loadout.equipped_items_of_kind(item) + pub fn equipped_items_replaceable_by<'a>( + &'a self, + item: &'a Item, + ) -> impl Iterator { + self.loadout.equipped_items_replaceable_by(item) } pub fn swap_equipped_weapons(&mut self) { self.loadout.swap_equipped_weapons() } diff --git a/common/src/comp/inventory/trade_pricing.rs b/common/src/comp/inventory/trade_pricing.rs index cd21d4430b..974f9aa0d3 100644 --- a/common/src/comp/inventory/trade_pricing.rs +++ b/common/src/comp/inventory/trade_pricing.rs @@ -259,9 +259,9 @@ impl From)>> for ProbabilityFile { .collect::>() .into_iter() }, - LootSpec::Nothing => Vec::new().into_iter(), + LootSpec::Nothing // TODO: Let someone else wrangle modular weapons into the economy - LootSpec::ModularWeapon { .. } => vec![].into_iter(), + | LootSpec::ModularWeapon { .. } => Vec::new().into_iter(), }) .collect(), } diff --git a/common/src/recipe.rs b/common/src/recipe.rs index 26dc0c57c5..96fc2bd5a0 100644 --- a/common/src/recipe.rs +++ b/common/src/recipe.rs @@ -13,11 +13,19 @@ use std::sync::Arc; #[derive(Clone, Debug, Serialize, Deserialize)] pub enum RecipeInput { + /// Only an item with a matching ItemDef can be used to satisfy this input Item(Arc), + /// Any items with this tag can be used to satisfy this input Tag(ItemTag), + /// Similar to RecipeInput::Tag(_), but all items must be the same. + /// Specifically this means that a mix of different items with the tag + /// cannot be used. TagSameItem(ItemTag, u32), - // List similar to tag, but has items defined in centralized file - // Intent is to make it harder for tag to be innocuously added to an item breaking a recipe + /// List is similar to tag, but has items defined in centralized file + /// Similar to RecipeInput::TagSameItem(_), all items must be the same, they + /// cannot be a mix of different items defined in the list. + // Intent of using List over Tag is to make it harder for tag to be innocuously added to an + // item breaking a recipe ListSameItem(Vec>, u32), } @@ -101,10 +109,10 @@ impl Recipe { .take(*slot, ability_map, msm) .expect("Expected item to exist in the inventory"); components.push(component); - let claimed = slot_claims + let to_remove = slot_claims .get_mut(slot) .expect("If marked in component slots, should be in slot claims"); - *claimed -= 1; + *to_remove -= 1; } for (slot, to_remove) in slot_claims.iter() { for _ in 0..*to_remove { @@ -376,10 +384,10 @@ impl assets::Compound for RecipeBook { RawRecipeInput::Tag(tag) => RecipeInput::Tag(*tag), RawRecipeInput::TagSameItem(tag) => RecipeInput::TagSameItem(*tag, *amount), RawRecipeInput::ListSameItem(list) => { - let assets = ItemList::load_expect_cloned(list).0; + let assets = &ItemList::load_expect(list).read().0; let items = assets - .into_iter() - .map(|asset| Arc::::load_expect_cloned(&asset)) + .iter() + .map(|asset| Arc::::load_expect_cloned(asset)) .collect(); RecipeInput::ListSameItem(items, *amount) }, diff --git a/common/src/states/utils.rs b/common/src/states/utils.rs index 06834f51e6..09ffe2dcaf 100644 --- a/common/src/states/utils.rs +++ b/common/src/states/utils.rs @@ -999,7 +999,7 @@ pub fn is_strafing(data: &JoinData<'_>, update: &StateUpdate) -> bool { (update.character.is_aimed() || update.should_strafe) && data.body.can_strafe() } -// Returns tool and components +/// Returns tool and components pub fn unwrap_tool_data<'a>( data: &'a JoinData, equip_slot: EquipSlot, diff --git a/server/src/events/inventory_manip.rs b/server/src/events/inventory_manip.rs index 99f8f641d6..f473c350ae 100644 --- a/server/src/events/inventory_manip.rs +++ b/server/src/events/inventory_manip.rs @@ -296,7 +296,7 @@ pub fn handle_inventory(server: &mut Server, entity: EcsEntity, manip: comp::Inv ItemKind::Consumable { effects, .. } => { maybe_effect = Some(effects.clone()); Some(comp::InventoryUpdateEvent::Consumed( - item.name().to_string(), + item.name().into_owned(), )) }, ItemKind::Throwable { kind, .. } => { diff --git a/server/src/persistence/character/conversions.rs b/server/src/persistence/character/conversions.rs index 333dbebe50..02452a79a5 100644 --- a/server/src/persistence/character/conversions.rs +++ b/server/src/persistence/character/conversions.rs @@ -291,8 +291,7 @@ where let component_index = inventory_items[index] .position .split('_') - .collect::>() - .get(1) + .nth(1) .and_then(|s| s.parse::().ok()); // Returns mutable reference to parent item of original item, by grabbing // the component representing the parent item from the parent item's parent @@ -468,12 +467,6 @@ pub fn convert_loadout_from_database_items( db_item.parent_container_item_id, db_item.item_id ))); } - // loadout - // .update_item_at_slot_using_persistence_key(& - // database_items[j].position, |parent| { - // parent.add_component(item, &ABILITY_MAP, - // &MATERIAL_STATS_MANIFEST); }) - // .map_err(convert_error)?; } else { return Err(PersistenceError::ConversionError(format!( "Couldn't find parent item {} before item {} in loadout", diff --git a/voxygen/src/hud/crafting.rs b/voxygen/src/hud/crafting.rs index 13f9e0bc21..830b7a35ca 100644 --- a/voxygen/src/hud/crafting.rs +++ b/voxygen/src/hud/crafting.rs @@ -472,11 +472,8 @@ impl<'a> Widget for Crafting<'a> { RecipeInput::TagSameItem(tag, _) => tag.name(), // Works, but probably will have some...interesting false positives // Code reviewers probably required to do magic to make not hacky - RecipeInput::ListSameItem(_defs, _) => { - // Cow::Owned(defs.iter().flat_map(|def| def.name().chars()).collect()) - // Code reviewers: I require someone who can solve a temporary value - // being created - Cow::Borrowed("Input") + RecipeInput::ListSameItem(defs, _) => { + Cow::Owned(defs.iter().map(|def| def.name()).collect()) }, } .to_lowercase(); @@ -919,42 +916,39 @@ impl<'a> Widget for Crafting<'a> { RecipeInput::Item(item_def) => Arc::clone(item_def), RecipeInput::Tag(tag) | RecipeInput::TagSameItem(tag, _) => { Arc::::load_expect_cloned( - &self + self .inventory .slots() - .filter_map(|slot| { + .find_map(|slot| { slot.as_ref().and_then(|item| { if item.matches_recipe_input(recipe_input) { - Some(item.item_definition_id().to_string()) + Some(item.item_definition_id()) } else { None } }) }) - .next() - .unwrap_or_else(|| tag.exemplar_identifier().to_string()), + .unwrap_or(&tag.exemplar_identifier()), ) }, RecipeInput::ListSameItem(item_defs, _) => Arc::::load_expect_cloned( - &self + self .inventory .slots() - .filter_map(|slot| { + .find_map(|slot| { slot.as_ref().and_then(|item| { if item.matches_recipe_input(recipe_input) { - Some(item.item_definition_id().to_string()) + Some(item.item_definition_id()) } else { None } }) }) - .next() - .unwrap_or_else(|| { + .unwrap_or({ item_defs .first() .map(|i| i.item_definition_id()) .unwrap_or("common.items.weapons.empty.empty") - .to_string() }), ), }; @@ -1058,12 +1052,11 @@ impl<'a> Widget for Crafting<'a> { RecipeInput::Tag(tag) | RecipeInput::TagSameItem(tag, _) => { format!("Any {} item", tag.name()) }, - RecipeInput::ListSameItem(_item_defs, _) => { - // format!("Any of {}", item_defs.iter().flat_map(|item_def| - // item_def.name().chars()).collect::()) - // Code reviewers: I require someone who can solve a temporary value - // being created - "Input".to_string() + RecipeInput::ListSameItem(item_defs, _) => { + format!( + "Any of {}", + item_defs.iter().map(|def| def.name()).collect::() + ) }, }; let input = format!( diff --git a/voxygen/src/hud/loot_scroller.rs b/voxygen/src/hud/loot_scroller.rs index 9ddefe1c77..2161a209e5 100644 --- a/voxygen/src/hud/loot_scroller.rs +++ b/voxygen/src/hud/loot_scroller.rs @@ -340,7 +340,7 @@ impl<'a> Widget for LootScroller<'a> { .set(state.ids.message_icons[i], ui); let label = if *amount == 1 { - item.name().to_string() + item.name().into_owned() } else { format!("{}x {}", amount, item.name()) }; diff --git a/voxygen/src/ui/widgets/item_tooltip.rs b/voxygen/src/ui/widgets/item_tooltip.rs index c24c4be155..f9b40bce42 100644 --- a/voxygen/src/ui/widgets/item_tooltip.rs +++ b/voxygen/src/ui/widgets/item_tooltip.rs @@ -464,7 +464,7 @@ impl<'a> Widget for ItemTooltip<'a> { let equip_slot = item .concrete_item() - .map(|item| inventory.equipped_items_of_kind(item)) + .map(|item| inventory.equipped_items_replaceable_by(item)) .into_iter() .flatten();