pmmp / PocketMine-MP

A server software for Minecraft: Bedrock Edition in PHP
https://pmmp.io
GNU Lesser General Public License v3.0
3.28k stars 1.56k forks source link

Scope-specific handler lists #2949

Open SOF3 opened 5 years ago

SOF3 commented 5 years ago

Terminology

Listener: the classical pocketmine\event\Listener interface, or classes/instances of this interface Handler: a function or similar that accepts an event object and is called when (or sometimes when) the event is called Event manager: an object responsible for managing the registration of handlers and dispatching of events

Prerequisites

This issue assumes #2825 and #2826 are implemented such that PocketMine does not need to worry about removing event handlers.

Proposal

Add EventManager:

class EventManager{
    /// similar to the classical PluginManager->registerEvent()
    public function add(string $eventClass, Closure $handler, EventPriority $priority = EventPriority::NORMAL, bool $handleCancelled = false) : void;
    /// similar to the classical PluginManager->registerEvents()
    public function addAll(Listener $listener) : void;

    /// Handles an event and passes them to corresponding handlers
    public function handleEvent(Event $event) : void;
}

Introduce protected function Event::getEventManagers() : Generator, which by default yields only EventManager::getGlobalInstance(). Implementations should yield instances of EventManager.

When Event->call() is called, the event is passed to each yielded event manager.

Different contexts might provide their own event managers. For instance, lifetime-sensitive objects like Entity, Player, etc., may have one EventManager per instance (but the global EventManager is also used). This enables an approach similar to player.on("chat", event => console.log(event.message)) in JavaScript-style events.

While there continues to be a global event manager, it is now possible to perform unit (to be precise, multiple but less units) testing on events through only registering events on a disposable event manager.

dktapps commented 5 years ago

The TL;DR of this: The purpose is to enable scope-isolated event handling. For example, you'll be able to listen to PlayerMoveEvent on a specific player instead of being forced to listen to all the players.

dktapps commented 5 years ago

It's technically possible to do this in a backwards-compatible manner. I don't know if it's worth doing so though.

tarik02 commented 5 years ago

What about an ability to remove event/listener from event manager?

SOF3 commented 5 years ago

Why would you need that? If you want a handler not to outlive a certain object (e.g. a player), you just register on the player event manager. However it makes sense to register a listener for custom scope objects, which is not permitted in this proposal. A way to add custom event managers from a context independent from the event definition module would be great, but I am not sure how that could be achieved. But anyway, the main point of this proposal is that there is no reason to want to remove an event listener. Do you have counter examples?

tarik02 commented 5 years ago

Let's look at example of minigame. There can be a few multiple independent games on single server. So, they can use scope related event listener with Level scope, yes?

But what if we use some pattern like 'Game State'? e.g. there is single game state at single time, the state can change itself to another one at any moment.

It would be very cool to be able to use the state as an event listener with the game's world scope. But, as I said previously, the state can be changed (previous disabled and new one enabled). So, we need to stop old listener to listen to world events and make a new one to do.

SOF3 commented 5 years ago

True, that's why it would be cool to be able to add custom event managers. However, I'm not sure how to implement this without involving a complex system (not another handler for adding handler managers!) and increasing modular coupling.

tarik02 commented 5 years ago

Also, about some optimization. If we register the same listener for the first time, we can grab information about the class once and save it to some array. In this case we will use much lesser reflection when we register the next listener (in this case, only getClosure will be used).

dktapps commented 5 years ago

premature optimization... it's not a hot path ...

tarik02 commented 5 years ago

So, in this case every player/world will its own EventManager? Or how?

dktapps commented 5 years ago

that's what he's been saying the whole time.

SOF3 commented 5 years ago

@Tarik02 if we really find performance a big problem, we might cache the reflection results statically. But that's on-demand optimization that doesn't need to be considered at this stage. At the current stage, let's focus on the API design and what it enables first.

tarik02 commented 5 years ago

@SOF3, okay, I understand.

SOF3 commented 4 years ago

Please do not implement this yet. There remains unresolved problems.

It is a bit confusing for plugins how to use this correctly. For example, a plugin only wants to listen to player killing player events in a specific world. Where should the handler be placed in?

In addition, I think the discussion on "lifetime" above is a bit vague. Can we do extra work to make sure the event manager won't be used beyond the valid lifetime of its scope?