Search Unity

Question How to mix abstract classes and scriptable objects?

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

  1. Magnilum

    Magnilum

    Joined:
    Jul 1, 2019
    Posts:
    241
    Hi, I am trying to improve my knowledge in class coding.

    I have DEMs (Digital Elevation Map) which is a fancy word in engineering to say heightmap. I have made a ScriptableObject class to handle the data of these heightmaps.

    Code (CSharp):
    1. [CreateAssetMenu(fileName = "New DEM", menuName = "Tools/Terrain Generator/DEM")]
    2. public class DEM : ScriptableObject {
    3.  
    4.     public static float scale = 0.5f;
    5.  
    6.     [SerializeField] ComputeShader computeDEM;
    7.  
    8.     [Header("Image")]
    9.     [SerializeField] Texture2D imageHeightMap;
    10.     [SerializeField] float meterPerPixel;
    11.     [SerializeField] float minValue;
    12.  
    13.     [Header("Coordinates Range")]
    14.     [SerializeField] Vector2 latitudeRange;
    15.     [SerializeField] Vector2 longitudeRange;
    16. }
    The issue is I have 2 types of DEMs, Polar and Cylindrical and both need a different process in order to get the same data. Let me explain.

    For instance, I need to convert my coordinates (latitude, longitude) to pixels coordinates. So I have created a method for this:

    Code (CSharp):
    1. public Vector2 GetPixelCoordinates(Vector2 coordinates)
    2. {
    3.  
    4. }
    But I need to make 2 different processes depending on the type of the DEM. I could use a simple if statement but it is not very scalable and I want to use this case to improve my coding skills.

    I also tried to create an abstract class which I guess is the best case. I have done it like this:

    Code (CSharp):
    1. public abstract class DEMStructure
    2. {
    3.  
    4.     public static float scale = 0.5f;
    5.  
    6.     public DEMData data { get; private set; }
    7.  
    8.     public DEMStructure(DEMData data)
    9.     {
    10.         this.data = data;
    11.     }
    12.  
    13.     public abstract Vector2 GetPixelCoordinates(Vector2 coordinates);
    14.     public abstract bool IsInsideSquare(Vector2 minBorderCoordinates, Vector2 maxBorderCoordinates);
    15. }
    16.  
    17. public class CylindricalDEM : DEMStructure
    18. {
    19.     public CylindricalDEM(DEMData data) : base(data) { }
    20.  
    21.     public override Vector2 GetPixelCoordinates(Vector2 coordinates)
    22.     {
    23.        return Vector2.zero;
    24.     }
    25.  
    26.     public override bool IsInsideSquare(Vector2 minBorderCoordinates, Vector2 maxBorderCoordinates)
    27.     {
    28.        return false;
    29.     }
    30. }
    31.  
    32. public class PolarDEM : DEMStructure
    33. {
    34.     public PolarDEM(DEMData data) : base(data) { }
    35.  
    36.     public override Vector2 GetPixelCoordinates(Vector2 coordinates)
    37.     {
    38.         return Vector2.zero;
    39.     }
    40.  
    41.     public override bool IsInsideSquare(Vector2 minBorderCoordinates, Vector2 maxBorderCoordinates)
    42.     {
    43.         return false;
    44.     }
    45. }
    I have removed all the computation to keep it very simple. The only thing you have to know is I need the data to perform the computation in the GetPixelCoordinates method.

    Here is my problem, it means I need to always call the data variable and in the data class add a public getter for all the field.

    My question is, is there a better way to make this?

    To resume, I have a ScriptableObject which handle the data of the DEMs. There are 2 types of DEMs, Polar and Cylindrical. Some methods can be applied on both like getting two dimensional float array and some others need to be different like getting the pixel coordinates.
     
  2. CodeSmile

    CodeSmile

    Joined:
    Apr 10, 2014
    Posts:
    5,907
    You have 2 of these. And you think an if/else won't scale. So ... how many more types of heightmap do you expect there to be? Three? Four?

    Unless we're talking at least half a dozen, you're not improving your coding skill, you're trying to discipline yourself to a paradigm (OOP) for no good reason and you're going to write wayyyyy more code than simply adding another else if.

    Just if/else until it becomes unsuitable, then refactor. ;)
     
    Kurt-Dekker, Ryiah and Bunny83 like this.
  3. CodeRonnie

    CodeRonnie

    Joined:
    Oct 2, 2015
    Posts:
    530
    I don't see that as a problem. It's often good practice to treat your ScriptableObjects as if they are pure data objects. Their single responsibility is to serialize data configurations as assets in the editor. You can add transient state and behavior to them, but that can often invite strange situations later. If you treat them as pure serialized data assets, and pass them into something like a pure C# class, instantiated at runtime as you have done, then you eliminate the possibility of something at runtime corrupting the other transient state data of that asset between play sessions for example.

    So, it often will be necessary to expose the ScriptableObject fields, at least via getter properties, so that your runtime scripts can access that data. There's nothing wrong with that. If you only want your scripts to have readonly access, then you can serialize the private field, and have a get only property. Or, you can use the [SerializeField] attribute on a property with the field attribute target to serialize the backing field of a property like this.
    Code (CSharp):
    1. [field:SerializeField] public Texture2D ImageHeightMap { get; private set; }
    2. [field:SerializeField] public float MeterPerPixel { get; set; }
    And I would use PascalCase and capitalize the first letter of class members. When I see something camelCase used within a method, I automatically think it must be a local variable. One exception I see is when using a property and an explicit backing field, where the backing field is preceded by an underscore, like _backingField. However, I've started using an underscore and PascalCase in that situation because it's just a convention, so I may as well do what I like. This whole paragraph is about preference. When you have to do some other logic in your property setter or getter you'll have to split up the declarations.
    Code (CSharp):
    1. // I have other code in Update() that continues to synchronize these two properties across two different objects whether there be live changes in the inspector, or via script.
    2. // This is just an example of a complex property/backing field combo declaration.
    3. public int FileSizeLimit { get => _FileSizeLimit; set { if(LogFileWriter != null) LogFileWriter.FileSizeLimit = value; _FileSizeLimit = value; } }
    4. [SerializeField][Delayed][Min(LogFileWriter.MaxBytesPerLog)] private int _FileSizeLimit = LogFileWriter.DefaultFileSizeLimit;
     
    Last edited: Feb 8, 2024
    Magnilum likes this.
  4. Magnilum

    Magnilum

    Joined:
    Jul 1, 2019
    Posts:
    241
    What I wanted to say is, I have other projects where I have a lot more of different types. I wanted to use this case as a training to know when should I use interfaces, inheritance or abstract class.

    I don't want to just put if/else statement untill it is not readable anymore. Of course it will scale but with hundreds lines more. I don't want to do this like a beginner, I want to learn the good practice and not rewritte my code each time I want to add something like the laws of coding say.
     
  5. spiney199

    spiney199

    Joined:
    Feb 11, 2021
    Posts:
    7,883
    Well the point of using either a base class or an interface is substitution. Used properly, you don't have to care about what derived/specific implementation of a given base class or interface you're using.

    If you have to check for specific types, then at that point you have failed to use either concept and have created an anti-pattern.

    What you probably need is some sort of system that can factory these CylindricalDEM. Perhaps with scriptable objects?

    Code (CSharp):
    1. public abstract class DEMFactoryBase : ScriptableObject
    2. {
    3.     public abstract DEMStructure GetDEMStructure(DEMData data);
    4. }
    5.  
    6. [CreateAssetMenu]
    7. public sealed class DEMFactoryCylindrical : DEMFactoryBase
    8. {
    9.     public override DEMStructure GetDEMStructure(DEMData data)
    10.     {
    11.         return new CylindricalDEM(data);
    12.     }
    13. }
    Just need a reference to one, then you can give it the required data, it gives you an instance, and you can use it regardless of what specific implementation of DEMStructure it gives you.
     
  6. Magnilum

    Magnilum

    Joined:
    Jul 1, 2019
    Posts:
    241
    Thanks a lot for these examples and explanations.

    I knew that it was possible to do this but I never used them since I thought it was not really good in convention or something else.

    From what you said, it is good to see the ScriptableObject as pure data handler and use them into other classes to treat and use these data.

    In my case, a DEM class to stores the data and a DEM Structure class to treat and use the data through it.

    You said, pure data serializer, do you mean that you never put methods inside ScriptableObject, only property fields?
     
  7. Magnilum

    Magnilum

    Joined:
    Jul 1, 2019
    Posts:
    241
    With this method, it means that I create two separated ScriptableObjects for Polar and Cylindrical. So in the create asset menu I will have DEM/Polar or DEM/Cylindrical.

    I wanted to have only a DEM as ScriptableObject and in the code determine if it is a Polar or a Cylindrical one by looking at the latitude.

    May be it is not the way I should go with, I don't know that is why I am asking.
     
  8. spiney199

    spiney199

    Joined:
    Feb 11, 2021
    Posts:
    7,883
    I mean you probably only ever need one instance of them, so you can create them, and then remove the Asset menu.

    In any case it was just an example. You could do the same thing with plain C# classes and SerializeReference if you have the inspector tooling for it, and thus don't need to make scriptable object instances for the factories.
     
  9. Magnilum

    Magnilum

    Joined:
    Jul 1, 2019
    Posts:
    241
    When I say only one DEM, I wanted to say only one class but many objects. I want to a have only one DEM class as ScriptableObject to handle data but I will have many of them since I have 234 DEMs. I just prefer not to separate them into 2 differents types when there are created since I want to keep it simple for the future users.

    I thing it was not clear, sorry.

    Cause I thought to the example you gave me and it is a very goos one which I use for my game but in this case, it does't work for the reasons I wrote.
     
  10. spiney199

    spiney199

    Joined:
    Feb 11, 2021
    Posts:
    7,883
    I mean it's still possible with the one scriptable object class. You just need the configuration to be in the plain C# land, serialised into the scriptable object.

    SerializeReference makes a lot of stuff like this possible.
     
    Magnilum likes this.
  11. Magnilum

    Magnilum

    Joined:
    Jul 1, 2019
    Posts:
    241
    So here is what I have done, I let you tell me if it is good or better can be done.

    I have created my ScriptableObject class DEMData:

    Code (CSharp):
    1. using UnityEngine;
    2.  
    3. [CreateAssetMenu(fileName = "New DEM", menuName = "Tools/Terrain Generator/DEM")]
    4. public class DEMData : ScriptableObject {
    5.  
    6.     public static float scale = 0.5f;
    7.  
    8.     [SerializeField] ComputeShader compute;
    9.  
    10.     [Header("Image")]
    11.     [SerializeField] Texture2D heightmap;
    12.     [SerializeField] float meterPerPixel;
    13.     [SerializeField] float minValue;
    14.  
    15.     [Header("Coordinates Range")]
    16.     [SerializeField] Vector2 latitudeRange;
    17.     [SerializeField] Vector2 longitudeRange;
    18.  
    19.     #region Getter & Setter
    20.  
    21.     public ComputeShader Compute => compute;
    22.     public Texture2D Heightmap => heightmap;
    23.     public float MeterPerPixel => meterPerPixel;
    24.     public float MinValue => minValue;
    25.     public Vector2 LatitudeRange => latitudeRange;
    26.     public Vector2 LongitudeRange => longitudeRange;
    27.  
    28.     #endregion
    29.  
    30.     public bool IsCylindrical()
    31.     {
    32.         return latitudeRange.x >= -Coordinates.polesLatitude && latitudeRange.y <= Coordinates.polesLatitude;
    33.     }
    34.  
    35.     public bool IsPolar()
    36.     {
    37.         return !IsCylindrical();
    38.     }
    39.  
    40.     public DEMStructure GetStructure()
    41.     {
    42.         return IsCylindrical() ? new CylindricalDEM(this) : new PolarDEM(this);
    43.     }
    44. }
    Then I have made the DEMStructure, may be another name would be more suitable.

    Code (CSharp):
    1. using UnityEngine;
    2. using System.Collections.Generic;
    3.  
    4. public abstract class DEMStructure {
    5.  
    6.     public static float scale = 0.5f;
    7.  
    8.     public DEMData data { get; private set; }
    9.  
    10.     public DEMStructure(DEMData data)
    11.     {
    12.         this.data = data;
    13.     }
    14.  
    15.     public bool Contains(Vector2 coordinates)
    16.     {
    17.         return true;
    18.     }
    19.  
    20.     public void GetHeightMap(ref float[,] heightmap, float size, Vector2 worldPos, Vector2 coordinates, bool skipInside, IInterpolation interpolation, Crater[] craters = null)
    21.     {
    22.         return heightmap;
    23.     }
    24.  
    25.     public float GetHeight(int x, int y)
    26.     {
    27.         if (x < 0 || x >= data.Heightmap.width) return 0;
    28.         if (y < 0 || y >= data.Heightmap.height) return 0;
    29.  
    30.         return ColorToHeight(data.Heightmap.GetPixel(x, y));
    31.     }
    32.  
    33.     float ColorToHeight(Color color)
    34.     {
    35.         return (color.r * 65025 + color.g * 255 + data.MinValue) * scale; // 255 * 255 = 65025
    36.     }
    37.  
    38.     public abstract Vector2 GetPixelCoordinates(Vector2 coordinates);
    39.     public abstract bool IsInsideSquare(Vector2 minBorderCoordinates, Vector2 maxBorderCoordinates);
    40. }
    41.  
    42. public class CylindricalDEM : DEMStructure
    43. {
    44.     public CylindricalDEM(DEMData data) : base(data) { }
    45.  
    46.     public override Vector2 GetPixelCoordinates(Vector2 coordinates)
    47.     {
    48.         return Vector2.zero;
    49.     }
    50.  
    51.     public override bool IsInsideSquare(Vector2 minBorderCoordinates, Vector2 maxBorderCoordinates)
    52.     {
    53.         return false;
    54.     }
    55. }
    56.  
    57. public class PolarDEM : DEMStructure
    58. {
    59.     public PolarDEM(DEMData data) : base(data) { }
    60.  
    61.     public override Vector2 GetPixelCoordinates(Vector2 coordinates)
    62.     {
    63.         return Vector2.zero;
    64.     }
    65.  
    66.     public override bool IsInsideSquare(Vector2 minBorderCoordinates, Vector2 maxBorderCoordinates)
    67.     {
    68.         return false;
    69.     }
    70. }
    So from the DEMData class I can call the GetStructure method and use it as my main class to use the data.

    Does it seem right to you?