search

jeffcampbellmakesgames / Entitas-Redux

An entity-component framework for Unity with code generation and visual debugging
MIT License
102 stars 13 forks source link

[FEATURE REQUEST] IPreInitializeSystem, for initialization that must be done before Initialize() but can't be done in a constructor #32

Closed JesseTG closed 3 years ago

JesseTG commented 3 years ago

Is your feature request related to a problem? Please describe.

I have all of my Systems set up as ScriptableObjects. One major upside is that I can configure my Systems in the Unity editor; instead of having to screw around with dependency injection, I just add some fields. It's very designer-friendly, which is good because I don't expect to be a solo dev forever.

There are disadvantages, however. Chief among them is that the ScriptableObject life cycle is more complicated than that of a plain C# object (like Systems usually are). It's difficult to simply construct a system and throw it away like I regularly do in my test suite. I work around this by heavily using IInitializeSystem and ITeardownSystem. Problem solved; now I don't need to figure out when exactly Awake(), OnEnable(), OnDestroy(), or OnDisable() are called (and on which systems).

A related disadvantage is that I can't initialize systems with constructors; generally, this is where you would run logic that needs to be done before Initialize(), like setting up Collectors or Groups. I set those objects up in Initialize()...and then hit a weird bug in my code.

I needed to create some Entitys within Initialize(). This worked fine. However, I expected them to be tracked by a Collector on the first frame of execution; this didn't happen because the Entitys were being created before the Collector was, so my customized ReactiveSystem didn't notice them.

This wouldn't have happened if I could guarantee that there was some kind of set up phase that occurred before any Entity was created.

Describe the solution you'd like

Simple; an IPreInitializeSystem that looks something like this:

public interface IPreInitializeSystem
{
    void PreInitialize(IContexts contexts);
}

By convention, Systems in Entitas have a unary constructor that takes a Contexts as an argument. The system might then store the Contexts and use it to create Groups or Collectors (like in ReactiveSystems). Since I can't use constructors, this interface would be the next best thing.

The interface takes an IContexts because Contexts is usually generated in each project; the expectation is that you would cast it inside your implementation of PreInitialize().

There's a secondary advantage, too. Since my ScriptableSystems can't use constructors, I have to pass around a singleton with a Contexts everywhere. This interface would eliminate that need, simplifying the process of implementing Systems as ScriptableObjects.

I don't know what use IPreInitializeSystem would have in POCO-based Systems, but I can't see it getting in the way. It doesn't place any more burden on the developer than the other phases. Game devs are creative, I'm sure you'll figure something out.

Describe alternatives you've considered See the first three paragraphs.

Additional context I'm still using vanilla Entitas but I figured Redux would benefit from this idea, hence my feature request.

jeffcampbellmakesgames commented 3 years ago

From what I understand, the problem is that there is a race condition between two systems with the IInitializeSystem method; one creating entities and the other creating a collector that expects those entities to be created both on the same frame and after this latter system is initialized. I understand what you're saying about not having a reliable constructor or init method for a ScriptableObject and it smells a bit like this might be more of a race condition issue than it is something specific to implementing a system as a ScriptableObject, though this is definitely good conversation for (https://github.com/jeffcampbellmakesgames/Entitas-Redux/issues/12) which also discusses scriptable features, systems.

This is a simplified order of operations for systems based on my interpretation of how you've described it.

These are the proposed changes:

A couple of questions/thoughts to kickstart a conversation:

jeffcampbellmakesgames commented 3 years ago

I don't have any sort of a plugin or PR yet for this kind of approach as well, but on my side-project I've handled this in a way that might be analogous to what you're thinking of here. This is a slightly modified version to fit more to your proposed use-case. In a similar way as to the role IPreInitializeSystem might play for a common way to provide an IContexts instance, ScriptableSystemBase.SetContexts could also be overriden to be used as a pseudo pre-initialization step. I'm curious to hear your thoughts given that you're working to ship a game largely using scriptable systems. 

/// <summary>
/// Abstract base scriptable system which can be serialized on <see cref="ScriptableObject"/> or <see cref="MonoBehaviour/>.
/// </summary>
public abstract class ScriptableSystemBase : ScriptableObject, ISystem 
{
    [NonSerialized]
    protected IContexts _contexts;

    /// <summary>
    /// Sets the <see cref="IContexts "/> contexts for this system.
    /// </summary>
    /// <param name="contexts">The contexts this system will be initialized with.</param>
    public virtual void SetContext(T contexts)
    {
        _contexts= contexts;
    }
}

