Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ScriptableObject-Architecture 2.0 using new serialization method #100

Open
DanielEverland opened this issue Nov 28, 2019 · 9 comments
Open
Assignees
Labels
enhancement New feature or request refactoring Requires extensive refactoring. Might not be implemented
Milestone

Comments

@DanielEverland
Copy link
Owner

DanielEverland commented Nov 28, 2019

SO-A's codebase is extremely cluttered, which makes introducing new features tedious. Much of this clutter is due to hacky workarounds in order to appease Unity's built-in serialization. This doesn't have to be the case, as projects such as Odin serializer allows for much more flexible serialization.

The main benefit of this are as follows

  • Serialize most types from a single class.
  • Serialize generics and interfaces directly

This might seem trivial, but I estimate you'd be able to reduce the size of the codebase, not to mention its complexity, simply by fixing these two issues.

@DanielEverland DanielEverland added the enhancement New feature or request label Nov 28, 2019
@DanielEverland DanielEverland added this to the Release 2.0 milestone Nov 28, 2019
@DanielEverland DanielEverland self-assigned this Nov 28, 2019
@jzapdot
Copy link
Contributor

jzapdot commented Dec 3, 2019

Can you expound upon what clutter or workarounds you're referring to that would make you want to change how fields are serialized? I've never thought of this library as having any of these types of issues. As an example of a library similar to this one that relies heavily on generics/interfaces and is a lot harder to add new features to I'd look at https://github.com/AdamRamberg/unity-atoms

@jzapdot
Copy link
Contributor

jzapdot commented Dec 3, 2019

My concern is mostly that rewriting the code-base to leverage a third-party serializer at first glance seems like overkill for the functionality this library provides and adds a pretty hefty dependency for both the library and its development. I'm open to hearing more about why this is desirable though!

@DanielEverland
Copy link
Owner Author

I hear what you're saying, and I appreciate the feedback greatly. It might not be a good idea, I'm not sold on doing it either, it's just been a bit of a thorn in my side for a while. As an example, here's a snippet:

We have our generic BaseVariable, which works about as you'd expect

public abstract class BaseVariable<T> : BaseVariable
{
}

This in turn inherits a non-generic BaseVariable class

public abstract class BaseVariable : GameEventBase
{
    public abstract bool IsClamped { get; }
    public abstract bool Clampable { get; }
    public abstract bool ReadOnly { get; }
    public abstract System.Type Type { get; }
    public abstract object BaseValue { get; set; }
}

I honestly don't remember why this is the case. By glancing at the code it certainly doesn't make sense, given that variables must belong to some sort of type, so what's the point in having a type-less base class? I can only assume this is some Unity limitation, semantically I can't really wrap my head around it.

Furthermore, this typeless base class inherits GameEventBase - that doesn't make much sense semantically either, given that a variable is certainly not an event. This is because I wanted the functionality of being able to subscribe to a variable, in order to raise events whenever the variable's value changes. This highlights another limitation, though: this doesn't work with types. So you're not able to subscribe to float events, this will only work if your even listener is a GameEventListener, meaning that you can't pass the new value along. This isn't a big deal, though, you should still be able to subscribe to FloatVariables, you just have to fetch the value yourself.

The hierarchy continues, because GameEventBase subscribes to SOArchitectureBaseObject, which contains the developer description.

public abstract class SOArchitectureBaseObject : ScriptableObject
{
#pragma warning disable 0414
    [SerializeField]
    private DeveloperDescription DeveloperDescription = new DeveloperDescription();
#pragma warning restore
}

This makes the codebase inflexible, since our variables inadvertently also started serializing developer descriptions, even though we really only want the event functionality. It happens to work out in this case, but that's simply by chance, not because I've engineered this in a sensible way.

Something this doesn't highlight is that you need to add code to your project in order to serialize new types. This obviously clutters the project unnecessarily i.e:
image

Which is unnecessary, since other serialization schemes handle generics properly.

TL;DR; The core of the codebase is structured in order to satisfy Unity, even though I'd prefer it to be structured in a completely different way. Switching serialization scheme might (I'm really not sure to be honest) allow more flexibility in this respect.

