Search Unity

Question How to keep clean code with linked classes?

Discussion in 'Scripting' started by Magnilum, Feb 24, 2024.

  1. Magnilum

    Magnilum

    Joined:
    Jul 1, 2019
    Posts:
    241
    Hi everyone,

    Since a while, I have tried to figure out how to keep my code clean and easy to change with many linked classes. Here is my problem.

    I have an InteractionController class, a Player class, an Inventory class and an Equipment class.

    What I would like to do is, the InteractionController trigger an intraction like a pickup item on ground. This is send to the player to add it to its inventory. Before adding it, he must check if it won't be too heavy for him to carry this new item or these new items. So he asks for the inventory its total weight and the same for the equiment. If the addition of the current weight of both and the new items is under his limit, he adds it to its inventory.

    My question here is, I have seen both of these videos Connecting scripts on the same object in Unity & Breaking up Code in Unity (Important game dev tips for beginners) and many others.

    They look pretty good, I tried to replicate the second one by breaking up my player into fewer scripts like the InteractionController, Inventory, Equipment which are all Monobehaviour classes.

    But with what I want to do, it seems that I must write a new function into the player class to add an item to its inventory and make all the validation steps to see if it is possible or not.

    Here is a quick overview of what I have in mind, does it seem correct to you. I am scared of using this method because of the spaghetti code mentioned many times. I really would like to get your opinion on this.

    Code (CSharp):
    1. public class InteractionController : MonoBehaviour
    2. {
    3.     Player player;
    4.  
    5.     public void Interact(GroundObject groundObject)
    6.     {
    7.         player.AddToInventory(groundObject.Item, groundObject.Amount);
    8.     }
    9. }
    10.  
    11. public class Player : MonoBehaviour
    12. {
    13.     float maxWeight;
    14.     Inventory inventory;
    15.     Equipment equipment;
    16.  
    17.     public void AddToInventory(Item item, int amount)
    18.     {
    19.         if (item.Weight * amount + inventory.GetTotalWeight() + equipment.GetTotalWeight() < maxWeight)
    20.             inventory.AddToInventory(item, amount);
    21.     }
    22. }
    23.  
    24. public class Inventory : MonoBehaviour
    25. {
    26.     public void AddToInventory(Item item, int amount)
    27.     {
    28.  
    29.     }
    30.  
    31.     public float GetTotalWeight()
    32.     {
    33.         return totalWeight;
    34.     }
    35. }
    36.  
    37. public class Equipment : MonoBehaviour
    38. {
    39.     public float GetTotalWeight()
    40.     {
    41.         return totalWeight;
    42.     }
    43. }
     
  2. SisusCo

    SisusCo

    Joined:
    Jan 29, 2019
    Posts:
    1,321
    Consider creating a simple, reusable abstraction like IInteractable, and writing the InteractionController to work around this in a general manner, without knowing anything about GroundObject in specific. GroundObject could then just be one of many implementations of the IInteractable abstraction, and new ones could be added to the game without having to always go add more and more methods to InteractionController.

    For example, something like this:
    Code (CSharp):
    1. // contract that any interactable component must fulfill
    2. public interface IInteractable
    3. {
    4.     bool CanInteract(Player player);
    5.     void Interact(Player player);
    6. }
    7.  
    8. public sealed class GroundObject : MonoBehaviour, IInteractable
    9. {
    10.     [SerializeField] Item item;
    11.     [SerializeField] int amount;
    12.  
    13.     // GroundObject fulfills the contract
    14.     public bool CanInteract(Player player) => item.Weight * amount + player.Inventory.GetTotalWeight() < player.Stats.MaxCarryWeight;
    15.     public void Interact(Player player) => player.Inventory.Add(item, amount);
    16. }
    17.  
    18. public sealed class InteractionController : MonoBehaviour
    19. {
    20.     IInteractable activeInteractable;
    21.  
    22.     void OnInteractionInputGiven()
    23.     {
    24.         //  InteractionController can work with any interactable, knows nothing about GroundObject
    25.         if(activeInteractable != null && activeInteractable.CanInteract(player))
    26.         {
    27.             activeInteractable.Interact(player);
    28.         }
    29.     }
    30. }
    31.  
    32. // Player could contain little to no logic, just references to other objects related to the player,
    33. // to avoid it becoming a bloated god object with too many responsibilities.
    34. public sealed class Player : MonoBehaviour
    35. {
    36.     [SerializeField] Inventory inventory;
    37.     [SerializeField] Stats stats;
    38.  
    39.     public Inventory Inventory => inventory;
    40.     public Stats Stats => stats;
    41. }
    42.  
    43. [CreateAssetMenu]
    44. public sealed class Stats : ScriptableObject
    45. {
    46.     [SerializeField] float maxWeight;
    47.  
    48.     public float MaxCarryWeight => maxWeight;
    49. }
    The key to making scalable systems that are easy to work with, is usually having the ability to extend them with new behaviour, without having to modify the existing classes (open-closed principle).
     
    Last edited: Feb 24, 2024
    angrypenguin, CodeRonnie and Nad_B like this.
  3. Magnilum

    Magnilum

    Joined:
    Jul 1, 2019
    Posts:
    241
    Thank you for your answer !!!

    I did not write/explained it but it is what I have done for the GroundObject class by adding an interface. The only thing I didn't do was to put the player as a parameter, which is dumb so I added it, thanks ;).

    I have 2 questions, for your exemple, you only check for the inventory weight but I would like to check for the Inventory and Equipment so I guess what I can do is the following:

    Code (CSharp):
    1. public interface IInteractable
    2. {
    3.     bool CanInteract(Player player);
    4.     void Interact(Player player);
    5. }
    6.  
    7. public sealed class GroundObject : MonoBehaviour, IInteractable
    8. {
    9.     [SerializeField] Item item;
    10.     [SerializeField] int amount;
    11.  
    12.     public bool CanInteract(Player player) => player.CanAdd(item, amount);
    13.     public void Interact(Player player) => player.Inventory.Add(item, amount);
    14. }
    15.  
    16. [CreateAssetMenu]
    17. public sealed class Stats : ScriptableObject
    18. {
    19.     [SerializeField] float maxWeight;
    20.  
    21.     public float MaxCarryWeight => maxWeight;
    22. }
    23.  
    24. public sealed class Player : MonoBehaviour
    25. {
    26.     [SerializeField] Stats stats;
    27.     [SerializeField] Inventory inventory;
    28.     [SerializeField] Equipment equipment;
    29.  
    30.     public Inventory Inventory => inventory;
    31.     public Stats Stats => stats;
    32.  
    33.     public bool CanAdd(Item item, int amount)
    34.     {
    35.         return item.Weight * amount + inventory.GetTotalWeight() + equipment.GetTotalWeight() < stats.MaxCarryWeight;
    36.     }
    37. }
    38.  
    39. public class Equipment : MonoBehaviour
    40. {
    41.  
    42. }
    43.  
    44. public class Inventory : MonoBehaviour
    45. {
    46.     public void Add(Item item, int amount)
    47.     {
    48.         // Adding it to the inventory
    49.     }
    50. }
    The second question is about the IInteractable which has 2 methods, Interact which is OK to me and CanInteract and I am asking if is it really necessary? It is not possible to set it directly into the player like this?


    Code (CSharp):
    1. public interface IInteractable
    2. {
    3.     void Interact(Player player);
    4. }
    5.  
    6. public sealed class GroundObject : MonoBehaviour, IInteractable
    7. {
    8.     [SerializeField] Item item;
    9.     [SerializeField] int amount;
    10.  
    11.     public void Interact(Player player) => player.AddToInventory(item, amount);
    12. }
    13.  
    14. [CreateAssetMenu]
    15. public sealed class Stats : ScriptableObject
    16. {
    17.     [SerializeField] float maxWeight;
    18.  
    19.     public float MaxCarryWeight => maxWeight;
    20. }
    21.  
    22. public sealed class Player : MonoBehaviour
    23. {
    24.     [SerializeField] Stats stats;
    25.     [SerializeField] Inventory inventory;
    26.     [SerializeField] Equipment equipment;
    27.  
    28.     public Inventory Inventory => inventory;
    29.     public Stats Stats => stats;
    30.  
    31.     public bool AddToInventory(Item item, int amount)
    32.     {
    33.         if (CanAdd(item, amount))
    34.         {
    35.             inventory.Add(item, amount);
    36.             return true;
    37.         }
    38.  
    39.         return false;
    40.     }
    41.  
    42.     public bool CanAdd(Item item, int amount)
    43.     {
    44.         return item.Weight * amount + inventory.GetTotalWeight() + equipment.GetTotalWeight() < stats.MaxCarryWeight;
    45.     }
    46. }
    47.  
    48. public class Equipment : MonoBehaviour
    49. {
    50.  
    51. }
    52.  
    53. public class Inventory : MonoBehaviour
    54. {
    55.     public void Add(Item item, int amount)
    56.     {
    57.         // Adding it to the inventory
    58.     }
    59. }
    Which really looks like what I wrote at first, isn't it?
     
  4. SisusCo

    SisusCo

    Joined:
    Jan 29, 2019
    Posts:
    1,321
    I would remove the Player.CanAdd method, and move the logic into GroundObject.CanInteract, to avoid the Player class getting bloated over time. Just add a Player.Equipment property, and then the GroundObject can pull all the information it needs and determine if the interaction is possible.

    What the optimal abstraction is depends on the needs of your game. If for example you want to grey out an icon in your UI when an interaction isn't possible, then you need to able to determine whether or not an interaction is possible, without also triggering it.

    Simplicity is great; the simpler the interface can be made, the better! But like Mr. Einstein said, "Make things as simple as possible, but no simpler."
     
    angrypenguin and Nad_B like this.
  5. SisusCo

    SisusCo

    Joined:
    Jan 29, 2019
    Posts:
    1,321
    To further clarify why I think it's bad to centralize all the logic in Player, is that I think it can easily become a pain point once the number of interactions in the game increases. With just a few additional interactions, the number of methods in the Player class could already grow really long:
    Code (CSharp):
    1. Player
    2. {
    3.     AddToInventory
    4.     CanAdd
    5.     Attack
    6.     CanAttack
    7.     Climb
    8.     CanClimb
    9.     Enter
    10.     CanEnter
    11.     Sit
    12.     CanSit
    13.     EquipGear
    14.     CanEquipGear
    15.     UnequipGear
    16.     CanUnequipGear
    17.     LevelUp
    18.     IncreaseStat
    19. }
    When all this code is instead spread out to all the different interactable objects, the project can continue to be easily readable and extendable even when there are dozens of interactables.
     
    Bunny83 and Nad_B like this.
  6. Magnilum

    Magnilum

    Joined:
    Jul 1, 2019
    Posts:
    241
    I totally agree with that and that is what I would like to avoid having everything inside my player script.

    What I am scared about with the spread of the logic into differents scripts is repetition. It means that before adding something to the inventory, each time I will need to check. If I create a new script for a merchant, for example, which adds items to the player, I need to write twice the same code:

    Code (CSharp):
    1. // contract that any interactable component must fulfill
    2. public interface IInteractable
    3. {
    4.     bool CanInteract(Player player);
    5.     void Interact(Player player);
    6. }
    7.  
    8. public sealed class GroundObject : MonoBehaviour, IInteractable
    9. {
    10.     [SerializeField] Item item;
    11.     [SerializeField] int amount;
    12.  
    13.     // GroundObject fulfills the contract
    14.     public bool CanInteract(Player player)
    15.     {
    16.         return item.Weight * amount + player.Inventory.GetTotalWeight() + player.Equipment.GetTotalWeight() < player.Stats.MaxCarryWeight;
    17.     }
    18.     public void Interact(Player player)
    19.     {
    20.         player.Inventory.Add(item, amount);
    21.     }
    22. }
    23.  
    24. public sealed class Merchant : MonoBehaviour
    25. {
    26.     Item[] items;
    27.     int[] amounts;
    28.  
    29.     public void CanSell(Player player)
    30.     {
    31.         return items[0].Weight * amounts[0] + player.Inventory.GetTotalWeight() + player.Equipment.GetTotalWeight() < player.Stats.MaxCarryWeight;
    32.     }
    33.  
    34.     public void Sell(Player player)
    35.     {
    36.         player.Inventory.Add(items[0].Weight, amounts[0]);
    37.     }
    38. }
    39.  
    40. public sealed class InteractionController : MonoBehaviour
    41. {
    42.     void OnInteractionInputGiven(IInteractable activeInteractable)
    43.     {
    44.         if (activeInteractable == null) return;
    45.  
    46.         //  InteractionController can work with any interactable, knows nothing about GroundObject
    47.         if (activeInteractable.CanInteract(player))
    48.             activeInteractable.Interact(player);
    49.     }
    50. }
    51.  
    52. [CreateAssetMenu]
    53. public sealed class Stats : ScriptableObject
    54. {
    55.     [SerializeField] float maxWeight;
    56.  
    57.     public float MaxCarryWeight => maxWeight;
    58. }
    59.  
    60. // Player could contain little to no logic, just references to other objects related to the player,
    61. // to avoid it becoming a bloated god object with too many responsibilities.
    62. public sealed class Player : MonoBehaviour
    63. {
    64.     [SerializeField] Stats stats;
    65.     [SerializeField] Inventory inventory;
    66.     [SerializeField] Equipment equipment;
    67.  
    68.     public Stats Stats => stats;
    69.     public Inventory Inventory => inventory;
    70.     public Equipment Equipment => equipment;
    71. }
    72.  
    73. public class Equipment
    74. {
    75.  
    76. }
    77.  
    78. public class Inventory
    79. {
    80.  
    81. }
    I really like the fact that each interaction is looking for its capability which seems pretty good and as you said everything is spread out among different scripts. But it is a trade with rewrite many times the same thing.

    Or may be there is a better way to do what I have done (I guess yes).
     
  7. SisusCo

    SisusCo

    Joined:
    Jan 29, 2019
    Posts:
    1,321
    The Inventory itself could definitely contain a
    bool TryAdd
    type method instead of
    void Add
    . Trying to design the public API of your classes in such a way that exceptions are impossible to occur is great practice.

    As I envisioned it, IInteractable would only be used by the InteractionController. All other classes that need to interact with the Inventory for example, could just call its methods directly, without doing it through the interactable classes. So the public API for Inventory and IInteractable could be very different.

    That is a good point.

    If you find yourself in a situation, where two different classes need to check exactly the same thing, then it is usually a good idea to centralize that logic in one place, and have both classes use the same code to calculate the result. This ensures that if you make modifications to the logic in the future, all the relevant systems will be automatically updated to reflect the change and can't go out-of-sync (single source of truth).

    However, sometimes repeating identical code in multiple places isn't a bad thing. There is a risk when multiple classes make use of a shared method, that when changes are made to this shared method for the benefit of one class, logic in any of the other classes could break. When there's this sort of tight coupling between pretty unrelated classes, it can make it more difficult to make modifications to any of the classes without introducing bugs, and your project can become more fragile.

    So it's important to differentiate between classes that just happen to have some identical logic at this moment, but they could have slightly different logic in the future, and classes that should inherently always have exactly the same logic, and you want them to always remain synchronized.

    It's not always easy to spot the difference, but it's good to keep in mind that code repetition is not always inherently bad, and can sometimes it's much better than the alternative.


    As for placing methods related to inventory management into the Player class, it kind of makes sense in that it has easy access to all the relevant classes... but it just feels wrong to me in terms of cohesion. It feels like things related to adding items should rather be part of the API of the Inventory, not the Player class. A method related to comparing the weight of an Inventory, Equipment and an Item to a maximum carry weight feels like it could be part of the Item's public API, or a method in a static utility class ItemUtilility, but not in Player.

    You could consider merging Inventory and Equipment into one class, if it feels like they are very tightly coupled together, and any methods you add into one needs to reference things in the other one. Then you could just use Inventory.GetTotalWeight() instead of Inventory.GetTotalWeight() + Equipment.GetTotalWeight() everywhere. Splitting classes into smaller ones isn't always a good idea either, if it just ends up increasing overall complexity in the project.

    You could also add a new struct like WeightLimit, and use that in Stats instead of a primitive float. That struct could then contain a method like
    WeightLimit.IsExceededBy(params[] float[] weights)
    , which both Merchant and GroundObject could make use of.

    But if it really feels like the best place to put those couple of methods is the Player class, then just go for it. You know your project best. And you can always refactor things later on, once your project has more code in it, and you have a better idea of where everything fits in naturally. You don't want to get paralyzed with overanalyzing things either :)
     
    Last edited: Feb 24, 2024
    Bunny83 likes this.
  8. Bunny83

    Bunny83

    Joined:
    Oct 18, 2010
    Posts:
    3,976
    I'd like to add that it should never be the responsibility of a ground item or a merchant to check if the one that is about to receive an item has enough capacity to do so. All this should be handlled by the player or the inventory. There are several ways how this could be abstracted. The Merchant and the ground object could just interact with the inventory. That way even NPCs or other things could interact with those entities in the same way. In that case the interface may have a callback to check if the item can be added (besides it's own checks). So the player object who "owns" the inventory could provide a method and pass that to the inventory to do the weight check when you use the Add / TryAdd of the inventory.

    Other objects should be agnostic about what that other thing is they interact with. Restrictions and rules that apply to an object should be enforced by that object and not by others.
     
    angrypenguin and SisusCo like this.
  9. Magnilum

    Magnilum

    Joined:
    Jul 1, 2019
    Posts:
    241
    Thanks for your answer! So it seems closer to what I first thought right?

    So what I understand is having a duplicated function of AddToInventory in the Player class and make the check steps inside.

    I am very curious of this part and you have any example as piece of code in mind, could you write it cause I struggle to image it.
     
  10. SisusCo

    SisusCo

    Joined:
    Jan 29, 2019
    Posts:
    1,321
    That is a good point... I do agree, it'd be unintuitive if the thing that has a max capacity didn't itself enforce it in any way.

    And if the maximum carry capacity is an attribute of the Player object, it does make sense that it is the Player class that has the logic about how much it can carry. The Inventory itself has theoretically infinite capacity, it's only when the Player object tries to carry that Inventory around, when the weight limit comes in.

    It would be a different story, if it was the Inventory itself that had a maximum capacity, like a Backpack that just can't fit more than a certain number of items, then the Player class could very well be left out of the equation.

    So yeah, maybe adding methods like
    bool CanTakeItem(Item item)
    and
    bool TakeItem(Item item)
    directly to the Player class makes sense after all.

    Then the method could even take into consideration factors other than just weight capacity in the future, like if the player is holding something in its hands and can't take items because of this, or has frozen status effect or whatever.
     
  11. Magnilum

    Magnilum

    Joined:
    Jul 1, 2019
    Posts:
    241
    Actually, I called it inventory but in reality, it is a backpack with a limited size. I did not want to mention it cause I did not want to provide more information than it was already. Also, it was not important to mention since the problem was still the same, my player is constrained by its max weight and if it is ok for him and he can carry the new load, then try to add it to the backpack if there is room for it.

    So what I get from all of this is to go with the following:

    Code (CSharp):
    1. public interface IInteractable
    2. {
    3.     void Interact(Player player);
    4. }
    5.  
    6. public sealed class GroundObject : MonoBehaviour, IInteractable
    7. {
    8.     [SerializeField] Item item;
    9.     [SerializeField] int amount;
    10.     public void Interact(Player player)
    11.     {
    12.         if (player.AddToInventory(item, amount))
    13.             Destroy(gameObject);
    14.     }
    15. }
    16.  
    17. [CreateAssetMenu]
    18. public sealed class Stats : ScriptableObject
    19. {
    20.     [SerializeField] float maxWeight;
    21.     public float MaxCarryWeight => maxWeight;
    22. }
    23.  
    24. public sealed class Player : MonoBehaviour
    25. {
    26.     [SerializeField] Stats stats;
    27.     [SerializeField] Inventory inventory;
    28.     [SerializeField] Equipment equipment;
    29.     public Inventory Inventory => inventory;
    30.     public Stats Stats => stats;
    31.     public bool AddToInventory(Item item, int amount)
    32.     {
    33.         if (CanAddToInventory(item, amount))
    34.             return inventory.Add(item, amount)
    35.         return false;
    36.     }
    37.     bool CanAddToInventory(Item item, int amount)
    38.     {
    39.         return item.Weight * amount + inventory.GetTotalWeight() + equipment.GetTotalWeight() < stats.MaxCarryWeight;
    40.     }
    41. }
    42.  
    43. public class Equipment : MonoBehaviour
    44. {
    45.     public float GetTotalWeight() => totalWeight;
    46. }
    47.  
    48. public class Inventory : MonoBehaviour
    49. {
    50.     [SerializeField] float maxSize;
    51.  
    52.     public bool Add(Item item, int amount)
    53.     {
    54.         if (CanAdd(item, amount))
    55.         {
    56.             // Adding it to the inventory
    57.             return true;
    58.         }
    59.         return false;
    60.     }
    61.  
    62.     bool CanAdd(Item item, int amount)
    63.     {
    64.         return item.Size * amount <= maxSize;
    65.     }
    66.  
    67.     public float GetTotalWeight() => totalWeight;
    68. }
     
  12. SisusCo

    SisusCo

    Joined:
    Jan 29, 2019
    Posts:
    1,321
    Looks good to me! The only thing I'd change is to have Player.CanAddToInventory internally check that Inventory.CanAdd is also true. Otherwise it seems to me that the method is lying. Or if not that, then at least rename the method to something more accurate like CanCarry :)
     
  13. Magnilum

    Magnilum

    Joined:
    Jul 1, 2019
    Posts:
    241
    Ok I see and that is right, it is more CanCarry than CanAddToInventory for the Player.

    Also, what about you wrote in your previous post:

    Because, with this method, I am going to reach this point with a Player who has pretty much all the methods that his Equiment, Inventory ... have.
     
  14. SisusCo

    SisusCo

    Joined:
    Jan 29, 2019
    Posts:
    1,321
    Yep... having an abstraction like Player in a project very easily leads down this path.

    Avoiding this really boils down to coming up with alternative abstractions that encapsulate everything that is needed for a particular interaction.

    As a thought exercise, you can try to imagine if you had a blank project, with no concept of a Player class, and you had to only write the bare minimum amount of classes needed to handle a particular interaction.

    Following this line of thinking, interactions like "Pick Up Ground Item" and "Equip Inventory Item" could also be handled by a class like this:
    Code (CSharp):
    1. //or InventoryController, InventoryHandler, PlayerInventory...
    2. public class Inventory : MonoBehaviour
    3. {
    4.     [SerializeField] InventoryStats stats;
    5.     [SerializeField] Equipment equipment;
    6.     [SerializeField] Backpack backpack;
    7.  
    8.     public float GetTotalWeight() => equipment.GetTotalWeight() + backpack.GetTotalWeight();
    9.     public float GetMaxWeight() => stats.MaxCarryWeight;
    10.  
    11.     public bool CanAddToBackpack(Item item, int amount) { ... }
    12.     public bool AddToBackpack(Item item, int amount) { ... }
    13.     public bool Equip(EquippableItem item) { ... }
    14.     public bool Unequip(EquippableItem item) { ... }
    15. }
    Similarly you could have classes like ClimbHandler and CombatController, instead of the Player class, be responsible for handling climb and attack interactions respectively. And so on.
     
  15. Magnilum

    Magnilum

    Joined:
    Jul 1, 2019
    Posts:
    241
    Ok so the goal is to encapsulate these methods with a new class if I have correclty understood.

    So in my case, having a Player class which refer to an Inventory class which contains the Backpack class and the Equipment class, right? I guess it seems logic. It means that to start I can write everithing inside the player class and when it stars to get messy, create new classes to encapsulate some methods.
     
    SisusCo likes this.
  16. SisusCo

    SisusCo

    Joined:
    Jan 29, 2019
    Posts:
    1,321
    Yeah that. Or you could use GetComponent instead of fetching all references through a Player class, if you want to have the interaction system work completely independently from the Player:
    Code (CSharp):
    1. public sealed class InteractionController : MonoBehaviour
    2. {
    3.     void OnInteractionInputGiven() => activeInteractable?.Interact(gameObject))
    4. }
    5.  
    6. public abstract class Interactable<TInteractionHandler> : MonoBehaviour, IInteractable where TInteractionHandler : Component
    7. {
    8.     public void Interact(GameObject interactor)
    9.     {
    10.         if(TryGetHandler(interactor, out TInteractionHandler interactionHandler))
    11.         {
    12.             Interact(interactionHandler);
    13.         }
    14.     }
    15.  
    16.     protected abstract void Interact(TInteractionHandler interactionHandler);
    17.  
    18.     protected virtual bool TryGetHandler(GameObject interactor, out TInteractionHandler handler) => interactor.TryGetComponent(out handler);
    19. }
    20.  
    21. public sealed class GroundObject : Interactable<Inventory>
    22. {
    23.     protected override void Interact(Inventory inventory) => inventory.AddToBackpack(item, amount);
    24. }
    Both approaches should work just fine, in terms of avoiding the Player class becoming a huge god object.
     
    Last edited: Feb 25, 2024
    CodeRonnie and Magnilum like this.
  17. Magnilum

    Magnilum

    Joined:
    Jul 1, 2019
    Posts:
    241
    Ok I see, going with the encapsulate class directly without passing through the Player.

    That is a good idea, I have to check if it fits to what I would like to do but I am keeping it in mind.
     
    SisusCo likes this.
  18. tleylan

    tleylan

    Joined:
    Jun 17, 2020
    Posts:
    616
    Pardon my saying so but the code is needlessly complicated in many places. That makes it more likely that bugs can hide. It doesn't have to be "wrong" to be a source of problems.

    You can see examples of where you use properties and then switch to methods for essentially the same "sort" of data. equipment.GetTotalWeight() vs stats.MaxCarryWeight for instance.

    And Inventory can be substantially reduced (though I don't know where totalWeight comes from in that class.

    Code (CSharp):
    1. public class Inventory : MonoBehaviour
    2. {
    3.     [SerializeField]
    4.     private float _maxSize;
    5.  
    6.     public float TotalWeight => totalWeight;
    7.  
    8.     public bool CanAdd(Item item, int amount) => item.Size * amount <= _maxSize;
    9. }
    Then there is a case where the Player determines CanAddToIventory is true but Inventory.Add determines that it cannot.

    And what is the benefit of a private named maxWeight and a public named MaxCarryWeight? Wouldn't both being named similarly be an improvement?

    Code (CSharp):
    1. public sealed class Stats : ScriptableObject
    2. {
    3.     [SerializeField] float maxWeight;
    4.     public float MaxCarryWeight => maxWeight;
    5. }
    6.  
    .
     
  19. Magnilum

    Magnilum

    Joined:
    Jul 1, 2019
    Posts:
    241
    Not sure to understand since what you wrote is pretty much the same as the code below right?

    Or maybe you are talking about the following one?

     
  20. tleylan

    tleylan

    Joined:
    Jun 17, 2020
    Posts:
    616
    Not at all :) Check the example you posted for the Inventory class and compare it to what I posted. You have a private method that isn't needed at all for instance.

    In any case it isn't a matter of making code smaller but rather to make it more concise. It should do what it needs to do, no more and no less. More "stuff" generally doesn't improve the code but it can make it more likely to be misunderstood or to hide (or develop) conditional bugs.

    Again I'll just mention your intermixing of properties and methods for basic values. Stick to one style and you won't wonder if it is a method or not.
     
  21. Magnilum

    Magnilum

    Joined:
    Jul 1, 2019
    Posts:
    241
    Sorry but I really try to understand the difference between what you wrote and what I did:

    Code (CSharp):
    1. public class MyInventoryClass : MonoBehaviour
    2. {
    3.     [SerializeField] float maxSize;
    4.  
    5.     bool CanAdd(Item item, int amount)
    6.     {
    7.         return item.Size * amount <= maxSize;
    8.     }
    9.  
    10.     public float GetTotalWeight() => totalWeight;
    11.  
    12.     public bool Add(Item item, int amount)
    13.     {
    14.         if (CanAdd(item, amount))
    15.         {
    16.             // Adding it to the inventory
    17.             return true;
    18.         }
    19.         return false;
    20.     }
    21. }
    Code (CSharp):
    1. public class YourInventoryClass : MonoBehaviour
    2. {
    3.     [SerializeField]
    4.     private float _maxSize;
    5.     public float TotalWeight => totalWeight;
    6.     public bool CanAdd(Item item, int amount) => item.Size * amount <= _maxSize;
    7. }
    I really don't understand the difference.
     
  22. tleylan

    tleylan

    Joined:
    Jun 17, 2020
    Posts:
    616
    This is the subtlety of "good code" :) No difference seems hard to believe surely you can see differences. You have a GetTotalWeight() method, mine doesn't. You fluctuate between "Get" methods and accessing public properties. You can surely see you have a private CanAdd method that adds no value to the solution. It is private and only being used by your Add method. It can't be reused, you can't pass it other parameters, it serves no purpose. If you have an expressions totat = 2 + 5 then creating a private Total method that you pass 2 and 5 to only serves to obfuscate things.

    To be clear the values you pass to CanAdd are known to the Add method. The computation can be done in place and you lose nothing except pushing values onto a stack and popping off a Boolean.

    I see what is missing however. Your Add method has a comment about adding to the inventory. Fair enough I missed that. Note now there is a guard clause and an immediate return if the preconditions are not met.

    Code (CSharp):
    1. public bool Add(Item item, int amount)
    2. {
    3.     if(item.Size * amount > maxSize) return false;
    4.  
    5.     // Adding it to the inventory
    6.     return true;
    7. }
     
  23. angrypenguin

    angrypenguin

    Joined:
    Dec 29, 2011
    Posts:
    15,619
    Yeah, I would almost certainly be splitting that "Player" class up into multiple classes, but, as has already been said, some of this does depend on the particular use case, personal preference, and so on.

    Anyhow, the reason I'd be splitting it up is more or less the Single Responsibility Principle - every class should have one responsibility which is easily described. In my case, that would typically be something like "the player moves around the world and can interact with it". If I'm implementing a piece of functionality and it doesn't clearly fit into that description, I'm going to seriously consider whether it fits in that class.

    So, the function I want to implement is "Pick up an item and carry it for later use". Well, ok, that's actually two things! "Pick up an object..." yep, that's a direct and fairly simple interaction. But "...carry it for later use" has nothing to do with moving around and is not an interaction. It does not fit in my Player class, so I'll write an Inventory component. Its single responsibility? "To store a collection of Items with some simple constraints."

    Of course the details of that description will depend on your game, and how other parts of your code are designed. It's as much art as it is science. An in this case, I've not covered where or how the pick up action is actually implemented - many options there.

    The key thing about those "Single Responsibilities" is to avoid what's called a "monolithic" class, where all sorts of stuff is smashed together in one huge code file, as they tend to become messy, easy to make bugs in, hard to maintain, sources of repeat code, and bottlenecks in a team environment.
     
    SisusCo, Ryiah and spiney199 like this.
  24. SisusCo

    SisusCo

    Joined:
    Jan 29, 2019
    Posts:
    1,321
    If GetTotalWeight() really only returns the value of a private field, then I agree that using a property would make a lot of sense.

    If it actually dynamically calculates the value with each call by iterating through all items in the inventory, multiplying the amount and weight of each item together, and adding that to the total sum returned, then using a method would be much more appropriate in my opinion, given how costly the computation would be compared to just accessing a field.
     
  25. Magnilum

    Magnilum

    Joined:
    Jul 1, 2019
    Posts:
    241
    That was exactly my point, of course I wrote a simple return to a variable not to add too many details into the example code but the real method goes though the inventory and sum the amount and the weight for each slot. It is completely dynamic because I don't like to use a simple variable for this since it must be updated at each change.

    I did not want you guys to focus on this part but I am seeing that it did not work. Now I know I should write everything even if the it makes the example code a bit longer.
     
    SisusCo likes this.
  26. Magnilum

    Magnilum

    Joined:
    Jul 1, 2019
    Posts:
    241
    I don't really know what to think about all of that. I totally agree with the one responsability class which is what I am trying to do.

    In many videos, I saw that all should pass though the player. Like I am doing, I try to use the player as much I possible so adding to inventory make him check for him if he CanCarry or not which looks for the current inventory and equipment weight.

    I don't really like the idea a rewriting the same method name inside the Player to make all the checks and then add it to the inventory. But I don't see another solution for this since my player must check if he is able to carry this load.

    Code (CSharp):
    1. public class Player : Entity, IPickable
    2.     {
    3.         [Header("Player")]
    4.         [SerializeField] Equipment equipment;
    5.         [SerializeField] BagCollection bags;
    6.  
    7.         public bool Pickup(ObjectData objectData, int amount)
    8.         {
    9.             if (objectData is BagData bagData)
    10.                 return AddBag(bagData);
    11.             else if (objectData is ItemData itemData)
    12.                 return AddItem(itemData, amount);
    13.  
    14.             return false;
    15.         }
    16.  
    17.         public bool AddBag(BagData bagData)
    18.         {
    19.             return bags.AddBag(new Bag(bagData));
    20.         }
    21.  
    22.         public bool AddItem(ItemData itemData, int amount)
    23.         {
    24.             if (CanCarry(itemData, amount))
    25.                 return bags.AddItem(new Item(itemData), amount);
    26.             return false;
    27.         }
    28.  
    29.         bool CanCarry(ItemData itemData, int amount)
    30.         {
    31.             return itemData.Weight * amount + bags.GetTotalWeight() <= maxWeight.Value;
    32.         }
    33.     }
    Also, i am using a method for adding a bag instead of going directly with the bags.AddBag for 2 reasons:
    - The first one is only the fact that it is easy to understand, in this case, the Player can add a bag to itself and make it public so from anywhere in other classes I can add a new bag to the player or remove it (the method does't not exist yet).
    - The second reason is, if I want add a new condition to the AddBag like a maximum number a bags (as I would like to do), I can simply add the new if statement inside the method and that all.
    Easy to understand, manage and scale.

    Code (CSharp):
    1. public bool AddBag(BagData bagData)
    2. {
    3.     return bags.AddBag(new Bag(bagData));
    4. }
     
    Last edited: Feb 26, 2024
  27. Magnilum

    Magnilum

    Joined:
    Jul 1, 2019
    Posts:
    241
    If I understand it is not necessary to anticipate the use of a method which is going to check if the Inventory can add or cannot inside the class? It is better for now to leave it inside the Add method.

    May be it is wrong but when I write a new class, I am making a many function as possible to reuse them later if it is necessary. And I am adding Actions and other things to make them complete and being use later.
     
  28. angrypenguin

    angrypenguin

    Joined:
    Dec 29, 2011
    Posts:
    15,619
    If something is really irrelevant, instead of code put something like "// ..." or "// stuff" in it.

    Without knowing what videos, for context, it's hard to be specific.

    That said, there's a big difference between "can" and "should". It's straightforward and easy to understand, so it's a fair approach for materials aimed at people who still need how-to videos. Your thinking with questions like these is starting to head past that, though.

    Also, as I said, some of this is up to preference or taste... though I'd be wary of anyone saying what you "should" do without further context, pros and cons, etc.
     
    spiney199 and Magnilum like this.
  29. Magnilum

    Magnilum

    Joined:
    Jul 1, 2019
    Posts:
    241
    Right, I will do this for the next time.

    Before starting something like an inventory system or some movement system or something else, I like to document myself and see all the different approaches and select the one which fits the best to my case.

    It is been many years (with many interuptions) I am trying to make my code cleaner and easy to use/modify especially for the inventory system but even after writing it 5 times, I have never found solutions to my problems. I did it on my own but now I really want to make it work and do it in the best way. I don't want to get lost in my code again.

    I don't think being a begginner in Unity, but I am far from knowing all the good patterns to use in C#.

    That is why now I prefer to spend more time to ask and discuss with you guys about your ideas than writing directly what I have in mind.
     
    angrypenguin likes this.
  30. SisusCo

    SisusCo

    Joined:
    Jan 29, 2019
    Posts:
    1,321
    What were these problems you mention exactly?

    I think that taking a deep look at what specific things were pain points in your previous attempts is key in learning from that, and being able to make adjustments the next time to avoid those.

    Adapting some design patterns, even from authoritative sources, are unlikely to magically make your problems away, unless you have a clear understanding of the problems you are trying to solve using them.

    It can of course be really useful to study what kind of tools are out there, and listen to what people with a lot of experience can teach you, but I would also warn against getting into the habit of doing things just because people say it's best practice. Every project and team is different, and the practical needs of those should always triumph over what all the Youtube videos and books out there say is the "right way".

    So when you say:
    Internalizing a principle like this isn't constructive, unless you can say why doing things this way is beneficial. What problems could I run into if I left the Player class out of the equation?

    Every design decision usually has its pros and cons. To get more flexibility, you often need to introduce more complexity. To improve efficiency, you often need to sacrifice readability.

    There are clear potential pain points that could arise from routing all interactions through the Player class:
    1. Scalability issues - the Player class can grow so bloated over time that it becomes unwieldy.
    2. Low cohesion - It's not necessarily unintuitive that to make modifications to existing interactions or to add new ones, you need to go find some method inside the Player class.
    Making your code more modular, so that the Player class isn't needed to handle all interactions, is one way that these pain points could potentially be avoided.

    And I'm not trying to say that you definitely should avoid routing interactions through the Player class - I'm just saying that approaching code architecture from the point of view of practical pros and cons tends to yield better results in my experience, than dogmatically following "best practices" just because.
     
    CodeRonnie and angrypenguin like this.
  31. Magnilum

    Magnilum

    Joined:
    Jul 1, 2019
    Posts:
    241
    It was a pain to manage and add new things like a UI which manage the position of the slots inside the inventory or the interaction between the inventory and the equipment.

    Don't worry, I am not following a tutorial and just try to replicate it and if it works, that is fine , else, try another one. Because I want to create a big game so I can't do that and I know it. When I say I watched a lot of videos, I mean to understand the patterns which are known as good and try to see if they can fit to my case.

    I know that saying that following a video is like swearing on this forum cause you are used to see many people complaining about a code that they don't understand but it is not my case. I watch them to document myself and take the best one to me, understand it and adapt it.

    This is what I am trying to achieve but from the code above with my player class:

    I really don't see how to move these checks ouside the Player since he is the heart and as I think, a class should manage itself and only itself which is the case here.

    So ...
     
    SisusCo likes this.
  32. SisusCo

    SisusCo

    Joined:
    Jan 29, 2019
    Posts:
    1,321
    Awesome, that's great to hear. I don't think there's anything wrong at all with watching tutorials; they can be great way to learn how other people approach solving different problems. I probably just read too much into some of your choices of words :)

    So it sounds like maintainability and extendability were the biggest pain points?

    If that is the case, I would recommend trying to come up with a design that follows the open-closed principle. Like the approach taken in this this comment, for example.

    This way it should always remain easy to add new interactions to the system, or make modifications to existing ones, regardless of how many existing interactions there are.
     
    Last edited: Feb 26, 2024
    Magnilum likes this.
  33. Magnilum

    Magnilum

    Joined:
    Jul 1, 2019
    Posts:
    241
    You get it. This was the main problem of my previous code.

    I heard about this principle but I didn't find an example to refer this. It is hard (to me), to understand this principle.

    I am going to see how it could be integrated inside my projet for my needs.

    Now I am doing everything step by step and change things as it goes along.
     
    SisusCo likes this.
  34. CodeRonnie

    CodeRonnie

    Joined:
    Oct 2, 2015
    Posts:
    529
    I can provide you with a concrete example of The Open-Closed Principle. I have a class library that includes a LogFileWriter. It is a sealed class, you cannot inherit from it. It is closed for modification, unless I receive a bug report. However, it accepts an ILogFormatter instance as a constructor argument. You may provide a custom formatter to extend the functionality of the feature complete log file writer by customizing the formatter it uses. In that way, it is open for extension. Closed for modification of its complete, tested, finalized, versioned, signed, sealed, and delivered core purpose, its single responsibility. Yet, open for extension by providing any object you choose, that can format a string of characters, according to the ILogFormatter interface. You pass that formatter in via the ideal form of dependency injection, as a constructor argument, to the plain old C# LogFileWriter class. That is how you can easily modify the behavior, from the outside, and in an expected way, without needing to modify anything about the closed, core implementation. There is a MonoBehaviour facade that wraps around the core log file writer, to adapt its purpose to the Unity environment, but that is a separate layer in the overall composition. You can achieve things like this with interfaces, but I often also see callback delegates used as a way to open up extending behavior. If you have a system that is finished, code-wise, but can keep being altered from the outside via callbacks or changing its dependencies, then it is closed, yet open.
     
    Last edited: Feb 26, 2024
    Nad_B, angrypenguin and SisusCo like this.
  35. SisusCo

    SisusCo

    Joined:
    Jan 29, 2019
    Posts:
    1,321
    The key thing is the ability to add new functionality into the system, without having to make any modifications to the existing code.

    Like in Unity, you're able to add new functionality into game objects, without having to go edit the GameObject class every time; you just create a new class that derives from MonoBehaviour, and you're done.
     
    angrypenguin and CodeRonnie like this.
  36. Magnilum

    Magnilum

    Joined:
    Jul 1, 2019
    Posts:
    241
    Thank you both for your explanations. I also watched some videos to get an idea and it seems that I am kinda using it in my game so I have a better view of what it is.
     
    angrypenguin and SisusCo like this.
  37. angrypenguin

    angrypenguin

    Joined:
    Dec 29, 2011
    Posts:
    15,619
    One thing I use for my "Player" type classes is the idea of an "Ability". A Player has a list of Abilities which can be added and removed, mapped to buttons, etc. This means that I can reuse one Player class even if different types of Players can do very different things. One of my Players may be have a JumpAbility, a ShootAbility and a DodgeAbility, while another has a ShieldAbility and a HealAbility. That way you could even have in-game events which add or remove Abilities.

    In Unity that can look like the Player having a list of Abilities in the Inspector, or even just having multiple Ability MonoBehaviours on the GameObject.

    That's another example of Open / Closed. The Player's functionality in terms of world interactions is actually quite simple - it just starts and stops Abilities.
     
    Magnilum, SisusCo and CodeRonnie like this.
  38. Magnilum

    Magnilum

    Joined:
    Jul 1, 2019
    Posts:
    241
    I really like this and trying to do the same for my inventory, equipment and the rest. I don't want to use singleton or static since I want to make an entity system where each entity is able to pickup items, equip an item or doing many other things. But i don't really know how to make it.
     
  39. Nad_B

    Nad_B

    Joined:
    Aug 1, 2021
    Posts:
    708
    A very straightforward example:

    Don't do this:

    Code (CSharp):
    1. public class Player : MonoBehaviour
    2. {
    3.     public void UseAxe();
    4.     public void UseSword();
    5.     public void PickupHealthPotion();
    6.     public void PickupManaPotion();
    7. }
    But this:

    Code (CSharp):
    1. // Open-Closed principle:
    2. // Player is closed for modification: we don't need to add an additional method for each new game/pickable item
    3. // Open for extension: by creating new game/pickable items that add
    4. // new behaviors (the literal meaning, not MonoBehaviours) to the player by just passing them to UseItem/PickupItem.
    5. public class Player : MonoBehaviour
    6. {
    7.     public void UseItem(IGameItem gameItem)
    8.     {
    9.         gameItem.UseItem(this);
    10.     }
    11.  
    12.     public void PickupItem(IPickableItem pickableItem)
    13.     {
    14.         Inventory.Add(pickableItem);
    15.     }
    16. }
    17.  
    18. public interface IGameItem
    19. {
    20.     // Game item properties/methods
    21.     Transform Transform { get; }
    22.     void UseItem(Player player);
    23. }
    24.  
    25. public interface IPickableItem
    26. {
    27.     // Pickable item properties/methods
    28.     Transform Transform { get; }
    29. }
    30.  
    31. public class Axe : MonoBehaviour, IGameItem
    32. {
    33. }
    34.  
    35. public class Sword : MonoBehaviour, IGameItem
    36. {
    37. }
    38.  
    39.  
    40. public class ManaPotion : MonoBehaviour, IPickableItem
    41. {
    42. }
    43.  
    44. // This is where it becomes interesting:
    45. // HealthPotion can be both an IGameItem and an IPickableItem (this is called Composition)
    46. public class HealthPotion : MonoBehaviour, IGameItem, IPickableItem
    47. {
    48.  
    49. }
    We can go further by abstracting the "Player" itself, by creating an ICharacter interface:

    Code (CSharp):
    1. public interface ICharacter
    2. {
    3.     Transform Transform { get; }
    4.     void UseItem(IGameItem gameItem);
    5. }
    6.  
    7.  
    8. public class Player : MonoBehaviour, ICharacter
    9. {
    10.     // Same code as before
    11. }
    12.  
    13. // We modify IGameItem to use this abstraction instead of Player directly
    14. public interface IGameItem
    15. {
    16.     Transform Transform { get; }
    17.     void UseItem(ICharacter character);
    18. }
    Now you can have AI/NPCs that can use game items too!
    Code (CSharp):
    1. public class Enemy : MonoBehaviour, ICharacter
    2. {
    3.     public void UseItem(IGameItem gameItem)
    4.     {
    5.         gameItem.UseItem(this);
    6.     }
    7.  
    8.     // Other Enemy code
    9. }
     
    Last edited: Feb 27, 2024
    Magnilum likes this.
  40. Magnilum

    Magnilum

    Joined:
    Jul 1, 2019
    Posts:
    241
    This is pretty close to what I have started to do, but instead of ICharacter, I have a class named Entity and every mob, player etc derives from this one and for the IGameItem, I have a Item class which is te base for the other items in my game, so the derive from it.

    This is what it looks like:

    Code (CSharp):
    1. public class Entity
    2. {
    3.     EntityData data;
    4.  
    5.     public void UseItem(Item item)
    6.     {
    7.  
    8.     }
    9. }
    10.  
    11. public class Player : Entity {
    12.     PlayerController controller;
    13.  
    14.     // ...
    15. }
    16.  
    17. public class Item {
    18.  
    19.     ItemData data;
    20.  
    21.     public virtual bool Use(Entity entity)
    22.     {
    23.         // ...
    24.     }
    25. }
    26.  
    27. public class Weapon : Item {
    28.  
    29. }
    30.  
    31. public class Axe : Item {
    32.  
    33. }
    34.  
    35. public class Potion : Item {
    36.     Effects[] effects;
    37. }
    But I think, I should at least make an interface IUsable and add it to the Item class.
     
    Last edited: Feb 27, 2024
  41. tleylan

    tleylan

    Joined:
    Jun 17, 2020
    Posts:
    616
    Interesting read but I'd tend towards a completely different structure. These "seem" to be modeling how one suspects things work in the real world but does it? Must every object be owned by a player? Where are the unowned ones? Where are the co-owned ones? When a player "loses" all their money (doesn't die) does one traverse the inventory collection checking for money objects and remove them?

    Isn't it more along the lines of objects exist in the world and among the attributes they have is an owner or owners? If we wanted to know the distribution of gold coins for instance would we loop through each player, loop through each collection and sum the gold coins? And who is in charge of calculating the percentage owned?

    Similarly (but different) are things that everyone "has" they might just have zero of it. So again it isn't like they get a magic potion but rather that the amount of magic potion they have is zero. We don't look to see if they have some we only check about the level.

    All modelling depends upon how the models will be used but people pick up and drop stuff all the time that surely shouldn't be added and removed from a collection on the player, right?
     
  42. Magnilum

    Magnilum

    Joined:
    Jul 1, 2019
    Posts:
    241
    I don't understand where you want to bring me.

    Do you mean that I should make an inventory with all the items and only a variable to know the amount. It seem a pretty bad idea to me since I could have a wooden sword with some damages and another one with another damage value.
     
  43. tleylan

    tleylan

    Joined:
    Jun 17, 2020
    Posts:
    616
    It is just a conversation about modeling. You do what you need to accomplish your goals at the time. Your needs will evolve over time as will your solution.

    I wouldn't suggest such an inventory system if it was a bad idea. This concept of inventory is something you have constructed however. Look around your house (inside and out) are you carrying everything you own? Would you do that if you could? How about the things that you owned at one time but no longer own?

    One might argue that knowing what you used to own doesn't matter but it could particularly the make, model, the cost and when you bought and sold it. The point here is these are objects with properties and a history and the concept of "previous owner" (of your sword) for instance... where does one find that?

    And you didn't address "nobody owns the sword" so which inventory is it in and how do we find it? Searching all the player's inventories sounds like a poor idea.

    I'm really just asking questions. Doesn't your sword exist in the world with ownership simply one of many attributes?

    I think one can test the robustness of a solution by imagining simple queries. How many players are in the game? We check the count of the players collection. How many damaged swords are there? Do we count all the swords in the inventories of all the players? I wouldn't.
     
  44. spiney199

    spiney199

    Joined:
    Feb 11, 2021
    Posts:
    7,800
    Are you trying to provide solid code advice here or just philosophising?

    In any case you can abstract out the concept of ownership. My current project centralises all live item instances into a central repository, broken up by which container they belong to. That is abstracted through an
    IContainerIdentifier
    , so I can implement all kinds of identifiers, allowing the player, storage containers, even levels to have their own identifier and thus, have items that belong to them.
     
    Nad_B likes this.
  45. tleylan

    tleylan

    Joined:
    Jun 17, 2020
    Posts:
    616
    Only two options? :) I am guessing but it would seem that nobody would know the last part you mentioned if we were not discussing solutions. I specifically mentioned ownership because from what I read in the messages objects had to belong to a player in an inventory attached to a player object. That seemed limiting.

    I also didn't tell anyone not to do anything, I wrote that I wouldn't approach it that way. Discussions used to be considered a good thing has that changed?.
     
  46. angrypenguin

    angrypenguin

    Joined:
    Dec 29, 2011
    Posts:
    15,619
    Well, some of those are good questions to ask when coming up with the description for a class's responsibility. That really ought to be done before writing code, of course rather than afterwards.

    Those examples seem to use a few instances of repeated functionality. UseItem() is repeated a bunch, for instance, because everything is routed through ICharacter. That may be reasonable, of course, it depends on the use case. But working in Unity, would it not make sense to consider putting that on its own component which Player and Enemy and stuff you add later can all use?
     
  47. spiney199

    spiney199

    Joined:
    Feb 11, 2021
    Posts:
    7,800
    My comment was more focused on OP struggling to understand you. I don't think more introspection is going to help. They need more concrete examples.
     
    tleylan likes this.
  48. tleylan

    tleylan

    Joined:
    Jun 17, 2020
    Posts:
    616
    Concrete examples can go a long way (of course) but they often run the risk of being uniquely contrived to demonstrate an advantage/disadvantage of an implementation. I admit I'm facilitating (or trying) rather than answering but that can be useful particularly for someone who doesn't tend to ask those sorts of questions before deciding on a solution.

    Doesn't your sword exist in the world with ownership simply one of many attributes?

    I ask myself such questions all the time. Which object owns this, will any other object need access? Might some agent in the world need to see it, etc. I'm huge on refactoring so I tend to get things working and then refine, refine, refine. Sometimes I'm shocked by (despite it working) how relatively poor the original implementation was.
     
  49. spiney199

    spiney199

    Joined:
    Feb 11, 2021
    Posts:
    7,800
    I mean you could design an item system where items can have multiple owners. That wouldn't be necessarily difficult; and would potentially cut down on duplicated data. You'd have to just be careful when mutating an item, being sure to split it off into a new instance. Also finding all items that belong to someone or something in particular could potentially be rather slow, given enough items.

    As always there's an potentially infinite number of ways to accomplish something like an inventory system, just with various upsides and downsides. Just a matter of testing them out and refactoring should something not suit the needs that develop during the life of your project.
     
  50. Magnilum

    Magnilum

    Joined:
    Jul 1, 2019
    Posts:
    241
    It is always a good thing to discuss, I agree but I really don't want to be mean because of the time your are giving to me, it is respectful to see you trying to help me and I sincerely appreciate. The only thing is what @spiney199 said:

    And he is right, I am looking for a concrete exemples. I am not making a game just an health bar, some abilities, some enemies and 5 different items. I want to make something more interesting for the players and first, for me since I really want to play to the game I have in mind.

    That is true that I did not explained so much about what I want to make and how I see it to let you guys help me to find my way.

    So here are more explanations on what I would like to do and how I have done it for now:

    I am making a single player game so I have only one player. I want my items to be as modular as possible cause I want to create a system for example where a sword can see an increase of damage depending on many factors like the skills of the blacksmith, the materials etc so it implies something very modular.

    That is why I cannot make an simple inventory system with just few items as enums and increase a variable for the amount but a real system with slots containing an amount of items and an item which contains data. This is at least what I have thought about.

    Now the code part, I started to create a ObjectData class which handle the basic data of an object:

    Code (CSharp):
    1. public abstract class ObjectData : ScriptableObject
    2. {
    3.     [SerializeField] new string name;
    4.     [Space(5)]
    5.     [SerializeField] Sprite icon;
    6.     [SerializeField] GameObject prefab;
    7.  
    8.     #region Getter & Setter
    9.  
    10.     public string Name => name;
    11.     public Sprite Icon => icon;
    12.  
    13.     #endregion
    14. }
    Then I use this class make 2 new ones, the BagData and the ItemData. The items can be stored inside a bag but a bag cannot be inside another.

    Code (CSharp):
    1. public class BagData : ObjectData
    2. {
    3.     [Header("Bag Data")]
    4.     [SerializeField] float maxSize;
    5.  
    6.     #region Getter & Setter
    7.  
    8.     public float MaxSize => maxSize;
    9.  
    10.     #endregion
    11. }
    12.  
    13. public class ItemData : ObjectData
    14. {
    15.     [Header("Item Data")]
    16.     [SerializeField] bool isStackable;
    17.     [SerializeField] float size;
    18.     [SerializeField] float weight;
    19.  
    20.     #region Getter & Setter
    21.  
    22.     public bool IsStackable => isStackable;
    23.     public float Size => size;
    24.     public float Weight => weight;
    25.  
    26.     #endregion
    27. }
    Then I have created a Item class and a Bag class. For now the Item class is empty but I will add more stuff later especially in the derived classes.

    Code (CSharp):
    1. public class Item
    2. {
    3.     [SerializeField] ItemData data;
    4.  
    5.     #region Getter & Setter
    6.  
    7.     public ItemData Data => data;
    8.     public bool IsStackable => data.IsStackable;
    9.     public float Size => data.Size;
    10.     public float Weight => data.Weight;
    11.  
    12.     #endregion
    13.  
    14.     public Item(ItemData data)
    15.     {
    16.         this.data = data;
    17.     }
    18.  
    19.     public bool Contains(ItemData itemData)
    20.     {
    21.         return data == itemData;
    22.     }
    23. }
    24.  
    25. public class Bag
    26. {
    27.     [SerializeField] BagData data;
    28.     [SerializeField] List<Slot> slots = new List<Slot>();
    29.  
    30.     #region Getter & Setter
    31.  
    32.     public BagData Data => data;
    33.  
    34.     #endregion
    35.  
    36.     public Bag(BagData bagData)
    37.     {
    38.         data = bagData;
    39.     }
    40.  
    41.     public event Action<Slot> OnSlotAdded;
    42.     public event Action<Slot> OnSlotRemoved;
    43.     public event Action<Slot> OnSlotChanged;
    44.     public event Action OnChanged;
    45.  
    46.     public bool AddItem(Item item, int amount)
    47.     {
    48.         if (item == null) return false;
    49.         if (amount <= 0) return true;
    50.  
    51.         if (!CanAddItem(item, amount)) return false;
    52.  
    53.         Slot bagSlot = GetSlot(item);
    54.  
    55.         if (bagSlot == null || !item.IsStackable || !bagSlot.Add(amount))
    56.         {
    57.             bagSlot = new Slot(item, amount);
    58.             AddSlot(bagSlot);
    59.             return true;
    60.         }
    61.  
    62.         OnSlotChanged?.Invoke(bagSlot);
    63.         OnChanged?.Invoke();
    64.  
    65.         return true;
    66.     }
    67.  
    68.     public void AddSlot(Slot slot)
    69.     {
    70.         if (slot == null) return;
    71.  
    72.         slots.Add(slot);
    73.  
    74.         OnSlotAdded?.Invoke(slot);
    75.         OnSlotChanged?.Invoke(slot);
    76.         OnChanged?.Invoke();
    77.     }
    78.  
    79.     bool CanAddItem(Item item, int amount)
    80.     {
    81.         return item.Size * amount + GetTotalSize() <= data.MaxSize;
    82.     }
    83.  
    84.     public bool RemoveItem(ItemData itemData, int amount)
    85.     {
    86.         if (itemData == null) return true;
    87.         if (amount <= 0) return true;
    88.  
    89.         Slot bagSlot = GetSlot(itemData);
    90.  
    91.         if (data == null) return true;
    92.  
    93.         return bagSlot.RemoveCheck(amount);
    94.     }
    95.  
    96.     public bool RemoveSlot(Slot slot)
    97.     {
    98.         if (slots.Remove(slot))
    99.         {
    100.             OnSlotRemoved?.Invoke(slot);
    101.             OnSlotChanged?.Invoke(slot);
    102.             OnChanged?.Invoke();
    103.             return true;
    104.         }
    105.  
    106.         return false;
    107.     }
    108.  
    109.     Slot GetSlot(Item item)
    110.     {
    111.         foreach (var slot in slots)
    112.             if (slot.Contains(item.Data))
    113.                 return slot;
    114.         return null;
    115.     }
    116.  
    117.     Slot GetSlot(ItemData itemData)
    118.     {
    119.         foreach (var slot in slots)
    120.             if (slot.Contains(itemData))
    121.                 return slot;
    122.         return null;
    123.     }
    124.  
    125.     public bool Contains(BagData bagData)
    126.     {
    127.         return data == bagData;
    128.     }
    129.  
    130.     public float GetTotalSize()
    131.     {
    132.         float totalSize = 0;
    133.         foreach (var slot in slots)
    134.             totalSize += slot.GetSize();
    135.         return totalSize;
    136.     }
    137.  
    138.     public float GetTotalWeight()
    139.     {
    140.         float totalWeight = 0;
    141.         foreach (Slot slot in slots)
    142.             totalWeight += slot.GetWeigth();
    143.         return totalWeight;
    144.     }
    145. }
    Then I made a Slot class which stores the amount of an item:

    Code (CSharp):
    1. public class Slot
    2. {
    3.     [SerializeField] Item item;
    4.     [SerializeField] int amount;
    5.  
    6.     #region Getter & Setter
    7.  
    8.     public Item Item => item;
    9.     public int Amount => amount;
    10.  
    11.     #endregion
    12.  
    13.     public Slot(Item item, int amount)
    14.     {
    15.         this.item = item;
    16.         this.amount = amount;
    17.     }
    18.  
    19.     public event Action<int> OnAdded;
    20.     public event Action<int> OnRemoved;
    21.     public event Action OnChanged;
    22.     public event Action OnNull;
    23.  
    24.     public bool Add(int value)
    25.     {
    26.         if (!item.IsStackable) return false;
    27.  
    28.         amount += value;
    29.  
    30.         OnAdded?.Invoke(value);
    31.         OnChanged?.Invoke();
    32.  
    33.         return true;
    34.     }
    35.  
    36.     public bool RemoveCheck(int value)
    37.     {
    38.         if (!CanRemove(value)) return false;
    39.  
    40.         Remove(value);
    41.  
    42.         return true;
    43.     }
    44.  
    45.     public void Remove(int value)
    46.     {
    47.         amount -= value;
    48.  
    49.         if (amount < 0)
    50.         {
    51.             value += amount;
    52.             amount = 0;
    53.         }
    54.  
    55.         OnRemoved?.Invoke(value);
    56.         OnChanged?.Invoke();
    57.  
    58.         if (amount <= 0)
    59.             OnNull?.Invoke();
    60.     }
    61.  
    62.     public bool CanRemove(int value)
    63.     {
    64.         return amount - value >= 0;
    65.     }
    66.  
    67.     public bool Contains(ItemData itemData)
    68.     {
    69.         return item.Contains(itemData);
    70.     }
    71.  
    72.     public float GetSize()
    73.     {
    74.         return item.Size * amount;
    75.     }
    76.  
    77.     public float GetWeigth()
    78.     {
    79.         return item.Weight * amount;
    80.     }
    81. }
    Finally, I made a BagCollection class which is the backpack of the entity:

    Code (CSharp):
    1. public class BagCollection : MonoBehaviour
    2. {
    3.     [SerializeField] int maxNumBags = 1;
    4.     [SerializeField] List<Bag> bags = new List<Bag>();
    5.  
    6.     #region Getter & Setter
    7.  
    8.     public int MaxNumBags => maxNumBags;
    9.  
    10.     #endregion
    11.  
    12.     public event Action<Bag> OnBagAdded;
    13.     public event Action<Bag> OnBagRemoved;
    14.     public event Action<Bag> OnChanged;
    15.  
    16.     public bool AddBag(Bag bag)
    17.     {
    18.         if (bag == null) return false;
    19.         if (bags.Count >= maxNumBags) return false;
    20.  
    21.         bags.Add(bag);
    22.  
    23.         OnBagAdded?.Invoke(bag);
    24.         OnChanged?.Invoke(bag);
    25.  
    26.         return true;
    27.     }
    28.  
    29.     public void RemoveBag(BagData bagData)
    30.     {
    31.         if (bagData == null) return;
    32.  
    33.         Bag bag = GetBag(bagData);
    34.  
    35.         if (bag == null) return;
    36.  
    37.         bags.Remove(bag);
    38.         OnBagRemoved?.Invoke(bag);
    39.         OnChanged?.Invoke(bag);
    40.     }
    41.  
    42.     Bag GetBag(BagData bagData)
    43.     {
    44.         foreach (var bag in bags)
    45.             if (bag.Contains(bagData))
    46.                 return bag;
    47.         return null;
    48.     }
    49.  
    50.     public bool AddItem(Item item, int amount)
    51.     {
    52.         if (item == null) return false;
    53.  
    54.         foreach (var bag in bags)
    55.             if (bag.AddItem(item, amount))
    56.                 return true;
    57.         return false;
    58.     }
    59.  
    60.     public bool RemoveItem(Item item, int amount)
    61.     {
    62.         if (item == null) return false;
    63.  
    64.         foreach (var bag in bags)
    65.             if (bag.RemoveItem(item.Data, amount))
    66.                 return true;
    67.         return false;
    68.     }
    69.  
    70.     public float GetTotalWeight()
    71.     {
    72.         float totalWeight = 0;
    73.         foreach (var bag in bags)
    74.             totalWeight += bag.GetTotalWeight();
    75.         return totalWeight;
    76.     }
    77. }
    I did not work on the use of item yet so that is why there are not methods related to this.

    Now I can show you the Entity class, as the items, I made a EntityData class to hold data:

    Code (CSharp):
    1. public class EntityData : ScriptableObject
    2. {
    3.     [SerializeField] float maxHealth;
    4.     [SerializeField] float maxMana;
    5.     [SerializeField] float maxStamina;
    6.     [Space]
    7.     [SerializeField] float strength;
    8.     [SerializeField] float maxWeight;
    9.     [Space]
    10.     [SerializeField] float movementSpeed;
    11.  
    12.     #region Getter & Setter
    13.  
    14.     public float MaxHealth => maxHealth;
    15.     public float MaxMana => maxMana;
    16.     public float MaxStamina => maxStamina;
    17.     public float Strength => strength;
    18.     public float MaxWeight => maxWeight;
    19.     public float MovementSpeed => movementSpeed;
    20.  
    21.     #endregion
    22. }
    Then I made an Entity class which manage these data in the world for the interactions:

    Code (CSharp):
    1. public class Entity : MonoBehaviour
    2. {
    3.     protected virtual void Awake()
    4.     {
    5.         InitStats();
    6.     }
    7.  
    8.     #region Stats
    9.  
    10.     [Header("Stats")]
    11.     [SerializeField] EntityData data;
    12.  
    13.     public EntityData Data => data;
    14.     public Value health { get; private set; }
    15.     public Value mana { get; private set; }
    16.     public Value stamina { get; private set; }
    17.     public BonusValue strength { get; private set; }
    18.     public BonusValue maxWeight { get; private set; }
    19.     public BonusValue movementSpeed { get; private set; }
    20.  
    21.     protected virtual void InitStats()
    22.     {
    23.         health = new Value(data.MaxHealth);
    24.         mana = new Value(data.MaxMana);
    25.         stamina = new Value(data.MaxStamina);
    26.         strength = new BonusValue(data.Strength);
    27.         maxWeight = new BonusValue(data.MaxWeight);
    28.         movementSpeed = new BonusValue(data.MovementSpeed);
    29.     }
    30.  
    31.     #endregion
    32.  
    33.     #region Controllers
    34.  
    35.     [Header("Controllers")]
    36.     [SerializeField] EntityAnimatorController animatorController;
    37.     [SerializeField] EntityMovementController movementController;
    38.  
    39.     public EntityAnimatorController AnimatorController => animatorController;
    40.     public EntityMovementController MovementController => movementController;
    41.  
    42.     #endregion
    43. }
    And the Player class which derives from the Entity class:

    Code (CSharp):
    1. public class Player : Entity, IPickable
    2. {
    3.     [SerializeField] PlayerInputController inputController;
    4.     [SerializeField] PlayerCameraController cameraController;
    5.     [SerializeField] PlayerInteractionController interactionController;
    6.  
    7.     [Header("Player")]
    8.     [SerializeField] Equipment equipment;
    9.     [SerializeField] BagCollection bags;
    10.  
    11.     #region Getter & Setter
    12.  
    13.     public new PlayerMovementController MovementController => (PlayerMovementController)base.MovementController;
    14.     public PlayerInputController InputController => inputController;
    15.     public PlayerCameraController CameraController => cameraController;
    16.     public PlayerInteractionController InteractionController => interactionController;
    17.  
    18.     public Equipment Equipment => equipment;
    19.     public BagCollection Bags => bags;
    20.  
    21.     #endregion
    22.  
    23.     public bool Pickup(ObjectData objectData, int amount)
    24.     {
    25.         if (objectData is BagData bagData)
    26.             return AddBag(bagData);
    27.         else if (objectData is ItemData itemData)
    28.             return AddItem(itemData, amount);
    29.  
    30.         return false;
    31.     }
    32.  
    33.     public bool AddBag(BagData bagData)
    34.     {
    35.         return bags.AddBag(new Bag(bagData));
    36.     }
    37.  
    38.     public bool AddItem(ItemData itemData, int amount)
    39.     {
    40.         if (CanCarry(itemData, amount))
    41.             return bags.AddItem(new Item(itemData), amount);
    42.         return false;
    43.     }
    44.  
    45.     bool CanCarry(ItemData itemData, int amount)
    46.     {
    47.         return itemData.Weight * amount + bags.GetTotalWeight() <= maxWeight.Value;
    48.     }
    49. }
    My goal with this system is to change the properties of an item based on its data, as I explained above.

    Since you have now what I have done and the idea of what I want to achieve, I hope it will help you to understand in which direction I want to go and how you can help me.
     
    Last edited: Feb 28, 2024