sschmid / Entitas

Entitas is a super fast Entity Component System (ECS) Framework specifically made for C# and Unity
MIT License
7.13k stars 1.11k forks source link

Events aka Reactive-UI #591

Closed sschmid closed 6 years ago

sschmid commented 6 years ago

Add new Attributes, Data Providers and Code Generators to generate what we know under the name Reactive UI (as explained in this Unite Talk video https://www.youtube.com/watch?v=Phx7IJ3XUzg)

[Event(false)]
public sealed class GameOverComponent : IComponent {
}

[Event(true)]
public sealed class PositionComponent : IComponent {
    public Vector3 value;
}

I plan to generate 2 different events, one that's independent from entities [Event(false)] and one that's bound to a specific entity [Event(true)]

public interface IGameOverListener {
    void OnGameOver(bool isGameOver);
}

public sealed class GameOverListenerComponent : IComponent {
    public IGameOverListener value;
}

public sealed class GameOverEventSystem : ReactiveSystem<GameEntity> {

    readonly IGroup<GameEntity> _listsners;

    public GameOverEventSystem(Contexts contexts) : base(contexts.game) {
        _listsners = contexts.game.GetGroup(GameMatcher.GameOverListener);
    }

    protected override ICollector<GameEntity> GetTrigger(IContext<GameEntity> context) {
        return context.CreateCollector(GameMatcher.GameOver.AddedOrRemoved());
    }

    protected override bool Filter(GameEntity entity) {
        return true;
    }

    protected override void Execute(List<GameEntity> entities) {
        foreach (var e in entities) {
            foreach (var listener in _listsners) {
                listener.gameOverListener.value.OnGameOver(e.isGameOver);
            }
        }
    }
}
public interface IPositionListener {
    void OnPosition(Vector3 position);
}

public sealed class PositionListenerComponent : IComponent {
    public IPositionListener value;
}

public sealed class PositionEventSystem : ReactiveSystem<GameEntity> {

    public PositionEventSystem(Contexts contexts) : base(contexts.game) {
    }

    protected override ICollector<GameEntity> GetTrigger(IContext<GameEntity> context) {
        return context.CreateCollector(GameMatcher.Position);
    }

    protected override bool Filter(GameEntity entity) {
        return entity.hasPosition && entity.hasPositionListener;
    }

    protected override void Execute(List<GameEntity> entities) {
        foreach (var e in entities) {
            e.positionListener.value.OnPosition(e.position.value);
        }
    }
}

This is the idea, the implementation might be different once I see it in action.

Discussion and feedback is welcome

ghost commented 6 years ago

[Event(false, EventType.AddedOrRemoved)] should generate IPositionListener.OnPosition(GameEntity entity, Vector3 position) and IPositionListener.OnPositionRemoved(GameEntity entity) otherwise you have to check if it's add or remove in the method if you want both. Seems to me ugly if I need to check everythime against isEnabled like this:

IPositionListener.OnPositionAddedOrRemoved(GameEntity entity, Vector3 position) {
    if(entity.isEnabled){
    } else {
    }
}
optimisez commented 6 years ago

Another issue I encounter is let say I have UI A and UI B. When I close UI A and switch to UI B, I will remove all the UI A listeners. Then when I close UI B and open UI A, the UI A does not refresh all the data when I add all the UI A listeners. I think I need something like [Event(false, EventType.AddedOrRemoved, RefreshListener)] to generate one more system to make it call listener one time when adding the ListenerComponent to the entity.

ghost commented 6 years ago

Listeners should be everytime getting called with the current state when they are added. We first thought it would be good to make a reactive system that triggers on the listener component getting added. We later found out it's not a very good idea because they are getting called delayed and not directly (like one tick after, when entitas is getting updated). Therefore we generate a ReactiveSystem for changing data and a InitializeSystem which calls the listener on group added so they are getting called instantly. It's working very nice. Imo that's one of the rare situations where you really want a group added event.

c0ffeeartc commented 6 years ago

Any ETA on updated feature?

sschmid commented 6 years ago

Will pack a new version.

Fyi, things I won't implement:

  1. calling the listener delegate method when adding the listener If needed, call the delegate manually after adding the listener. This is more flexible and work in more scenarios compared to always calling the delegate when added.

  2. Group events All Entitas events are reactive and aggregate changes for now. No group events planned.

sschmid commented 6 years ago

Please feel free to continue the discussion, I will only close because this issue is related to 1.1.0 which is out.

c0ffeeartc commented 6 years ago

Made a repository EventsCE alternative to Entitas Events feature. Current step is replicate Events generation into Generated/EventsCE folder. After step is done other features are planned see Readme

optimisez commented 6 years ago

@sschmid I think just keep the Event(true) and Event(false) attribute naming as I will use extensively and I dun really want to have breaking change anymore.

Another issue I encounter is let say I have UI A and UI B. When I close UI A and switch to UI B, I will remove all the UI A listeners. Then when I close UI B and open UI A, the UI A does not refresh all the data when I add all the UI A listeners. I think I need something like [Event(false, EventType.AddedOrRemoved, RefreshListener)] to generate one more system to make it call listener one time when adding the ListenerComponent to the entity.

For this problem, do you have any good solution for it? I think another way is to just get the component and replace back the component with the same data to make it call listener. Will it better than generate one more system to make it call listener one time when adding the ListenerComponent to the entity?

c0ffeeartc commented 6 years ago

Another issue I encounter is let say I have UI A and UI B. When I close UI A and switch to UI B, I will remove all the UI A listeners. Then when I close UI B and open UI A, the UI A does not refresh all the data when I add all the UI A listeners. I think I need something like [Event(false, EventType.AddedOrRemoved, RefreshListener)] to generate one more system to make it call listener one time when adding the ListenerComponent to the entity.

For this problem, do you have any good solution for it? I think another way is to just get the component and replace back the component with the same data to make it call listener. Will it better than generate one more system to make it call listener one time when adding the ListenerComponent to the entity?

when adding listeners there is access to needed entity, call listener directly after adding listeners.

ghost commented 6 years ago

@optimisez I already explained the solution directly under your posted issue.

optimisez commented 6 years ago

Listeners should be everytime getting called with the current state when they are added. We first thought it would be good to make a reactive system that triggers on the listener component getting added. We later found out it's not a very good idea because they are getting called delayed and not directly (like one tick after, when entitas is getting updated). Therefore we generate a ReactiveSystem for changing data and a InitializeSystem which calls the listener on group added so they are getting called instantly. It's working very nice. Imo that's one of the rare situations where you really want a group added event.

@StormRene If I understand correctly ur solution is using InitializeSystem that only called one time at initialization phase and will not call anymore later. My use case is I will need to keep switching UI back and forth. So, it will keep adding and removing the same listener and each time when I add back the listener, I will need to refresh the listener to make sure the UI always display the latest data.

ghost commented 6 years ago

Nope that's not what I said. From the beginnig of this discussion I'm saying it's important that listener are getting called directly after the registering. All the time. In this text I proposed a problem we found while developing our reactive ui system. Our first solution was write a reactive system with a collector on the specific listener so it can inform the listener. We found out it's not a good solution, because the listener will not be informed instantly but somewhere later (when the next tick is happening in entitas). This leads towards that we wrote all the time in our register listener methods that we also first grab all the data to initialize the object and getting informed with the same data one tick later. (duplicated code) So we corrected the whole thing and made a Initialize System per listener with directly onEntityAdded on the group. Like so:

public void Initialize() {
    this.listenerGroup = uiContext.GetGroup(UIMatcher.HealthStateListener);
    this.listenerGroup.OnEntityAdded += Execute;
}
private void Execute(Entitas.IGroup<UIEntity> group, UIEntity listener, int index, Entitas.IComponent component) {
    var target = coreContext.GetEntityWithId(listener.healthStateListener.targetEntityId);
    if(target) listener.healthStateListener.listener.HealthChanged(target.health.value);
}

So it will be instantly called on register and grab all the data if exists. Otherwise it will maybe getting informed if it will exist in the future by the solution I proposed earlier in this discussion.

Our listeners are getting added with OnEnable and destroyed with OnDisable.

optimisez commented 6 years ago

In this text I proposed a problem we found while developing our reactive ui system. Our first solution was write a reactive system with a collector on the specific listener so it can inform the listener. We found out it's not a good solution, because the listener will not be informed instantly but somewhere later (when the next tick is happening in entitas).

Weird. If I understand correctly, @mzaks says before that reactive system will aggregate and execute everything it needs to execute in one shot. Meaning that it will be done in one frame. Can you share how you do it with reactive system previously?

ghost commented 6 years ago

@optimisez Yes that's true it will execute in one shot. But only when your MonoBehaviour.Update / FixedUpdate where your EventsFeature.Execute is getting called. If you instantiate a bunch of enabled GameObjects and they register itself in the OnEnable on some data they will be not showing the correct data between their particular OnEnable and the next Update call, because the listener is not already called. So with a little bad luck you are showing for a fraction of a second a menu without correct state. We started to write all the time many queries to set the data instantly with the reactive approach only to get the correct data later. It lead us to many redundant code so we said it would be nice if the state is already correctly set when the scope of OnEnable ends. group.onEntityAdded was the solution for that.

FNGgames commented 6 years ago

@StormRene this is why i dont use OnEnable or any other unity message. Instead have an initialisation method as part of an interface on my monobehaviour that gets called by the system instantiating the gameobject. Using the unity messages you're giving up control over execution time.

If you use gameObject.Link() anywhere in your code, then you should be able to call your intialisation methods in the same place.

ghost commented 6 years ago

@FNGgames But OnEnable an OnDisable is very handy. When your designers are working on particular views they can drop your listener scripts on their GameObject and connect the data to show it. Otherweise you always need a coder when a designer just want to add a new healthbar somewhere, because you need to write a system/some code, or am I getting you wrong?

FNGgames commented 6 years ago

I'm just suggesting that the code from OnEnable be moved to another method that's part of an interface e.g. IViewController.InitializeView(); That way, right after you call Instantiate() to create the gameobject, you can do foreach (var view in GetComponents<IViewController>) view.InitializeView();

So your listener scripts just have to implement that interface and you change the name of OnEnable() to InitView() or whatever you want. That way they are always initialized at a controlled, defined time, where you know exactly the state of every relevant object / entity. I also do the same thing with Destroy() / Disable() - there are equivalent methods in my view interface.

ghost commented 6 years ago

And how do you solve the problem when a GameObject is not instantiated by the Controller/a Coder? E.g. a ParticleSystem that needs additional data and the designer just dropped it in the scene.

FNGgames commented 6 years ago

If they're there on scene load perhaps an initial FindObjectsOfType?

ghost commented 6 years ago

No thats not a good idea at all. You want to iterate over all GameObjects and their childs just to call it yourself. That would be insane for a big scene.

FNGgames commented 6 years ago

Ok, how about using onEnable to create sth like RequestInitializationComponent and have a system take care of it?

I'm not as in-tune with the designer / programmer separation since I'm doing both for my team :)

