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

[Entitas:] Find alternative to pool.CreateSystem() #233

Closed
sschmid opened this issue Dec 12, 2016 · 8 comments
Closed

[Entitas:] Find alternative to pool.CreateSystem() #233

sschmid opened this issue Dec 12, 2016 · 8 comments
Assignees
Milestone

Comments

@sschmid
Copy link
Owner

sschmid commented Dec 12, 2016

PoolExtension.cs contains the convenience methods pool(s).CreateSystem()

3 things:

  1. CreateSystem is still the old method name from older versions, where this method actually created a system. It doesn't anymore, instead we pass in a created system so the name is wrong.
  2. It might sound misleading, because it sounds like the pool 'owns' the system (which it doesn't)
  3. This:
return new Feature("Systems")
            .Add(pools.CreateSystem(new IncrementTickSystem()))
            ...
            .Add(pools.core.CreateSystem(new AddViewSystem()))

Sometimes we use pools.CreateSystem(), sometimes pools.core.CreateSystem(). Would be great to streamline this.

@sschmid
Copy link
Owner Author

sschmid commented Dec 12, 2016

What about sth like this

.Add(new IncrementTickSystem().Bind(pools))
.Add(new AddViewSystem().Bind(pools.core))
.Add(new AddViewSystem().Bind(pools.bullet))

Bind (or a different term? Suggestions?)) actually does the same thing as CreateSystem. If a system implements ISetPoolsthen pass pools in, if it implements ISetPool then pass the pool in, if it doesn't implement either, do nothing (same behaviour as it is right now).

This might be less confusing and more explicit. Sounds more like we're using this system with the specified pool

@sschmid
Copy link
Owner Author

sschmid commented Dec 12, 2016

Any suggestions other than .Bind(core)?
.Use(core)
.Inject(core)
.With(core)
.Link(core)

This will most likely be an extension method on ISystem

@sschmid
Copy link
Owner Author

sschmid commented Dec 13, 2016

UPDATE: new idea (forget .Bind())

Since this might be a big change I'd like to have your opinion on that.

2 thoughts:

The goal is to simplify the creation of system lists. I'm thinking about removing ISetPool and ISetPools. They spell almost the same I guess people get easily confused what it is and what the differences are.

Instead, use pools only and pass them in as a constructor argument if needed.

Currently CreateSystem sets the pools if ISetPools is implemented. It also wraps IReactiveSystem in a ReactiveSystem.
I'm thinking of removing CreateSystem and move the reactive system wrapping to the systems.Add() method.

As a result we'd write sth like this:

        return new Feature("Systems")

            .Add(new IncrementTickSystem(pools))
            .Add(new CreatePlayerSystem(pools))
            .Add(new CreateEnemySystem(pools))

            .Add(new AddViewSystem(pools))
            .Add(new AddViewFromObjectPoolSystem(pools))

compared to the current solution:

        return new Feature("Systems")

            .Add(pools.CreateSystem(new IncrementTickSystem()))
            .Add(pools.CreateSystem(new CreatePlayerSystem()))
            .Add(pools.CreateSystem(new CreateEnemySystem()))

            .Add(pools.core.CreateSystem(new AddViewSystem()))
            .Add(pools.bullets.CreateSystem(new AddViewFromObjectPoolSystem()))

I would need to add some mechanism to define the pool which a reactive sub system is working on to make this work, but this could be achieved by modifying IReactiveSystem

public interface IReactiveSystem : IReactiveExecuteSystem {
    TriggerOnEvent trigger { get; }
    Pool Bind(Pools pools);
}

You basically hardcode the reactive system to a pool. We kind of doing this anyway already by using {Pool}Matcher as a trigger.

Any thoughts before I get started?

@sschmid sschmid added the rfc label Dec 13, 2016
@ghost
Copy link

ghost commented Dec 13, 2016

I don't like the idea of adding in Pools like that because what if I wanted to reuse the system for a specific pool of the same type (referring to type safety branch) I would prefer the instance of the pool not a class with pools in to access.

@sschmid sschmid changed the title Find alternative to pool.CreateSystem() [Entitas:] Find alternative to pool.CreateSystem() Dec 13, 2016
@sschmid
Copy link
Owner Author

sschmid commented Dec 14, 2016

@T2RKUS Yes, that's the only thing I have in mind too. If we move in this direction Pools becomes more than just sugar but a first class citizen. I'll think about it but I can imagine that every game will have more than one pool, so moving away from pool to pools might not be that bad.

@MoritzVossKing
Copy link

Why "pools"?

Doesn't a system live in one pool, and that's it? (yes, it could technically have a dependency on multiple pools, but that feels a little smelly, as CreateSystem is a member of Pool.)

ISetPool is very repetitive indeed, about 30-70% of my systems implement ISetPool. Any chance this could be done through inheritance instead? (I generally find inheritance highly underrated).

I often use SetPool to just create a group which I'm using throughout the life of an IExecuteSystem, etc.

@MoritzVossKing
Copy link

Oh, I understand and was misled all the time by the things you called out as misleading in the first post. 👍

@sschmid sschmid self-assigned this Dec 14, 2016
@sschmid
Copy link
Owner Author

sschmid commented Dec 14, 2016

It's the end of the year and a good time to clean up a little bit ;)

I updated IReactiveSystem

    public interface IReactiveSystem : ISystem {
        EntityCollector GetTrigger(Pools pools);
        void Execute(List<Entity> entities);
    }

This change enabled me to get rid of loooots of code . It's a breaking change but I hope the result is less confusion and less code to maintain.

I deleted:

  • IMultiReactiveSystem
  • TriggerOnEvent
  • IMatcherExtensions
  • .CreateSystem()

... without loosing any features. The new IReactiveSystem can do the same as IMultiReactiveSystem and IEntityCollectorSystem

Adding systems to the list is more streamlined and less code

.Add(new IncrementTickSystem(pools))

sschmid added a commit that referenced this issue Dec 14, 2016
# Conflicts:
#	Entitas/Entitas/Interfaces/IReactiveSystem.cs
@ghost ghost removed the needs review label Dec 14, 2016
@sschmid sschmid added this to the 0.36.0 milestone Jan 6, 2017
@sschmid sschmid added this to Entitas Jul 2, 2023
@github-project-automation github-project-automation bot moved this to Todo in Entitas Jul 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

No branches or pull requests

2 participants