Search Unity

  1. Unity 2019.2 is now released.
    Dismiss Notice

Polymorphism on Scriptable Objects

Discussion in 'Scripting' started by dfinke, Aug 13, 2019 at 6:16 PM.

  1. dfinke

    dfinke

    Joined:
    Dec 29, 2015
    Posts:
    2
    Hi everyone,

    let's say I wanted to create an inventory system. I would have a few scriptable objects like:
    ItemBase and Consumable:ItemBase for example. I create a ConsumableItem in the Editor and drag it onto the OtherClass script in the editor.

    Code (CSharp):
    1. public class ItemBase : ScriptableObject{
    2.     [SerializeField] private string itemName;
    3.     public string ItemName{get{return itemName;}}
    4. }
    5.  
    6. public class ConsumableItem : ItemBase{
    7.     [SerializeField] private string hpToAdd;
    8.     public string HpToAdd{get{return hpToAdd;}}
    9. }
    10.  
    11. // added to a gameObject in the scene like the mainCamera so it get's executed on play
    12. public class OtherClass : MonoBehaviour{
    13.     public ItemBase myItem; // a consumableItem scriptable object is dragged in here in the editor
    14.  
    15.     void Start(){
    16.         printItemName(myItem);
    17.         printHpToAdd(myItem);
    18.     }
    19.  
    20.     void printItemName(ItemBase item){
    21.         print(item.ItemName); // will work because it's a prop of ItemBase
    22.     }
    23.  
    24.     void printHpToAdd(ItemBase item){
    25.         print(item.HpToAdd); // won't work because function is taking ItemBase as a param and not ConsumableItem
    26.     }
    27. }
    Even if I used a item database I would always have the mismatch between types.
    How is this done correctly?
     
  2. lordofduct

    lordofduct

    Joined:
    Oct 3, 2011
    Posts:
    6,609
    That's not what polymorphism is.

    Polymorphism is allowing you to treat ConsumableItem as a ItemBase... not the other way around, like you want.

    'OtheClass' doesn't know if ItemBase is a ConsumableItem or not. The variable/field 'myItem' is typed as ItemBase, as a result you could have dragged anything is an ItemBase (or inherits from ItemBase) that isn't necessarily a ConsumableItem.

    If 'OtherClass' expects 'myItem' to be a ConsumableItem, then type the variable as ConsumableItem.
    Code (csharp):
    1.  
    2. public class OtherClass : MonoBehaviour {
    3.     public ConsumableItem myItem;
    4. ...
    5.  
    If 'OtherClass' expects 'myItem' to be a ItemBase, BUT has optional functionality if it just happens to be a ConsumeableItem do something like this:
    Code (csharp):
    1.  
    2. void printHpToAdd(ItemBase item){
    3.     if(item is ConsumableItem) print((item as ConsumableItem).HpToAdd);
    4. }
    5.  
    ... mind you I don't get why you're printItemName and printHpToAdd methods are private and take in an ItemBase. Those methods are private and local to 'OtherClass'... they could just access the local field/variable. Anyways, that's a semi-related tangent.
     
  3. Owen-Reynolds

    Owen-Reynolds

    Joined:
    Feb 15, 2012
    Posts:
    692
    Can also read about just regular polymorphism: inheritance, overloading, abstract base classes. It's a general topic, so you're not stuck reading only Unity sites.

    That type of confusion is common -- the first time someone sees something is in Unity, so they check Unity sources for it; not knowing it's in common use and Unity is using it the normal way.
     
    lordofduct likes this.
  4. StarManta

    StarManta

    Joined:
    Oct 23, 2006
    Posts:
    6,365
    While that works, I would generally caution against checking for specific subclasses except as a last resort. Ideally, the class (e.g. OtherClass) that's making use of the base class (e.g. ItemBase) shouldn't need to know about any of the derived classes (e.g. ConsumableItem) in order to function; rather, the derived classes should have overrides that handle the different functionality. This will help keep your code organized.

    The provided example code is not a good example of this, which is to say, OtherClass shouldn't have a "printHpToAdd" method at all. If this is part of some supplementary data that different derived classes might have, then you'd want ItemBase to have a PrintSupplementaryData() function (probably empty), and then override that in any derived classes to output whatever supplementary data is appropriate to that class.
    Code (csharp):
    1.  
    2. public class ItemBase : ScriptableObject{
    3.     [SerializeField] private string itemName;
    4.     public string ItemName{get{return itemName;}}
    5. public virtual void PrintSupplementaryData() { }
    6. }
    7.  
    8. public class ConsumableItem : ItemBase{
    9.     [SerializeField] private string hpToAdd;
    10.     public string HpToAdd{get{return hpToAdd;}}
    11. public override void PrintSupplementaryData() {
    12. print(HpToAdd);
    13. }
    14. }
    15.  
    16. // added to a gameObject in the scene like the mainCamera so it get's executed on play
    17. public class OtherClass : MonoBehaviour{
    18.     public ItemBase myItem; // a consumableItem scriptable object is dragged in here in the editor
    19.  
    20.     void Start(){
    21.         printItemName(myItem);
    22.         printSupplementaryData(myItem);
    23.     }
    24.  
    25.     void printItemName(ItemBase item){
    26.         print(item.ItemName); // will work because it's a prop of ItemBase
    27.     }
    28.  
    29.     void printSupplementaryData(ItemBase item){
    30.         item.PrintSupplementaryData(); //will print nothing for BaseItem, but will print the hpToAdd for ConsumableItem
    31.     }
    32. }
    Exceptions to this guideline might be if you don't have control over the classes (e.g. they came from a package you don't want to or can't modify), or if you need some data from the current scope that the classes don't have (though even then a virtual method with a parameter is usually preferable.)
     
    eisenpony and lordofduct like this.
  5. lordofduct

    lordofduct

    Joined:
    Oct 3, 2011
    Posts:
    6,609
    I agree @StarManta, it's definitely smelly for OtherClass to do that. It's why I offered it up as the secondary "but" scenario.

    I would have liked to go into a lengthy diatribe as to why it's smelly, but alas, at work today. So my posts are shorter.
     
  6. StarManta

    StarManta

    Joined:
    Oct 23, 2006
    Posts:
    6,365
    Why do you think mine are so long? :D
     
    jeffreyschoch likes this.
  7. eisenpony

    eisenpony

    Joined:
    May 8, 2015
    Posts:
    788
    In the spirit of learning, especially about things that are not Unity specific...

    One technique for dealing with the specific problem you identified is Double Dispatch. You can read about it here: https://lostechies.com/derekgreer/2010/04/19/double-dispatch-is-a-code-smell/

    If you even got as far as the url, you'll notice that double dispatch is considered bad practice. The reason is it causes an explosion of modifications whenever you go about adding new types.


    StarManta's advice is better; rather than requiring your "controller" class to know what it's dealing with, think about the common tasks all items will be able to do. For items, I think of "using", or "apply the effect".

    It might be confusing how to adapt the simple example to a real implementation, so let me take it one step further.

    Very likely, you won't be interested in "printing supplementary data", but actually doing something. Doing things in Unity typically requires GameObjects, so let's define our interface to require some.

    Code (CSharp):
    1. public abstract class ItemBase : ScriptableObject{
    2.     [SerializeField] private string itemName;
    3.     public string ItemName{get{return itemName;}}
    4.     public abstract void Apply(GameObject user, GameObject target);
    5. }
    6.  
    7. public class ConsumableItem : ItemBase{
    8.     [SerializeField] private string hpToAdd;
    9.     public string HpToAdd{get{return hpToAdd;}}
    10.  
    11.     public override void Apply(GameObject user, GameObject target)
    12.     {
    13.       var hp = target.GetComponent<IHPSystem>();
    14.       if (hp != null)
    15.         hp.Heal(HpToAdd);
    16.     }
    17. }
    18.  
    19. // added to a gameObject in the scene like the mainCamera so it get's executed on play
    20. public class OtherClass : MonoBehaviour{
    21.     public ItemBase myItem; // a consumableItem scriptable object is dragged in here in the editor
    22.  
    23.     void Start(){
    24.       myItem.Apply(this.gameObject, this.gameObject);
    25.     }
    26. }
    This of course assumes another component: the HPSystem. A simple implementation might look like this:

    Code (csharp):
    1. interface IHPSystem
    2. {
    3.   void Heal(int amount);
    4.   void Damage(int amount);
    5. }
    6. class HPSystem : MonoBehaviour, IHPSystem
    7. {
    8.   [SerializeField] private int hp;
    9.   public int HP => hp;
    10.  
    11.   [SerializeField] private int maxHp;
    12.   public int MaxHP => maxHp;
    13.  
    14.   public void Heal(int amount)
    15.   {
    16.     hp = Mathf.Max(hp + amount, maxHp);
    17.   }
    18.  
    19.   public void Damage(int amount)
    20.   {
    21.     hp = hp - amount;
    22.     // Here is a great place to test if hp <= 0 and trigger an event
    23.   }
    24. }
    If you're paying close attention, you might realize it would be useful to disallow the use of an item when targeting something without an HPSystem. Or even when the target doesn't need the item's effect. You could use an exception or error code in the Apply method, but there is a better option to avoid this vexing problem.

    Let's modify our item interface slightly:
    Code (CSharp):
    1. public abstract class ItemBase : ScriptableObject{
    2.     [SerializeField] private string itemName;
    3.     public string ItemName{get{return itemName;}}
    4.     public abstract bool TryApply(GameObject user, GameObject target, out string failureReason);
    5. }
    Now we can do the following:
    Code (csharp):
    1. public class ConsumableItem : ItemBase{
    2.    [SerializeField] private string hpToAdd;
    3.     public string HpToAdd{get{return hpToAdd;}}
    4.  
    5.     public override bool TryApply(GameObject user, GameObject target, out string failureReason)
    6.     {
    7.       var hp = target.GetComponent<IHPSystem>();
    8.       if (hp == null)
    9.       {
    10.         failureReason = "The target cannot be healed because it cannot be damaged!";
    11.         return false;
    12.       }
    13.       else
    14.       {
    15.         hp.Heal(HpToAdd);
    16.         failureReason = "";
    17.         return true;
    18.       }
    19.     }
    20. }
    21.  
    22. // added to a gameObject in the scene like the mainCamera so it get's executed on play
    23. public class OtherClass : MonoBehaviour{
    24.     public ItemBase myItem; // a consumableItem scriptable object is dragged in here in the editor
    25.  
    26.     void Start(){
    27.       if (!myItem.TryApply(this.gameObject, this.gameObject, out var error)
    28.         print(error);
    29.     }
    30. }
    Just the addition of a few common pieces of data, and a simple method will open up a lot of new use cases. I hope that shows how rethinking the design to prefer calling methods on the base class, passing in the potentially useful information, and hiding the implementation in the derived classes keeps things much simpler than trying to know what concrete type your "controller" class is holding.
     
    Last edited: Aug 13, 2019 at 10:47 PM
    lordofduct likes this.
  8. dfinke

    dfinke

    Joined:
    Dec 29, 2015
    Posts:
    2
    Hey @eisenpony @lordofduct @Owen-Reynolds @StarManta ,
    honestly I'm surprised about how fast and detailed your replies came in. I've seen much less supportive forums. So thanks so much!

    I admit the example I provided is not ideal. I see that now. It was meant to make a point and to give repliers at least something to work with, without posting the whole inventory system in the forum.

    I will read through your replies soon.
     
  9. Suddoha

    Suddoha

    Joined:
    Nov 9, 2013
    Posts:
    2,482
    This can introduce quite some logic to the items though.

    And unfortunately, there's the problem that "using an item" does not necessarily imply one certain kind of effect / action, i.e. there's not just "it does work" and "it does not work", and even for an action, it'd likely be possible to play out differently depending on the target. If you wanted to cover all that in item classes, it'd clutter the items really quickly.

    Sometimes these problems are not noticeable from the beginning, and become a problem later down the road.
     
  10. eisenpony

    eisenpony

    Joined:
    May 8, 2015
    Posts:
    788
    Hi Suddoha, how are things?

    It's been a while since we could debate. Okay, I'll bite!

    Quite right, and for the short term, I think that's precisely where that logic should be. Once I have a database of dozens of items, I may notice some common patterns and refactor to use a common "targeting system" or something like that. If I do though, I'm more likely to inject it into those items that need it, or use the decorator pattern to wrap the items with "AllyTargetableItem" or something like that. I'd prefer to stay away from moving that responsibility to the "controller" class, especially if it's not 100% common to all possible items. To mee this is the best pragmatic choice.

    I'm not 100% sure I know what you're getting at, but a binary "worked or didn't work" seems pretty good for any item's I can think of. Even if an item was only partially used, such as using an item which heals hp and mp on a character that only requires hp. Besides that, my suggestion actually provides more information in the case of "didn't work", so it's not really just binary.

    Again, I think I need more context to understand your point. Also, injecting components that contain that logic into the items that need it seems like a simple way around your proposed issue.

    Quite right. However, I think my suggestion strikes a good balance of forethought and pragmatism. I don't have the same experience you guys do building games, but this seems like a good solution to the LOB App developer in me. Still, I'd love to hear any hard-earned nuggets of wisdom others have from building their own item systems!
     
  11. Suddoha

    Suddoha

    Joined:
    Nov 9, 2013
    Posts:
    2,482
    That's certainly a way to implement it. There's no wrong or right.

    However, now the items need to know how to to resolve the components. What if the health wasn't implemented as a component? I'm aware that's just an example, but at the moment it sort of dictates the requirements for the consumers.

    Also, given that the consumer is likely the one who "uses an item", it seems a little odd to pass itself as "some target" of which the actual component has to be resolved. I mean, why attempting to apply an item when the consumer may already have the information whether or not it can use that item in the first place?

    So, if we suppose that the consumer is the one using the item (directly), it'd make more sense to pass at least IHPSystem as the argument:
    - no knowledge of how to resolve the target component
    - no knowledge of whether or not the system is a component
    - no "target cannot use it" cases (I'd argue an item should be doing that)
    - no calls to items that do not support it, unless it accepts such argument and still "doesn't support" it

    Of course, this might require more work on the callers side in order to manage different item types, as you cannot have that one method signature for everything.

    But at this point, we could already do the following
    Code (csharp):
    1. private int _hpRecovery = 20;
    2. // ...
    3. public void TryApply(IHPSystem system)
    4. {
    5.      if (system == null)
    6.      {
    7.          // throw due to wrong usage, or report "false" using the try-pattern
    8.      }
    9.    
    10.      system.Heal(_hpRecovery);
    11. }
    12.  
    Now when you look at that, it's almost like we're running an extra-mile for nothing. Instead, we could get the HP through a property and apply it in our own specific way.

    The following circumstances may support that decision:
    - entity has full hp
    - entity is dead
    - entity cannot heal at the moment

    These cases have to be handled somewhere and that's likely not an item's responsibility. So we could move the entire "apply health" process to something that has all the information. There's much more to applying HP than just "add X HP", which the item cannot satisfy without all the extra information that it shouldn't know about.

    Consider a game with debuffs. A specific item's effect might be weakened, useless, inverted... That might no longer be a trivial example, but usually games start out very simple, but as the time goes by new features are introduced. So now the calling instance needs to evaluate which item it's dealing with, and with that in mind, it needs to intercept and modify the values before they are applied.

    And the case you proposed attempts to avoid a vexing exception using the try-pattern, but even the try-pattern is sort of fixing a problem that shouldn't occur in the first place, IMO. Reason being that it yields information about whether the argument is capable of providing the type-based functionality, whereas the arguments can be constrained using a specific type.

    It's very common in Unity to pass GameObjects around, and let other types use GetComponent to resolve the components it needs. That tends to be very flexible in many cases, but it also moves more responsibilities to those components and systems that they wouldn't need to care about when they just receive an argument of the required type.