Merge branch 'juliancoffee/entity_refactor' into 'master'

Improve terms of EntityConfig and LoadoutBuilder

See merge request veloren/veloren!2688

assets/common/entity/template.ron

@@ -1,40 +1,16 @@
+/// Template file for EntityConfig, check documentation in code for more
+/// By the time of writing this comment it lives in common/src/generation.rs
 EntityConfig (
-    /// Name of Entity
-    /// Can be Name(String) with given name
-    /// or Automatic which will call automatic name depend on Body
-    /// or Uninit (means it should be specified somewhere in code)
     name: Name("Paddy"),
 
-    /// Body
-    /// Can be Exact (Body with all fields e.g BodyType, Species, Hair color and such)
-    /// or RandomWith (will generate random body or species)
-    /// or Uninit (means it should be specified somewhere in code)
     body: RandomWith("humanoid"),
 
-    /// Alignment, can be Uninit
     alignment: Alignment(Enemy),
 
-    /// Loot
-    /// Can be Item (with asset_specifier for item)
-    /// or LootTable (with asset_specifier for loot table)
-    /// or Uninit (means it should be specified something in the code)
     loot: LootTable("common.loot_tables.humanoids"),
 
-    /// Hands:
-    /// - TwoHanded(ItemSpec) for one 2h or 1h weapon,
-    /// - Paired(ItemSpec) for two 1h weapons aka berserker mode,
-    /// - Mix {
-    ///     mainhand: ItemSpec,
-    ///     offhand: ItemSpec,
-    /// } for two different 1h weapons,
-    /// - Uninit which means that tool should be specified somewhere in code,
-    /// Where ItemSpec is taken from loadout_builder module
     hands: TwoHanded(Item("common.items.weapons.sword.cultist")),
 
-    /// Meta Info
-    /// Possible fields:
-    /// LoadoutAsset(String) with asset_specifier for loadout
-    /// SkillSetAsset(String) with asset_specifier for skillset
     meta: [
         LoadoutAsset("common.loadout.village.merchant"),
         SkillSetAsset("common.skillset.village.merchant"),

common/src/comp/inventory/loadout_builder.rs

@@ -24,17 +24,14 @@ use tracing::warn;
 /// `ItemConfig`
 ///
 /// ```
-/// use veloren_common::{
+/// use veloren_common::{comp::Item, LoadoutBuilder};
-///     assets::AssetExt,
-///     comp::item::tool::AbilityMap,
-///     comp::Item,
-///     LoadoutBuilder,
-/// };
 ///
-/// // Build a loadout with character starter defaults and a specific sword with default sword abilities
+/// // Build a loadout with character starter defaults
-/// let loadout = LoadoutBuilder::new()
+/// // and a specific sword with default sword abilities
+/// let sword = Item::new_from_asset_expect("common.items.weapons.sword.steel-8");
+/// let loadout = LoadoutBuilder::empty()
 ///     .defaults()
-///     .active_mainhand(Some(Item::new_from_asset_expect("common.items.weapons.sword.steel-8")))
+///     .active_mainhand(Some(sword))
 ///     .build();
 /// ```
 #[derive(Clone)]
@@ -362,13 +359,9 @@ fn default_main_tool(body: &Body) -> Item {
     maybe_tool.unwrap_or_else(Item::empty)
 }
 
-impl Default for LoadoutBuilder {
-    fn default() -> Self { Self::new() }
-}
-
 impl LoadoutBuilder {
     #[must_use]
-    pub fn new() -> Self { Self(Loadout::new_empty()) }
+    pub fn empty() -> Self { Self(Loadout::new_empty()) }
 
     #[must_use]
     /// Construct new `LoadoutBuilder` from `asset_specifier`
@@ -376,7 +369,7 @@ impl LoadoutBuilder {
     pub fn from_asset_expect(asset_specifier: &str, rng: Option<&mut impl Rng>) -> Self {
         // It's impossible to use lambdas because `loadout` is used by value
         #![allow(clippy::option_if_let_else)]
-        let loadout = Self::new();
+        let loadout = Self::empty();
 
         if let Some(rng) = rng {
             loadout.with_asset_expect(asset_specifier, rng)
@@ -392,7 +385,7 @@ impl LoadoutBuilder {
     /// NOTE: make sure that you check what is default for this body
     /// Use it if you don't care much about it, for example in "/spawn" command
     pub fn from_default(body: &Body) -> Self {
-        let loadout = Self::new();
+        let loadout = Self::empty();
         loadout
             .with_default_maintool(body)
             .with_default_equipment(body)
@@ -691,7 +684,7 @@ mod tests {
     #[test]
     fn test_loadout_presets() {
         for preset in Preset::iter() {
-            std::mem::drop(LoadoutBuilder::default().with_preset(preset));
+            std::mem::drop(LoadoutBuilder::empty().with_preset(preset));
         }
     }
 

common/src/comp/inventory/mod.rs

@@ -81,7 +81,7 @@ impl InventorySortOrder {
 /// that contains items etc) must first ensure items are unloaded from the item.
 /// This is handled in `inventory\slot.rs`
 impl Inventory {
-    pub fn new_empty() -> Inventory { Self::new_with_loadout(LoadoutBuilder::new().build()) }
+    pub fn new_empty() -> Inventory { Self::new_with_loadout(LoadoutBuilder::empty().build()) }
 
     pub fn new_with_loadout(loadout: Loadout) -> Inventory {
         Inventory {

common/src/comp/inventory/test.rs

@@ -21,7 +21,7 @@ fn push_full() {
             .iter()
             .map(|a| Some(a.duplicate(ability_map, msm)))
             .collect(),
-        loadout: LoadoutBuilder::new().build(),
+        loadout: LoadoutBuilder::empty().build(),
     };
     assert_eq!(
         inv.push(TEST_ITEMS[0].duplicate(ability_map, msm))
@@ -41,7 +41,7 @@ fn push_all_full() {
             .iter()
             .map(|a| Some(a.duplicate(ability_map, msm)))
             .collect(),
-        loadout: LoadoutBuilder::new().build(),
+        loadout: LoadoutBuilder::empty().build(),
     };
     let Error::Full(leftovers) = inv
         .push_all(
@@ -71,7 +71,7 @@ fn push_unique_all_full() {
             .iter()
             .map(|a| Some(a.duplicate(ability_map, msm)))
             .collect(),
-        loadout: LoadoutBuilder::new().build(),
+        loadout: LoadoutBuilder::empty().build(),
     };
     inv.push_all_unique(
         TEST_ITEMS
@@ -90,7 +90,7 @@ fn push_all_empty() {
     let mut inv = Inventory {
         next_sort_order: InventorySortOrder::Name,
         slots: vec![None, None],
-        loadout: LoadoutBuilder::new().build(),
+        loadout: LoadoutBuilder::empty().build(),
     };
     inv.push_all(
         TEST_ITEMS
@@ -109,7 +109,7 @@ fn push_all_unique_empty() {
     let mut inv = Inventory {
         next_sort_order: InventorySortOrder::Name,
         slots: vec![None, None],
-        loadout: LoadoutBuilder::new().build(),
+        loadout: LoadoutBuilder::empty().build(),
     };
     inv.push_all_unique(
         TEST_ITEMS

common/src/generation.rs

@@ -57,14 +57,73 @@ enum Meta {
     SkillSetAsset(String),
 }
 
+// FIXME: currently this is used for both base definition
+// and extension manifest.
+// This is why all fields have Uninit kind which is means
+// that this field should be either Default or Unchanged
+// depending on how it is used.
+//
+// When we will use exension manifests more, it would be nicer to
+// split EntityBase and EntityExtension to different structs.
+//
+// Fields which have Uninit enum kind
+// should be optional (or renamed to Unchanged) in EntityExtension
+// and required (or renamed to Default) in EntityBase
+/// Struct for EntityInfo manifest.
+///
+/// Intended to use with .ron files as base definion or
+/// in rare cases as extension manifest.
+/// Check assets/common/entity/template.ron or other examples.
+///
+/// # Example
+/// ```
+/// use vek::Vec3;
+/// use veloren_common::generation::EntityInfo;
+///
+/// // create new EntityInfo at dummy position
+/// // and fill it with template config
+/// let dummy_position = Vec3::new(0.0, 0.0, 0.0);
+/// let entity = EntityInfo::at(dummy_position).with_asset_expect("common.entity.template");
+/// ```
 #[derive(Debug, Deserialize, Clone)]
 pub struct EntityConfig {
+    /// Name of Entity
+    /// Can be Name(String) with given name
+    /// or Automatic which will call automatic name depend on Body
+    /// or Uninit (means it should be specified somewhere in code)
     name: NameKind,
+
+    /// Body
+    /// Can be Exact (Body with all fields e.g BodyType, Species, Hair color and
+    /// such) or RandomWith (will generate random body or species)
+    /// or Uninit (means it should be specified somewhere in code)
     body: BodyBuilder,
+
+    /// Alignment, can be Uninit
     alignment: AlignmentMark,
+
+    /// Loot
+    /// Can be Item (with asset_specifier for item)
+    /// or LootTable (with asset_specifier for loot table)
+    /// or Uninit (means it should be specified something in the code)
     loot: LootKind,
+
+    /// Hands:
+    /// - TwoHanded(ItemSpec) for one 2h or 1h weapon,
+    /// - Paired(ItemSpec) for two 1h weapons aka berserker mode,
+    /// - Mix { mainhand: ItemSpec, offhand: ItemSpec,
+    /// } for two different 1h weapons,
+    /// - Uninit which means that tool should be specified somewhere in code,
+    /// Where ItemSpec is taken from loadout_builder module
+    // TODO: better name for this?
+    // wielding? equipped? what do you think Tigers are wielding?
+    // should we use this field for animals without visible weapons at all?
     hands: Hands,
-    // Meta fields
+
+    /// Meta Info for optional fields
+    /// Possible fields:
+    /// LoadoutAsset(String) with asset_specifier for loadout
+    /// SkillSetAsset(String) with asset_specifier for skillset
     meta: Vec<Meta>,
 }
 
@@ -476,7 +535,7 @@ mod tests {
             match field {
                 Meta::LoadoutAsset(asset) => {
                     let rng = &mut rand::thread_rng();
-                    let builder = LoadoutBuilder::default();
+                    let builder = LoadoutBuilder::empty();
                     // we need to just load it check if it exists,
                     // because all loadouts are tested in LoadoutBuilder module
                     std::mem::drop(builder.with_asset_expect(&asset, rng));

common/src/states/basic_summon.rs

@@ -91,7 +91,7 @@ impl CharacterBehavior for Data {
 
                         let loadout = {
                             let loadout_builder =
-                                LoadoutBuilder::new().with_default_maintool(&body);
+                                LoadoutBuilder::empty().with_default_maintool(&body);
                             // If preset is none, use default equipment
                             if let Some(preset) = loadout_config {
                                 loadout_builder.with_preset(preset).build()

server/src/character_creator.rs

@@ -36,7 +36,7 @@ pub fn create_character(
     let stats = Stats::new(character_alias.to_string());
     let skill_set = SkillSet::default();
 
-    let loadout = LoadoutBuilder::new()
+    let loadout = LoadoutBuilder::empty()
         .defaults()
         .active_mainhand(Some(Item::new_from_asset_expect(&tool_id)))
         .build();

server/src/persistence/character/conversions.rs

@@ -361,7 +361,7 @@ pub fn convert_loadout_from_database_items(
     loadout_container_id: i64,
     database_items: &[Item],
 ) -> Result<Loadout, PersistenceError> {
-    let loadout_builder = LoadoutBuilder::new();
+    let loadout_builder = LoadoutBuilder::empty();
     let mut loadout = loadout_builder.build();
     let mut item_indices = HashMap::new();
 

server/src/rtsim/tick.rs

@@ -119,7 +119,7 @@ impl<'a> System<'a> for Sys {
             // TODO: this should be a bit more intelligent
             let loadout = match body {
                 comp::Body::Humanoid(_) => entity.get_loadout(),
-                _ => LoadoutBuilder::new().with_default_maintool(&body).build(),
+                _ => LoadoutBuilder::empty().with_default_maintool(&body).build(),
             };
 
             let event = match body {

server/src/sys/terrain.rs

@@ -230,7 +230,7 @@ impl<'a> System<'a> for Sys {
                 };
 
                 let loadout = {
-                    let mut loadout_builder = LoadoutBuilder::new();
+                    let mut loadout_builder = LoadoutBuilder::empty();
                     let rng = &mut rand::thread_rng();
 
                     // If main tool is passed, use it. Otherwise fallback to default tool

voxygen/src/audio/sfx/event_mapper/combat/tests.rs

@@ -12,7 +12,7 @@ use std::time::{Duration, Instant};
 
 #[test]
 fn maps_wield_while_equipping() {
-    let loadout = LoadoutBuilder::new()
+    let loadout = LoadoutBuilder::empty()
         .active_mainhand(Some(Item::new_from_asset_expect(
             "common.items.weapons.axe.starter_axe",
         )))
@@ -39,7 +39,7 @@ fn maps_wield_while_equipping() {
 
 #[test]
 fn maps_unwield() {
-    let loadout = LoadoutBuilder::new()
+    let loadout = LoadoutBuilder::empty()
         .active_mainhand(Some(Item::new_from_asset_expect(
             "common.items.weapons.bow.starter",
         )))
@@ -61,7 +61,7 @@ fn maps_unwield() {
 
 #[test]
 fn maps_basic_melee() {
-    let loadout = LoadoutBuilder::new()
+    let loadout = LoadoutBuilder::empty()
         .active_mainhand(Some(Item::new_from_asset_expect(
             "common.items.weapons.axe.starter_axe",
         )))
@@ -106,7 +106,7 @@ fn maps_basic_melee() {
 
 #[test]
 fn matches_ability_stage() {
-    let loadout = LoadoutBuilder::new()
+    let loadout = LoadoutBuilder::empty()
         .active_mainhand(Some(Item::new_from_asset_expect(
             "common.items.weapons.sword.starter",
         )))
@@ -168,7 +168,7 @@ fn matches_ability_stage() {
 
 #[test]
 fn ignores_different_ability_stage() {
-    let loadout = LoadoutBuilder::new()
+    let loadout = LoadoutBuilder::empty()
         .active_mainhand(Some(Item::new_from_asset_expect(
             "common.items.weapons.axe.starter_axe",
         )))

voxygen/src/menu/char_selection/ui/mod.rs

@@ -180,7 +180,7 @@ impl Mode {
     pub fn create(name: String) -> Self {
         let tool = STARTER_SWORD;
 
-        let loadout = LoadoutBuilder::new()
+        let loadout = LoadoutBuilder::empty()
             .defaults()
             .active_mainhand(Some(Item::new_from_asset_expect(tool)))
             .build();