@jzapdot
Copy link
Contributor

jzapdot commented Dec 4, 2019

Gotcha, I'm hearing a couple of concerns/issues here I'd like to try and summarize. Please let me know if I'm off-base or have misunderstood.

  • There are parts of the code-base that in hindsight we're not sure why they're written a certain way, whether this was done for a Unity-specific reason or otherwise.
  • There is the desire to be able to implement reactive (RX) style eventing on variables, but its inheritance hierarchy has limited this to only being able to subscribe in such a way where its T type is not passed (due to the event-related code being in the base class GameEventBase that doesn't know about its derive Variable type).

Some specific points of feedback here.

This in turn inherits a non-generic BaseVariable class

public abstract class BaseVariable : GameEventBase
{
    public abstract bool IsClamped { get; }
    public abstract bool Clampable { get; }
    public abstract bool ReadOnly { get; }
    public abstract System.Type Type { get; }
    public abstract object BaseValue { get; set; }
}

I honestly don't remember why this is the case. By glancing at the code it certainly doesn't make sense, given that variables must belong to some sort of type, so what's the point in having a type-less base class? I can only assume this is some Unity limitation, semantically I can't really wrap my head around it.

Based on some of the past work I've contributed, I'm betting this was done in part to simplify the editor/inspector code since each type of thing (event, variable, collection, etc...) shares a single inspector. This is something we could potentially refactor or change if desired.

Something this doesn't highlight is that you need to add code to your project in order to serialize new types. This obviously clutters the project unnecessarily i.e:
image

Which is unnecessary since other serialization schemes handle generics properly.

TL;DR; The core of the codebase is structured in order to satisfy Unity, even though I'd prefer it to be structured in a completely different way. Switching serialization scheme might (I'm really not sure to be honest) allow more flexibility in this respect.

Couple of key things to know about Unity's serialization scheme for ScriptableObject and Monobehavior.

  • In order for a ScriptableObject to be able to be created in the editor as an asset, it needs to live in a file named the same (case-sensitive) as the class and needs to be the first class in the file. For the ScriptableObject derived classes, this is why they are all in separate files. Monobehavior has the same restrictions to be able to be added as a component, which is why they'd also be in separate files.
  • For abstract ScriptableObject or Monobehavior classes, these restrictions don't apply since they don't need to be instantiated in any sort of way; their derived non-abstract versions only have to comply with that restriction.

TLDR; I'd be happy to help attack some of these issues/concerns if you like and break them out into separate pieces of work. I've run into the subscribe to Variable issues you've described and have worked around it and am familiar with some of the challenges you're facing there. Perhaps it would help to reflect and share some of the development goals you have for this library in a short, bulleted format and we can use that as a basis for working towards those.

@4quet
Copy link

4quet commented Apr 1, 2020

Hello !

I think this is the appropriate place to post this.
In Unity 2020.1.0a3 changelog there's a line :

Scripting: The serializer can now serialize fields of generic types (e.g. MyClass someField) directly;
it is no longer necessary to derive a concrete subclass from a generic type in order to serialize it.

This might resolve some of the issues you mentioned in this thread, but sadly it would be only compatible with Unity 2020.

@DanielEverland DanielEverland added the refactoring Requires extensive refactoring. Might not be implemented label Apr 19, 2020
@xvnnnk
Copy link

xvnnnk commented Apr 27, 2020

Odin Serializer is much slower than Unity. It will increase game's start up time which is no good for Mobile games (it will increase it up to 20 seconds in my experience). It will kill this repo if you add it.

@ThimoVBE
Copy link

Are there concrete plans on refactoring to the new Unity 2020.1 serialization possibilities and the variable events, editor/inspector code?

@DanielEverland
Copy link
Owner Author

@ThimoVBE I will look into this soon! I apologize for the late reply, I've been really busy this past year.

@DanielEverland
Copy link
Owner Author

It unfortunately only works with fields, so we won't be able to serialize something like a BaseVariable<float> into an asset directly. This means the derived classes pr. type are still required, so it doesn't really solve anything.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request refactoring Requires extensive refactoring. Might not be implemented
Projects
None yet
Development

No branches or pull requests

5 participants