tzachshabtay / MonoAGS

AGS (Adventure Game Studio) reimagined in Mono
https://tzachshabtay.github.io/MonoAGS/
Artistic License 2.0
27 stars 8 forks source link

Support "stop propagation" for events #150

Closed tzachshabtay closed 6 years ago

tzachshabtay commented 7 years ago

For event subscribers to stop the event bubbling to the next subscriber.

ghost commented 6 years ago

I actually need this, but there is another serious issue about events. In AGS you could have a control over order in which events are received (by ordering script modules). MonoAGS does not give such ability atm. This means that even if "stop propagation" would work, that will solve the problem only partially.

Regarding implementation, would that work if we add a flag in the input event args, and check that flag after each handler's call?

tzachshabtay commented 6 years ago

Regarding implementation, would that work if we add a flag in the input event args, and check that flag after each handler's call?

Yes, something like that should work.

But can you explain your scenario? Why do you need this (and the ability to reorder events)? What are you trying to do?

ghost commented 6 years ago

I may have several objects at a time that are subscribed for key events:

The conflict between e.g. car control and menu is solved simply by checking for the current game state (race is paused when the menu is on). On retrospect I could also implement a code that would subscribe and unsubscribe events as the game state changes, but checking for a single flag was easier at the time being (since I was copying code from AGS).

The actual problem I am having right now is this: the menu is called from the room by hitting any key during demo run or Escape during actual game. Menu is closed by Escape too. I've got a bug (ironically, same bug as I had in AGS) - that when I close menu with Escape, room handler gets same key event next and reopens menu immediately.

As for ordering events, it's more about the way of choosing which input handler has a "focus", I think. Maybe it could be solved differently.

ghost commented 6 years ago

Hmm, as I think about this, perhaps all that may be resolved again by introducing the "state controller" objects, or "system" controller, as you wanted to call them, like the ones we discussed in connection to pausing and time control. This will separate all game into parts, where each may be blocked from receiving events. Such approach would make events handling more manageable perhaps.

tzachshabtay commented 6 years ago

Right, so you can solve this in 2 ways even without the StopPropagation feature:

  1. Unsubscribing/resubscribing events like you mentioned.
  2. Creating an input handler class that will be the only subscriber to input and propagate to the current "in focus" component.
ghost commented 6 years ago

I think it may still be required, depending on how subscribing work. What will happen if I subscribe to event while it is being sent to receivers, will I receive it also at that time?

Because what is happening, and still will be happening in my case is this:

tzachshabtay commented 6 years ago

I think it may still be required, depending on how subscribing work. What will happen if I subscribe to event while it is being sent to receivers, will I receive it also at that time?

Hmm, I'm not sure what will happen in this case, but tbh, I wouldn't want to rely on something like this anyway, it's kind of shady.

Anyway, you can still do the second option then (creating an input handler class), which will not have that issue.

And another workaround (for this particular issue): subscribe to the "key up" event for the menu and the "key down" event for the room. I think this should work, as the menu will close on key up, which comes after key down, so the room will not get that event (and you might need to do the reverse for when opening the menu).

tzachshabtay commented 6 years ago

But how that will change anything?

It will change because you will no longer have a list of subscribers, but just one subscriber.


class Menu
{
     public void OnKey(args)
     {
           if (args.Key == Key.Escape) 
                ...
     }
}

class Room
{
    public void OnKey(args)
     {
           if (args.Key == Key.Escape) 
                ...
     }
}

class InputHandler
{
    void Init()
    {
          _input.OnKeyUp.Subscribe(onKeyUp);
    }

    void onKeyUp(args)
    {
         if (_inFocus == _room) _room.OnKey(args);
         else if (_inFocus == _menu) _menu.OnKey(args);
    }
}

So it's basically like the system controller you described, only without putting it in the engine.

Perhaps I do not understand, but what is the purpose of stopping event then if you cannot control the order of events?

The order in which you subscribe to the events dictates their order, so if you can control the order of subscription you can control the order of the events. And I didn't say that controlling the order of the events cannot be useful, as in some cases it might not be possible to change the order of subscription.

UPDATE: actually the order of subscription doesn't dictate the order, added CallbackPriority as part of #246 to address that.

ghost commented 6 years ago

I made this as an experiment, but noticed that you removed "good first issue" tag, so idk if it's a good idea to suggest this as a pull request: https://github.com/ivan-mogilko/MonoAGS/commit/a1adf3a88f7139b95ceea5a93b7d3502eeebd4b8

Per previous discussion, I think focus switching might work in my case, but claiming would still be useful, because I may have two or more active objects that may need to receive key events at the same time. Ofcourse key commands duplication is a rare thing, but better have a secure solution imho. The custom input controller could check for this flag too.

E: hmm no, I keep changing my mind... realized there may be better solution.

tzachshabtay commented 6 years ago

So I removed the "good first issue" tag because I wasn't sure what's the best way to accomplish it anymore, and also considered maybe adding the "rearrange events" as part of it too.

Your commit is pretty much what I had in mind intuitively at first, but this means that we'll want to add that interface to each event we'll want this supported for (and is there any reason why we wouldn't want this functionality in an event?), and also the casting on every invocation might have a minor performance impact.

An alternative solution that occurred to me, would be to allow an additional overload to the Subscribe functions that will receive an additional claim token argument (so you wouldn't have to use it if you don't need it):


IEvent event1;
IEvent<MyArgs> event2;

event1.Subscribe(onEvent1WithNoToken);
event1.Subscribe(onEvent1WithToken);

event2.Subscribe(onEvent2WithNoToken);
event2.Subscribe(onEvent2WithToken);

private void onEvent1WithNoToken() {}

private void onEvent1WithToken(ClaimEventToken token)
{
    token.ClaimEvent();
}

private void onEvent2WithNoToken(MyArgs args) {}

private void onEvent2WithToken(MyArgs args, ClaimEventToken token)
{
    token.ClaimEvent();
}
tzachshabtay commented 6 years ago

If there are no objections, I'll start working on this soon (also want to use this opportunity to do some cleanup to the event classes).

Btw, if you're wondering why some of the comments went missing, this is probably because of this: https://techcrunch.com/2018/03/02/the-worlds-largest-ddos-attack-took-github-offline-for-less-than-tens-minutes/

ghost commented 6 years ago

Btw, if you're wondering why some of the comments went missing, this is probably because of this:

Oh, actually I deleted couple of my comments myself before you replied to them, because I figured some of the answers myself. It's a bad habit I have to continiously edit my posts...

As for the event claim, I decided to try something else for now, switching to different "mindset" from one I had to use when scripting for AGS.