Use subscription based events handling

[bradphelan]

This is a redesign of the event system to approximate a rough observable model.

There is no longer a removeEventListener methods. Subscribing to an event will return a subscription handle which you hold onto as long as you need the event. It's the equivalent of a smart point for events streams.

EventListener is defined as a simple function and a subscription handle as a std::shared_ptr to void. shared_ptr can take a custom deleter and this is leveraged to perform the unsubscribe when all copies of the shared pointer are gone.

    using EventListener = std::function<void(Event&)>;
    using Subscription = std::shared_ptr<void>;

There are various way to subscribe and unsubscribe.

        // subscribe and unsubscribe when the Subscription is disposed.
        [[nodiscard]]
        Subscription subscribe( EventListener listener);  
        // subscribeForEver but you can set the unsubscribe field on the event to true if you want
        void subscribeForEver( EventListener listener);
        // subscribe with listener and unsubscribe after the first event
    void subscribeOnce(EventListener listener);
        // subscribe with listener until s fires an event then unsubscribe
        void subscribeUntil(EventDispatcher &s, EventListener listener); 

The unit test shows how the above works

#include "threepp/core/EventDispatcher.hpp"
#include "threepp/materials/MeshBasicMaterial.hpp"

#include <catch2/catch_test_macros.hpp>

using namespace threepp;

TEST_CASE("Test subscribe") {

    EventDispatcher evt;

    int nCalls = 0;
    {
        auto s0 = evt.subscribe( [&](Event& e) {nCalls++; });

        REQUIRE(nCalls == 0);
        evt.send(Event{});
        REQUIRE(nCalls == 1);
        evt.send(Event{});
        REQUIRE(nCalls == 2);
    }
        evt.send(Event{});
    REQUIRE(nCalls == 2);

}

TEST_CASE("Test addEventListenerOneOwned") {

    EventDispatcher evt;

    int nCalls = 0;
    evt.subscribeForever([&](Event& e) {
        nCalls++;
        if (nCalls == 2) { e.unsubscribe = true; } });

    REQUIRE(nCalls == 0);
    evt.send(Event{});
    REQUIRE(nCalls == 1);
    evt.send(Event{});
    REQUIRE(nCalls == 2);
    evt.send(Event{});
    REQUIRE(nCalls == 2);
}
TEST_CASE("Test subscribeOnce") {

    EventDispatcher evt;

    int nCalls = 0;
    evt.subscribeOnce([&](Event& e) {nCalls++; });
    REQUIRE(nCalls == 0);
    evt.send(Event{});
    REQUIRE(nCalls == 1);
    evt.send(Event{});
    REQUIRE(nCalls == 1);
}

Mouse and Keyboard listeners are now broken into separate objects. The Orbit controller is a good example of how clean it is now.

            auto& mouse = scope.pimpl_->canvas.mouse;
            mouse.OnMouseUp.subscribeOnce([&](MouseButtonEvent& e) {
                onMouseUp(e, scope);
        });
            mouse.OnMouseMove.subscribeUntil(mouse.OnMouseUp, [&](MouseEvent& e) {
                onMouseMove(e, scope);
            });

here after the mouse down we subscribe once to the mouse up and subscribe to the mouse move until mouse up fires.

To see how unsubscribe works looks at OrbitControls.cpp and OrbitControls::Impl

struct OrbitControls::Impl {

    /// Holds the subscriptions and follows rule of zero in that no
    /// destructor is required in the holding class to remove subscriptions.
    /// They are simply unsubscribed when the holding object subs_ is
    /// destroyed which happens when the instance of Impl goes out of scope.
    Subscriptions subs_;

    Impl(OrbitControls& scope, PeripheralsEventSource& canvas, Camera& camera)
        : scope(scope), canvas(canvas), camera(camera)
      {
        subs_ << canvas.keys.Pressed.subscribe([&](auto& e) mutable { onKeyPressed(e, scope); });
        subs_ << canvas.keys.Repeat.subscribe([&](auto& e) mutable { onKeyRepeat(e, scope); });

        subs_ << canvas.mouse.Down.subscribe([&](MouseButtonEvent& e) {
            this->onMouseDown(e, scope);
        });

        subs_ << canvas.mouse.Wheel.subscribe([&](MouseWheelEvent& e) {
            this->onMouseWheel(e, scope);
        });

        update();
    }

}

[markaren]

Thanks for the work. This is quite a change, however, so I need to think about it.

Note that instancing example chrases when InstancedMesh is destroyed. On your first iteration this was fixed by keeping the copy then iterate approach, but this still crashes on the latest version. Have not looked much into it yet.

Edit: The copy on send does indeed work fix the issue, and the underlying issue is that listerers_ was modified during looping.

[bradphelan]

