Search Unity

Question How to keep clean code with linked classes?

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

  1. spiney199

    spiney199

    Joined:
    Feb 11, 2021
    Posts:
    7,859
    I think you're on the right approach with the ScriptableObject item and a wrapper class, though what I've had success with is have both express a common interface, so they can be used interchangeably. Not every item has to be wrapped, after all.

    The ObjectData base class seems kind of pointless to be honest. Is there ever a point where you will need to treat both as
    ObjectData
    ? Personally I'm not a fan of inheritance for the sake of reusing code.

    If you want things to express similar properties, an interface is probably a better option. Also I really wouldn't hide base properties with
    new
    . Just use a different, more descriptive field/property name such as
    DisplayName
    .

    Like others have mentioned before, I'd probably be separating anything to do with the player into multiple components, and not use inheritance. Single-responsibility-principle and all that. This is Unity, we can slap as many components onto a game object as we care to. Stats is one component, inventory is another component. You can unify how they're expressed via interfaces as well.

    Stuffing everything into the same class is going to cascade out of control the more and more features you add to your game. Don't be afraid to add new components. There's 15 components on the main game object for my player in my current project and I expect there to be more.
    upload_2024-2-28_21-58-9.png

    (Yes, lots of 'handlers')

    The next important thing to consider is save-games. Most of this is not save-game compatible, as you have data that's not encapsulated and being serialised directly into monobehaviours, and have a number of direct references to Unity objects. Your data is also scattered all over the place, too.

    You need to keep save-game compatibility in the fore-front of your mind when designing anything you want to persist between play sessions. For certain types of games, you might find most of your architecture is centered around making it compatible with save games.

    We can't serialise Unity objects as-is, nor we can serialise references to them. Continuing on as you are, you'd have to make serialisable surrogate classes for probably everything. I would only consider this if you only have a limited about of data to save.

    But if you think you'll have a large amount - which it sounds like you will - then you might want to consider making as much of your game-state inherently save-game compatible as possible, if not all of it. This will mean more plain C# objects, indirect references to Unity objects*, and centralising your data as much as possible.

    Anyway, that's as much as I can be bothered to write at the moment.

    *By indirect references, I mean rather than serialising a direct reference to assets, serialise a unique identifier it expresses, and use that to hot-lookup the asset when being accessed for the first time.
     
    CodeRonnie and SisusCo like this.
  2. Magnilum

    Magnilum

    Joined:
    Jul 1, 2019
    Posts:
    241
    I am curious to see what you did, I don't know what it seems to look like.

    I have made it to create a common base class to the ItemData and BagData and maybe add more classes later derived from it.
    I like inheritance because it allows me to have common properties and method already defined as opposed to interface where it is not defined. Since it is a very basic class I don't need to have very specific behaviours at this step. So, yes this ObjectData may seems pointless but it is just here to me to make the code a bit more structured and not having to rewrite the base properties and methods which are going to be the same at least, at this step for now.

    I really see the inheritance as a way to define common properties and methods because what is going to derive from it will be almost the same thing with some modifications.
    I see the interface like sharing the same methods and properties but for classes which are completely different from each other and as a way to create a common thing like intercation with the player.

    Don't know if it is the good approach.

    You are right, I wanted to change that but I forgot.

    I don't mind having 30 components on my player gameobject the only problem I have is how to connect them. I am gonna take the initial example, my player can carry a certain weight and before adding an item to the bag collection, I need to check if he is able to carry this new load. How am I doing this.

    I really want to make this component web system your are talking because I am starting to understand the power of splitting responsabilities among many classes but how to manage this kind of intercation. Should I ask the player if he is able to carry it or directly try to add it the bag collection which is going to ask to the player if he can carry it. How should I handle these types of checks. It was the reason of this thread.

    I have never reach a point in my previous development where I needed a save system so I am completely new to this. I really don't know how to plan this type of system since I have never used it before.

    I also wanted to know how to reference things. As you saw in my code I serialized almost everything with direct connection to the player for instance with the input system etc. Then I use the player (entity) as the main HUB of this machinery. I seeing it growing very quickly that is why I try to split everything into one class responsible of its own.

    I have planned to make data base as scriptable objects. One for the items, another for the entities etc. I don't know if it is what you had in mind.
     
  3. CodeRonnie

    CodeRonnie

    Joined:
    Oct 2, 2015
    Posts:
    529
    I haven't been following every detail of this thread closely, so this is just general information.

    It used to be true that only base class inheritance could provide a default implementation for methods and properties. However, interfaces do support that now, since C# 8.0. (Not to be confused with .NET 8.) :rolleyes: I just tested it in a build, in Unity 2021.3, and it all compiled just fine. Interfaces can have default properties and methods, already implemented. This lets you use them as "traits." You can just slap that interface on a class definition, and boom! It now has new functionality. No new code necessary. No shared base class required. (But, you could already extend interface behavior with extension methods, so it's not that revolutionary.)

    https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/proposals/csharp-8.0/default-interface-methods

    Some caveats:

    Interfaces can't have instance fields, like a base class can. Static fields, shared by the whole interface are allowed. This is the same with using static extension methods.

    You have to access the object as that interface type to access the default properties and methods. You can't just reference it as a MonoBehaviour. So, it's most useful when already doing something like programatically gathering components that implement that interface. But, you can just cast to or store a reference as the interface if necessary.

    Since default interface implementations arrived, interfaces are more similar to base classes, but without the requirement that types must derive from that certain base. However, if you always expect derived types to share a base class, then using an interface could be seen as a bit of a violation of the YAGNI principle. You ain't gonna need it. Using an interface or a base class is now more of a subtle difference, that hinges on whether you want to support any type of class, or if you really expect all implementations to derive from the same base class. Also, if you need derived instances to automatically acquire basic fields, not just properties.

    Final edit?: Actually interface extension methods allow you to do the same thing without having to cast to the interface type. My own meandering thoughts have brought me full circle yet again. That's probably why I wasn't already using them. (DIMs, not my thoughts. :p I am already using interface extension methods.) They say they are for class libraries to add new behavior to old interfaces, but that sounds like a possible violation of The Interface Segregation Principle if you ask me. But, the point remains, interfaces can have default methods, as long as they are static because you can just extend them with extension methods.
     
    Last edited: Feb 28, 2024
  4. tleylan

    tleylan

    Joined:
    Jun 17, 2020
    Posts:
    618
    This is done in apps all the time... While game development introduces the need for slightly better response times that isn't absent from the business software world where there aren't hundreds of objects but rather millions of them.

    Probably better discussed in another thread but I don't think waiting until the count of items (or in the example posted the total weight) is needed is the way to go. The system should know that total at all times. One doesn't sneak an item into the inventory so add to the inventory and adjust the weight totals. Again people should ask usability questions like "if I had to display the total weight of inventory on a heads-up display would I loop through the items each time"?

    If one isn't going to solve it that way in that extreme example what is the point of looping through it all when the value is needed slightly less often?
     
  5. tleylan

    tleylan

    Joined:
    Jun 17, 2020
    Posts:
    618
    I wasn't approaching this i so much as a "review my design" thread as I was a "let's discuss design" one :) So my replies may not apply to you directly but perhaps to others who stumble upon it in the future.

    It seems odd to me to have a Slot.Add method that can return false as in "no you cannot add this because it isn't a stackable item". I have no idea but it seems like Add shouldn't be available as a method if there are common cases where calling it will be denied. Maybe not, but it seems different from the case of "you don't have enough funds" or other dynamic conditions. This is a case of it never permitting Add to be used on a particular Slot.

    And again I don't have any idea I'm not willing to comb through the code but it looks like you have lots of methods declared as public that possibly shouldn't be. Like Slot.Remove which will reduce the value and RemoveCheck which seemingly checks first and then removes. There is nothing to prevents Remove from being called directly however.

    In any case... I still don't see the need for the player to have bags of stuff when the stuff exists and can be assigned to the player. Think about it from the standpoint of a banking app (how many transactions and accounts they have) and that they are dealing with money and time. Each account doesn't get a table that contains only their transactions but rather the transactions table contains an id that references the account it belongs to.

    We come back around to how do you calculate the total swords of all players or the average number of swords each player has? Yes, you do not need this for your game but you are making it hard to determine that if you should ever need to know. Your design of this game may not provide much help in your design of future games.

    Good luck on your game.. I have no doubt that in the end it will work I was thinking there might be a more flexible, less code intensive way to manage it.
     
  6. Magnilum

    Magnilum

    Joined:
    Jul 1, 2019
    Posts:
    241
    So since the difference it getting less noticeable, it is better to go with the interfaces now. I am not used to use them in such a way but it is gonna give me the occasion I guess. From what you said, I understand that I can keep my current inheritance and may change later if needed. But I don't really see the benefit of an interface for my BagData and ItemData since there are both quite the same, data for my game objects.

    Sadly, it seems not working.

    I totally agree, it is not useful to loop over the entire inventory each time I add an item, that is right. I could just make a viariable in the inventory called totalWeight and increase this one each time I add something to the inventory.

    Since I could have some effect which could change the size or the weight of an item, I just have to find a way to tell to the backpack, "hey, this item currently inside you has seen its weight changed, add the difference to the totalWeight".

    But still, it is not the main topic of this thread, I am just looking for a the good way to make my component communicate between them. When I add something to the player inventory, should I create a method inside the player class to check if he can carry and if so, add it to the backpack or directly ask to the inventory to add this item and it is going to ask to the player if he can carry this. Or another way, I don't know that is why I am asking.

    First I planned to add a stack system, so check before adding if the maximum stack was not reached, then I changed for this new system of backpack and size so the stack system is useless and I didn't remove it yet.

    I am going to take stamina as an example. Some times you want your player to make a attack if he has enough stamina. And sometimes you just remove a certain amount no matter if it is going below zero. Same for this system, I want to remove 10 sticks from its inventory, if he has more, good for him, if he has the exact amount or less, I remove everything. And some times I want him to pay using item like a trade. So if he doesn't have the exchange, I cannot make the trade.

    Yes since I have only one player in my world I don't know why I would like to count the number of sword in this one and even if I had more players. But this is going to be a fraction of time for the PC to loop through the backpack and count. And if there is a lag due to this, I will find a way to optimize it but It is far of my concerns now.

    But why not provinding solutions with concrete example. I mean, instead of going around talking about the same thing again and again, counting the number of swords in all of my players, why don't you give me some clues to make it better. Because the following statement is completlely true:

    And yes, we are discussing but talking about the same thing is not going to help me to have a better understanding of the problem and what can be the solution.

    I am just looking for an answer to continue my way and learn new things. I am not a beginner but I far from being a master so I don't always know what you mean by counting the number of swords in all player inventories etc.

    I don't want to be rude but we now have reached the second page of threads and I have the feeling of not having any clue for the future since the answer to my question is still abscent. May be I did not understand what you gave me or it is just too complexe to be resumed in some pieces of code.
     
  7. spiney199

    spiney199

    Joined:
    Feb 11, 2021
    Posts:
    7,859
    It's just an interface to express the properties items should have, and then a scriptable object, and a wrapper class both implementing the same interface:
    Code (CSharp):
    1. public interface IItemDefinition
    2. {
    3.     string DisplayName { get; }
    4.    
    5.     // other properties
    6. }
    7.  
    8. [CreateAssetMenu]
    9. public class ItemDefinition : ScriptableObject, IItemDefinition
    10. {
    11.     [SerializeField]
    12.     private string _displayName;
    13.    
    14.     public string DisplayName => _displayName;
    15.    
    16.     // etc etc
    17. }
    18.  
    19. [System.Serializable]
    20. public class ItemInstance : IItemDefinition
    21. {
    22.     [SerializeField]
    23.     private ItemDefinition _itemDefinition;
    24.    
    25.     public string DisplayName
    26.     {
    27.         get
    28.         {
    29.             string name = _itemDefinition.DisplayName;
    30.            
    31.             // potentially modify name
    32.            
    33.             return name;
    34.         }
    35.     }
    36. }
    So, regardless of whether you have a wrapper or an actual asset, you can treat them the same.

    Honestly... just by having them reference one another via the inspector. Generally it's best for these relationships to be one-way. One component references another, but not the other way around. Then you just introduce suitable API's as needed.

    I would strongly suggest that before you proceed with a game like this, that you spend some time on learning the basics of a save system. As you can very likely get into a position where everything you have written is not save game compatible at all, and you either have to hack in a really awful solution, or undo most if not all of your progress so far.

    You need to think of save-games ahead of time. For a game like the one you're making, save-games will likely be guiding a lot of your data structures.

    As mentioned, we cannot write a UnityEngine.Object disk, nor can we write references to Unity objects to disk, and vice versa. Your save data needs to be entirely plain C# object data. Think ahead when designing your data structures, and consider whether it needs to be part of your save games or not.
     
  8. tleylan

    tleylan

    Joined:
    Jun 17, 2020
    Posts:
    618
    Since I could have some effect which could change the size or the weight of an item, I just have to find a way to tell to the backpack, "hey, this item currently inside you has seen its weight changed, add the difference to the totalWeight".

    It seems incredibly unlikely that the weight of something in the inventory pool could change and it would mean you accepted items into a backpack that you can no longer carry. You're making the game un-programmable by introducing "what ifs" that have not happened and most probably won't. BUT... when and if some need develops that was unforeseen earlier you adjust the code accordingly.

    You also don't need to "find a way" as if it would be difficult. You have one player, one backpack surely a Recalculate() can be written.

    In any case I'll bow out for now. I'm not so much interested in the minutiae as I am in the big picture when it comes to threads such as these :)
     
  9. angrypenguin

    angrypenguin

    Joined:
    Dec 29, 2011
    Posts:
    15,620
    Indeed!

    Rather than trying to account for lots of specific cases (weight increased) I'd do my best to encapsulate them in something pretty broad, e.g. "an item changed". In response, I then do a generic recalculation of anything that might matter.

    Why? Only a single code path to address all cases, and less code, thus fewer opportunities to make mistakes, less to test, less to maintain. These are all good things.

    The potential downside is that it's a bit more work for the computer, but here's the thing: that code will barely ever run, so unless there's something crazy in there, or large numbers of items, optimisation time is likely best spent elsewhere.
     
    Magnilum likes this.
  10. Magnilum

    Magnilum

    Joined:
    Jul 1, 2019
    Posts:
    241
    Ok so instead of using inheritance to transfer the name contained in the item data, you use an interface.

    But why the interface is better than just making a public property and returns the name. Cause for me, using an interface means that at a certain point you will have to loop through something which contains ItemDefinition and ItemInstance, isn't it. Or pointing at something which could be either one or the orther but as the definition is not an instance, this case seems not possible.

    I would have done this since an item instance exists only with an ItemDefinition and both cannot be placed together.

    Code (CSharp):
    1. [CreateAssetMenu]
    2. public class ItemDefinition : ScriptableObject
    3. {
    4.     [SerializeField]
    5.     private string _displayName;
    6.  
    7.     public string DisplayName => _displayName;
    8.  
    9.     // etc etc
    10. }
    11. [System.Serializable]
    12. public class ItemInstance
    13. {
    14.     [SerializeField]
    15.     private ItemDefinition _itemDefinition;
    16.  
    17.     public string DisplayName
    18.     {
    19.         get
    20.         {
    21.             string name = _itemDefinition.DisplayName;
    22.          
    23.             // potentially modify name
    24.          
    25.             return name;
    26.         }
    27.     }
    28. }
    Sorry, I know that I may focus on this too much and may be it is an evidence for you but I really want to understand.

    And thank you for this example, it helps me a lot to see what you were talking about.

    So I just need to figure out which class will handle the reference now to the other, either the backpack to the player or the player to the backpack.

    I will do it, since it seems very important and I don't want to redo everything or make a weird system not pleasant to work with for the save system.

    So my Entity script or BagCollection scripts won't work at all for being saved?!

    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. }
    44.  
    45. public class BonusValue
    46. {
    47.     [SerializeField] float value;
    48.  
    49.     public float bonus { get; private set; }
    50.  
    51.     public BonusValue(float value)
    52.     {
    53.         this.value = value;
    54.     }
    55.  
    56.     #region Getter & Setter
    57.  
    58.     public float InitValue => value;
    59.     public float Value => value + bonus;
    60.  
    61.     #endregion
    62.  
    63.     public event Action<float> OnBonusAdded;
    64.     public event Action<float> OnBonusRemoved;
    65.     public event Action<float> OnChangedChanged;
    66.  
    67.     public virtual void SetInitValue(float value) {}
    68.  
    69.     public void SetBonus(float value) {}
    70.  
    71.     public void AddBonus(float value) {}
    72.  
    73.     public void RemoveBonus(float value) {}
    74. }
    75.  
    76. public class Value
    77. {
    78.     [SerializeField] BonusValue maxValue;
    79.  
    80.     public float value { get; private set; } = 0;
    81.  
    82.     public Value(float maxValue) : this(new BonusValue(maxValue))
    83.     {
    84.  
    85.     }
    86.  
    87.     public Value(BonusValue maxValue)
    88.     {
    89.         this.maxValue = maxValue;
    90.  
    91.         maxValue.OnChangedChanged += (val) =>
    92.         {
    93.             if (value > maxValue.Value)
    94.                 value = maxValue.Value;
    95.         };
    96.     }
    97.  
    98.     #region Getter & Setter
    99.  
    100.     public BonusValue MaxValue => maxValue;
    101.     public float Percentage => value / maxValue.Value;
    102.  
    103.     #endregion
    104.  
    105.     public event Action<float> OnAdded;
    106.     public event Action<float> OnRemoved;
    107.     public event Action<float> OnChanged;
    108.  
    109.     public void SetToMax() {}
    110.  
    111.     public void SetValue(float val) {}
    112.  
    113.     public bool AddCheck(float val) {}
    114.  
    115.     public void Add(float val) {}
    116.  
    117.     public bool RemoveCheck(float val) {}
    118.  
    119.     public void Remove(float val) {}
    120. }
    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.     public void RemoveBag(BagData bagData) {}
    19.  
    20.     Bag GetBag(BagData bagData) {}
    21.  
    22.     public bool AddItem(Item item, int amount) {}
    23.  
    24.     public bool RemoveItem(Item item, int amount) {}
    25.  
    26.     public float GetTotalWeight() {}
    27. }
    I had in mind to upgrade some items directly inside the player inventory which could lead to an increase of the size or the weight. But I guess it is better to make it into a workbench or limit the upgrades to some values and not the weight and the size.

    Sad, cause the big picture is very important to determine what must be done but the details are the realization of all the previous work.

    This is a good point!
     
  11. spiney199

    spiney199

    Joined:
    Feb 11, 2021
    Posts:
    7,859
    The point of an interface is for different objects to express the same properties, and this can they all be treated as if they were said interface type. Polymorphism works with interfaces, remember. They can be treated as the same thing, while have slightly different implementations.

    High change that different parts of an inventory system won't care whether it's an instance or an asset, so having them express a common interface mean either can be used indifferently, and reduces doubled up code. And in cases where you do care, you can validate your data before proceeding (such as converting an asset into a wrapper).

    Pretty much. Save data is best wrapped up in plain C# objects, rather than serialised directly into monobehaviours or scriptable objects. Thus you can just take that C# object, and write it to disk. Though generally I always have a object that encapsulates all my save data, and various systems access it parts of it, and modify it's data as needed. Then said object is written to disk whenever the player saves the game.

    You may have save data outside this object of course. The current exception in my current project is that the data for my procedurally generates worlds get saved as separate files. They, of course, are also a single plain C# object, with plain C# data all the way down; no direct Unity object references (but definitely indirect ones).
     
  12. Magnilum

    Magnilum

    Joined:
    Jul 1, 2019
    Posts:
    241
    I knew this but I did not understand the purpose of using interfaces with what I consider as same object like an item, a sword, an axe etc.

    I see the benefit but not the use case. I will may be run into this later so I keep it in mind.

    Do you have any example to give me? Or any good videos I should watch.

    I think I am not sure about the definition of a plain C# objects. To me, it seems like a class which holds the data to be saved easily.
     
    Last edited: Feb 29, 2024
  13. SisusCo

    SisusCo

    Joined:
    Jan 29, 2019
    Posts:
    1,326
    Plain old C# object refers to an instance of a type that doesn't have a dependency to a framework.

    In the context of Unity the term is usually used to refer to instances of any classes that don't derive from UnityEngine.Object.
     
    spiney199 and Magnilum like this.