/// <summary>
/// A scriptable representation of a feature for a specific <see cref="IContexts"/>.
/// </summary>
public abstract class ScriptableFeatureBase : ScriptableObject
{
    [SerializeField]
    protected abstract List<ScriptableSystemBase> _systems;

    [SerializeField]
    protected string _name;

    [NonSerialized]
    protected Feature _feature;

    /// <summary>
    /// Returns the name of this feature.
    /// </summary>
    public virtual string GetFeatureName()
    {
        return string.IsNullOrEmpty(_name) ? name : _name;
    }

    /// <summary>
    /// Returns the feature for the contained systems.
    /// </summary>
    /// <param name="contexts">The contexts each system will be initialized with.</param>
    public Feature GetFeature(IContexts contexts)
    {
        if (_feature == null)
        {
            _feature = new Feature(GetFeatureName());
            for (var i = 0; i < systems.Count; ++i)
            {
                systems[i].SetContexts(contexts);
                _feature.Add(systems[i]);
            }
        }

        return _feature;
    }
}
JesseTG commented 3 years ago

From what I understand, the problem is that there is a race condition between two systems with the IInitializeSystem method; one creating entities and the other creating a collector that expects those entities to be created both on the same frame and after this latter system is initialized.

You understand correctly. Also "race condition" is probably not a bad term for it; but to be clear, there's no multithreading happening here. This is a matter of sequencing, not timing.

I understand what you're saying about not having a reliable constructor or init method for a ScriptableObject and it smells a bit like this might be more of a race condition issue than it is something specific to implementing a system as a ScriptableObject, though this is definitely good conversation for (#12) which also discusses scriptable features, systems.

This is a simplified order of operations for systems based on my interpretation of how you've described it.

  • IInitializeSystem
    • System A creates several entities
    • System B creates a collector, but isn't able to collect those entities as they existed beforehand.

These are the proposed changes:

  • IPreInitializeSystem
    • System A does nothing
    • System B creates a collector
  • IInitializeSystem
    • System A creates several entities
    • System B's collector tracks these new entities and is able to do something with them.

Yes, this is an accurate understanding.

A couple of questions/thoughts to kickstart a conversation:

    • What order are these systems being initialized in; is the above order of operations correct? Is this solved by re-ordering System B to be initialized before System A? That would also result in the collector being created before these entities. This feels appropriate given that System B depends on System A performing work before it can properly work and would likely work as a solution/workaround.

In my particular case, rearranging the offending systems solved my problem. But I'm not certain that it always will; if such arrangements result in Features whose organization isn't obvious, I consider that a loss.

    • With regards to the part of the interface IPreInitializeSystem receiving a IContexts, is the chief problem this is solving having a native path in the framework for passing all relevant context instances in a IContexts to a system implemented as a ScriptableObject? You're intuition is right in that this might be redundant for POCO based systems where constructors are a more common pathway that also allow for a pseudo pre-initialization step. I'm not sure that an additional system interface is the right level of solution for this kind of problem, but I would like to have something like this for scriptable systems and features as part of an EntitasRedux feature for Unity.

Well, the reason I suggested a separate interface is that those who don't need it just don't use it. The addition of IPreInitializeSystem should not change any existing code.

I don't have any sort of a plugin or PR yet for this kind of approach as well, but on my side-project I've handled this in a way that might be analogous to what you're thinking of here. This is a slightly modified version to fit more to your proposed use-case. In a similar way as to the role IPreInitializeSystem might play for a common way to provide an IContexts instance, ScriptableSystemBase.SetContexts could also be overriden to be used as a pseudo pre-initialization step. I'm curious to hear your thoughts given that you're working to ship a game largely using scriptable systems.

/// <summary>
/// Abstract base scriptable system which can be serialized on <see cref="ScriptableObject"/> or <see cref="MonoBehaviour/>.
/// </summary>
public abstract class ScriptableSystemBase : ScriptableObject, ISystem 
{
  [NonSerialized]
  protected IContexts _contexts;

  /// <summary>
  /// Sets the <see cref="IContexts "/> contexts for this system.
  /// </summary>
  /// <param name="contexts">The contexts this system will be initialized with.</param>
  public virtual void SetContext(T contexts)
  {
      _contexts= contexts;
  }
}

/// <summary>
/// A scriptable representation of a feature for a specific <see cref="IContexts"/>.
/// </summary>
public abstract class ScriptableFeatureBase : ScriptableObject
{
  [SerializeField]
  protected abstract List<ScriptableSystemBase> _systems;

  [SerializeField]
  protected string _name;

  [NonSerialized]
  protected Feature _feature;

