Resolved Problem that seems to be related to the difference between CreateInstance and Instantiate()

Okay guyz, I'm looking for help to understand what am I doing wrong here, lemme me explain the problem

1. My most inner class was called GameAttribute, it has an integer grow the field and notifies changes through an event

Code (CSharp):

1.     public class GameAttribute
2.     {
3.         private int grow = 1;
4.  
5.         [PropertyOrder(-1)]
6.         [MinValue(1)]
7.         [OdinSerialize]
8.         public int Grow
9.         {
10.             get => grow;
11.             protected set
12.             {
13.                 grow = value;
14.  
15.                 Debug.Log($"NOTIFY on GAME ATTRIBUTE");
16.                 OnUpdatedGrow?.Invoke(grow);
17.             }
18.         }
19.        
20.         public event Action<int> OnUpdatedGrow;
21.     }

2. This GameAttribute is part of a GameAttributeSector that is my second layer, it actually has multiple GameAttributes and notifies changes on any of them through an event, it is linked with the GameAttribute events through the constructor. Actually, this link happens on the base class.

Code (CSharp):

1.     // BASE CLASS
2.     public abstract class GameAttributeChartSector : IEnumerable<GameAttribute>
3.     {
4.         public event Action OnUpdatedAccumulatedPoints;
5.        
6.         protected GameAttributeChartSector()
7.         {
8.             foreach (var gameAttribute in this)
9.                 gameAttribute.OnUpdatedGrow += _ =>
10.                 {
11.                     Debug.Log($"OnUpdatedAccumulatedPoints on sector: {GetType()}");
12.                     OnUpdatedAccumulatedPoints?.Invoke();
13.                 };
14.         }
15.  
16.         public abstract IEnumerator<GameAttribute> GetEnumerator();
17.         IEnumerator IEnumerable.GetEnumerator() => GetEnumerator();
18.     }
19.  
20.     // ACTUAL SECTOR
21.     public class StrengthSector : GameAttributeChartSector
22.     {
23.         [PropertyOrder(1)]
24.         [InlineIndentedPropertyWithTitle]
25.         [OdinSerialize]
26.         public GameAttribute AttackAttribute { get; protected set; } = new();
27.  
28.         public override IEnumerator<GameAttribute> GetEnumerator()
29.         {
30.             yield return AttackAttribute;
31.         }
32.     }

3. And finally we have the upper layer of abstraction, a GameAttributeChart that will contain multiple sectors and will trigger an event when changes on any GameAttribute from any GameAttributeSector happens using the same premise from Sector, BUT as this is a scriptable object to simplify the creation of multiple blueprints using inspector I can't use constructor, so I'm relying on the OnEnable message

Code (CSharp):

1.     public class GameAttributeChart : SerializedScriptableObject
2.     {
3.         [HideReferenceObjectPicker]
4.         [OdinSerialize]
5.         public StrengthSector StrengthSector { get; protected set; } = new();
6.        
7.         private void OnEnable()
8.         {
9.             StrengthSector.OnUpdatedAccumulatedPoints += () => OnChangeSpentPoints?.Invoke();
10.         }
11.  
12.         public event Action OnChangeSpentPoints;
13.     }

4. Now to finish the setup we have a HUD component that will get this chart and register on those events updating HUD on demand

Code (CSharp):

1.     [InfoBox("Let it null to create a new chart on setup")]
2.     [TitleGroup("Settings")]
3.     [OdinSerialize]
4.     private GameAttributeChart gameAttributeChart;
5.  
6.     [ShowInInspector]
7.     [OdinSerialize]
8.     private GameAttributeChart temporaryChart;
9.  
10.     [DisableInEditorMode]
11.     [Button]
12.     public void Setup()
13.     {
14.         // THIS IS WHERE THE PROBLEM IS
15.         temporaryChart = gameAttributeChart == null
16.             ? ScriptableObject.CreateInstance<GameAttributeChart>()
17.             : Instantiate(gameAttributeChart);
18.  
19.         remainingPointsHUD.Setup(temporaryChart);
20.  
21.         strengthSectorHUD.Setup(temporaryChart, temporaryChart.StrengthSector);
22.         vitalitySectorHUD.Setup(temporaryChart, temporaryChart.vitalitySector);
23.         dexteritySectorHUD.Setup(temporaryChart, temporaryChart.dexteritySector);
24.         intelligenceSectorHUD.Setup(temporaryChart, temporaryChart.intelligenceSector);
25.     }

Now the problem is when attributing a new instance to temporaryChart through ScriptableObject.CreateInstance<GameAttributeChart>() things work as expected and everything is okay, BUT if I set an attribute chart on HUD through the gameAttributeChart field making the temporaryChart receive a new instance through Instantiate(), it breaks all connect between layers, you can see this happening on the gif



Thanks for any help, I really not getting what I'm misunderstanding here. If some information is missing or unclear lemme know and I give you further information

OBS: When I set some breakpoints into GameAttributeSector.Strenght setter AND StrenghtSector.AttackAttribute setter I can see that it only goes there when I'm creating a new instance throught Instantiate, not when creating throught CreateInstance.

 

Well

CreateInstance

and

Instantiate

serve two entirely different purposes. For former makes a completely fresh, default instance, and the latter creates a copy of an existing instance.

Mind you I personally thinking making instances of scriptable objects leads is a bad idea. If you need to duplicate and share around what amounts to just data, do it with plain C# classes and save yourself the pain of dealing with the added weight around Unity objects.

 

About the problem, it seems that it has some kind of problem on the serialization step, if I change the sector property serialization from odin to unity serializing on field, things works as expected on both cases

Code (CSharp):