ghost commented 6 years ago

Would it not be the same problem, but in a obfuscated way? You would rather like to see:

void OnEnable() {
   Contexts.SharedInstance.view.CreateEntity().AddRequestInitialization(this);
}
void InitializationAnswer() {
   Contexts.SharedInstance.view.CreateEntity().AddHealthListener(idToListen, this);
}
void IHealthListener.HealthChanged(int newHealth) {
   //stuff
}

than the simple solution:

void OnEnable() {
   Contexts.SharedInstance.view.CreateEntity().AddHealthListener(idToListen, this);
}
void IHealthListener.HealthChanged(int newHealth) {
   //stuff
}

In both ways you need to use a group.onEntityAdded if you want direct response and not waiting for the next Execute call of your event/initrequest features.

FNGgames commented 6 years ago

Forgive me if I'm misunderstanding. I think we have somewhat different architectures that make what I'm suggesting really easy for me, but really awkward for you. I'm allergic to using scenes for level design - i can't deal with the Russian roulette 'who gets initialized first' thing, so i initialize everything with code. I guess ignore me for now, I'm probably not helping! 😄

ghost commented 6 years ago

@FNGgames Haha I know the probem. But if you are working in a team with designers and artists you can't control everything they do - and you shouldn't. They should be creative and life their designing dreams, otherwise you are doing something wrong and don't offer the correct tools. It's also really really annoying if a designer has to come to you every 5 mins, because they want to add something. In the end they are stopping to disturb you and don't make cool stuff. But I agree with you, when you are a team only of coders or techy designer this approach could work although I thinking it's a bit overengineered for just creating view. Mostly their should be no interference if object a is created after object b or the other way around, because the listener are reactive. The relevant code is in entitas.

FNGgames commented 6 years ago

Sure, that's somewhat outside of my experience - my "team" is an artist and me - he doesn't even touch unity! So yeah probably shouldn't follow my advice here, I'll stfu now 😄

DianeOfTheMoon commented 6 years ago

It might be good to change the false/true to something like EventScope.Context/EventScope.Entity to help with readability.