  /// <summary>
  /// Returns the name of this feature.
  /// </summary>
  public virtual string GetFeatureName()
  {
      return string.IsNullOrEmpty(_name) ? name : _name;
  }

  /// <summary>
  /// Returns the feature for the contained systems.
  /// </summary>
  /// <param name="contexts">The contexts each system will be initialized with.</param>
  public Feature GetFeature(IContexts contexts)
  {
      if (_feature == null)
      {
          _feature = new Feature(GetFeatureName());
          for (var i = 0; i < systems.Count; ++i)
          {
              systems[i].SetContexts(contexts);
              _feature.Add(systems[i]);
          }
      }

      return _feature;
  }
}

This is actually similar to what I do in my existing code, except for the presence of SetContexts; I believe having it as a separate initialization phase would make it easier to control the timing of whatever happens on the inside. Your SetContexts() is basically used the same way I envision IPreInitializeSystem to be.

jeffcampbellmakesgames commented 3 years ago

From what I understand, the problem is that there is a race condition between two systems with the IInitializeSystem method; one creating entities and the other creating a collector that expects those entities to be created both on the same frame and after this latter system is initialized.

You understand correctly. Also "race condition" is probably not a bad term for it; but to be clear, there's no multithreading happening here. This is a matter of sequencing, not timing.

Good, glad we're on the same page :) A race condition as a concept isn't specific to threading though it often comes up with regards to syncing output or work between threads; it just means X system did something before Y system needed it to and caused a bug as a result.

A couple of questions/thoughts to kickstart a conversation:

    • What order are these systems being initialized in; is the above order of operations correct? Is this solved by re-ordering System B to be initialized before System A? That would also result in the collector being created before these entities. This feels appropriate given that System B depends on System A performing work before it can properly work and would likely work as a solution/workaround.

In my particular case, rearranging the offending systems solved my problem. But I'm not certain that it always will; if such arrangements result in Features whose organization isn't obvious, I consider that a loss.

Well, the reason I suggested a separate interface is that those who don't need it just don't use it. The addition of IPreInitializeSystem should not change any existing code.

There are more implications and work to adding a system interface than just it's immediate usage, which is why I'm cautious at adding more without ensuring it covers a well-suited niche. The visual debugging feature in the Unity Editor shows/tracks performance based on the system interfaces it implements (along with the underlying infrastructure to support it) and in the future a more comprehensive performance tracking feature for device profiling will also do this. Even when unused, there is a significant amount of effort to add the interface for the framework across multiple features more than just the wiring it into Systems. In particular because this problem derives from a race condition between two systems initialization method calls that is easily solved re-ordering them or any number of other ways, it doesn't smell like a system level niche.

This is actually similar to what I do in my existing code, except for the presence of SetContexts; I believe having it as a separate initialization phase would make it easier to control the timing of whatever happens on the inside. Your SetContexts() is basically used the same way I envision IPreInitializeSystem to be.

The way we've discussed this with relation to POCOs is that its an equivalent to a constructor, but for a ScriptableObject. In this way, I think we can achieve the goal of this issue by providing better native framework support for ScriptableObjects in general which would include providing a overridable equivalent initialization step that would always execute before any other system interfaces would be executed, including IInitializationSystem. In this way there is parity between POCO and scriptable systems that doesn't rise to the level of a system interface.

As far as next steps, what I would do is to create a new issue to cover all of the various aspects scriptable systems, features would entail. I'll link that here when it's ready.

jeffcampbellmakesgames commented 3 years ago

Created issue https://github.com/jeffcampbellmakesgames/Entitas-Redux/issues/35 to better handle describing implementation and tasks for scriptable systems and features.

jeffcampbellmakesgames commented 3 years ago

Closing this issue in favor of the one linked here (https://github.com/jeffcampbellmakesgames/Entitas-Redux/issues/35).

JesseTG commented 3 years ago

Actually, wait, I have another use case that isn't necessarily tied to ScriptableObject-backed systems.

Injecting dependencies that must be processed within Initialize(), but are not available at the time of construction. To make it more general, let's call it IInjectSystem and make it look like this:

public interface IInjectSystem : ISystem
{
    void Inject(object obj);
}

obj could be a Contexts, but it doesn't have to be. It could also be:

It would be each system's responsibility to use Inject() to look at the given obj and retrieve whatever information it needs. Here's what an implementation pulled from my own code looks like:

public void Inject(object obj)
{
    switch (obj)
    {
        case ITest test:
            _test = test;
            break;
        case Contexts contexts:
            _contexts = contexts;
            break;
        case EntityCommandSequence commands:
            _commands = commands;
            break;
    }
}

For null or objects of any other type, the system simply does nothing because it doesn't need it.