1.     [HideReferenceObjectPicker]
2.     [OdinSerialize]
3.     public StrengthSector StrengthSector { get; protected set; } = new();

TO

Code (CSharp):

1.     [field: HideReferenceObjectPicker]
2.     [field: SerializeField]
3.     public StrengthSector StrengthSector { get; protected set; } = new();

I'm not really sure why this has solved it, but at this point, I'm just accepting, if you know why this is the case I would love to know.

About plain class vs SO, I also prefer to keep it as a plain class, but if I go this path I lose the ability to design things on Inspector, I've been thinking in had an SO that contains just the plain class, generates APIs to just delegate to the inner class and using the InlineEditor of odin show it in the inspector, but every time that I change o create new things on the class I need to go back to the SO and delegate the new member. Do you have some third option? I would love to hear about it

And thanks for your reply =D

 

I wonder why you were Odin serialising it in the first place. If you didn't need polymorphism, there was no point in using it. If you needed polymorphism, you have SerializeReference. You really only need Odin serialisation if you absolutely need it, and in this case you didn't.

Have a plain C# class for the data. Then, have an SO that serialises an instance of this plain class. Rather than just exposing the class via the SO, you provide a method that returns a copy of the class.

I use this pattern regularly.

 

Some of those properties are non-auto properties and some have become non-auto properties after some refractories, so instead of keep remembering that I need to change the serializer when changing the property I just set it to the Odin, that in my mind would work with both cases, btw If it serializes when we need it why didn't serialize when we want?

Got it, I'm trying to apply law of demeter on my code lately, to see if I can get some benefits, and this kinda breaks it, but its a direct and good option, thanks, but talking about it, why should you avoid creating SO on runtime? I mean, the OnEnable message seems to be safe enough to cover a kinda "constructor" phase, and aside from this serialization problem that I got there it's pretty consistent, can you explain why you call it a bad idea?

 

[OdinSerialize]

working on properties is a convenience, and not what I would call requirement that entails using it. I would personally, as good practice, only ever serialise fields or the backing fields of properties.

I will reiterate that you should only use the Odin serialiser when absolutely necessary.

Because like all Unity objects instanced on the fly, you have to destroy it when no longer using it, or you get memory leaks. So I find it's better to not care about any of that, and just operate with plain C# classes, letting the garbage collector handle that for you.

Yes the prototype pattern that

Object.Instantiate()

allows is convenient, but heavy handed when dealing with packets of data.

I don't see how this breaks Law of Demeter as well. Your scriptable objects just become a factory for your data. They don't know about anything else.

 

Serializing only backing fields remove your ability to trigger things through get/set when changing values on the inspector but I got your point, I will just accept that something can goes wrong when serializing properties, maybe a better understanding about how the Odin serializer actually work could be useful for me now lol

Maybe we are talking about different things here, the SO will be just a wrapper for the composite plain class, so to access the plain class field method you will have something like - mySOReference.MyPlaingClassFieldReference.SomeMethod - and this isn't generating nor giving any object at runtime. Or were you talking about actually having a factory SO to provide different instances of your plain classes to ur system?

 

Ignore this, we can serialize the backing field and just expose the properties on inspector and we will have the same result

 

You're... you're using Odin Inspector! You have access to attributes like

[OnValueChanged]

to do this for you! There's no need to serialise properties for this purpose!

The idea is the SO's are just there to let you author data in the editor. Then at runtime you use these SO's to give you copies of these classes to use during runtime, after which they will disappear leaving the authored data untouched.

For example, I have scriptable objects in my project that define the behaviour (brains/AI, etc) for playable and non-playable entities. I want to be able to set values for these in the inspector, but as they maintain state I can't use the SO to run these.

So the So serialise the behaviour as so:

Code (CSharp):

1. [SerializeReference]
2. private ObjectBehaviourBase objectBehaviour = new ObjectBehaviourNone();

And when I need a runtime instance I just get the copy:

Code (CSharp):

1. public IObjectBehaviour GetObjectBehaviour(IObjectController objectController)
2. {
3.     return this._objectBehaviour.GetBehaviourInstance(objectController);
4. }

(Some dependency injection of sorts going on as well)

The actual serialised behaviour instance isn't exposed at all.

 

Isn't exactly the same thing right, to invoke events I need to wrap it inside a method before setup it and this will be called by the inspector only, code access to the field will still happen though the property, which means that different code runs when I change a field from the inspector and for code, this will be a mess to debug... but as I said upper, serializing the backing field and just exposing the property is a good option

You're totally right, I understood it wrong, I tough that you said to make the SO just as a bucket for the class and then the caller will be responsible to generate the new instance of the class, but making it an actual factory is insanelly better, I will definitely add this to my toolset, thanks for sharing the idea.

Just curious, did you manually clone each instance field from your serialized field?

 

Just... code it to do the same thing in either case. It's easy:

Code (CSharp):

1. public class GameAttribute
2. {
3.     [SerializeField, OnValueChanged(nameof(CallOnUpdatedGrow))]
4.     private int grow = 1;
5.  
6.     public int Grow
7.     {
8.         get => grow;
9.         protected set
10.         {
11.             grow = value;
12.             this.CallOnUpdatedGrow();
13.         }
14.     }
15.  
16.     public event Action<int> OnUpdatedGrow;
17.  
18.     private void CallOnUpdatedGrow()
19.     {
20.         OnUpdatedGrow?.Invoke(grow);
21.     }
22. }
23.  

You get the same result either way.

Yes, though things are well encapsulated that the boiler plate is minimal. And there are cheat ways such as using

Sirenix.Serialization.SerializationUtility.CopyObject(object)

.

 

Awesome, thanks ;P