I'm not entirely happy with the pull request design. In principle it works fine. However it is a fairly rough hack on what should be a more rounded concept, one which I didn't have time to fully flesh out. I would refer to

https://victimsnino.github.io/ReactivePlusPlus/v2/docs/html/md_docs_2readme.html

as a better model. For example instead of

object.mouse.OnMouseDown.subscribe([](Event e){ // do something });

It would be better

object.mouse.OnMouseDown 
   | rpp::operator::subscribe([](Event e){ // do something });

That in itself is no improvement but now you can do things like below. In this case the throttle combinator ensures that the mouse events are restricted to occurring with a minimum interval of 300ms. If an upstream event is generated within 300ms of a previous event it is discarded.

object.mouse.OnMouseDown 
    | rpp::operator::throttle(std::chrono::milliseconds{300}) 
    | rpp::operator::subscribe([](Event e){ // do something });

or filtering for a specific key press and then debouncing it.

object.keys.OnKeyPress
   | rpp::operator::filter([](KeyEvent k){return k.key='j';})
   | rpp::operator::debounce(std::chrono::milliseconds(200))
   | rpp::operator::subscribe([](KeyEvent k){// do something;})

There are all sorts of operators available that make processing event streams easier

Unfortunately the lib is c++20 only. I have had great success building UI's in .net with https://www.reactiveui.net/ using the same principles. It may be enough just to ensure the threepp is compatible with such an interface rather than using it internally.

[markaren]

The following code will crash the program as subs go out of scope:

Modified GLRenderer.cpp

        if (programs.empty()) {
            // new material
            subs.emplace_back(material->OnDispose.subscribe( [this](Event& event) {
                this->deallocateMaterial(static_cast<Material*>(event.target));
                event.unsubscribe = true;
            }));
        }

How do we remove the subscription from the scoped list?

The lifetime of events is something that has been an issue from the get go. Initially I used weak_ptrs where the shared_ptrs were provided by the user. However, this was cumbersome.

[bradphelan]

Can you create and push unit test demonstrating the crash?

[markaren]

This will trigger the issue.

    threepp::Subscriptions subs;

    auto geometry = threepp::BoxGeometry::create();
    auto material = threepp::MeshBasicMaterial::create();
    auto mesh = threepp::Mesh::create();

    subs.emplace_back(mesh->OnDispose.subscribe([](auto& evt){}));

[bradphelan]

That is clearly bad code because the subs must live longer than the object they are subscribed to. Objects are disposed in reverse order they are created. So subs are destroyed last. One should just use subscribeForever or subscribeOnce if there is no need to actually unsubscribe before the event generator itself is destroyed. We could ensure that all instances of TEventDispatcher are handled via shared_point and enable_shared_from_this and then

        [[nodiscard]] Subscription subscribe(EventListener listener) {
            size_t current_id = id_;
            id_++;
            listeners_.insert({current_id, listener});
            return utils::at_scope_exit([this, current_id]() { unsubscribe(current_id); });
        }

becomes

        [[nodiscard]] Subscription subscribe(EventListener listener) {
            size_t current_id = id_;
            id_++;
            listeners_.insert({current_id, listener});
            return utils::at_scope_exit([weak_self=shared_from_this(), current_id]() { 
                 if (auto self  = weak_self.lock())
                     self->unsubscribe(current_id); 
            });
        }

[markaren]

I have figured out how to solve the real problem I have had. While destructing GLRenderer et.al, I need to hook into all references to materials and geometries and clear their dispose subscribers. This allows objects to outlive the GLRenderer, which is what I have been struggeling with. ~For this PR it only means that we need a clear() function available.~

[bradphelan]

I think the EventDispatcher should generate subscriptions that hold weak pointer references just in case the user gets the lifetimes wrong. If you know the lifetime of the EventDispatcher is less than the observer then it is better to use SubscribeForever but maybe it's not always obvious and some safety could be built in. I might have a go at that when I get some time.

[bradphelan]

Or maybe subscription objects are held by both the generator and the observer and when the observer goes out of scope it clear all subscription objects so they can't fire again.

[markaren]

At least the internal listeners are now cleared as part of destruction. Any referenced texture, material, geometry and rendertarget are forced to dispose when the GLRenderer destructs. InstancedMesh, however is not tracked internally, so that one is not cleared. Since dispose is used, this PR did not need any change.

[markaren]

I am mostly in favor of merging this PR more or less as it is now. However, I do wonder how generic events ought to be handled? Any insights? On the flip side, one could say that this is something to be solved in "user land".

[bradphelan]

I am mostly in favor of merging this PR more or less as it is now. However, I do wonder how generic events ought to be handled? Any insights? On the flip side, one could say that this is something to be solved in "user land".

I added some comments about this idea above. Pairing with an observable pattern library or at least making sure that it is adaptable would be good.

https://github.com/markaren/threepp/pull/237#issuecomment-1976031304