markaren / threepp

C++20 port of three.js (r129)
MIT License
625 stars 59 forks source link

Event listeners should be lambda friendly #236

Open bradphelan opened 8 months ago

bradphelan commented 8 months ago

Currently it's not possible to pass lambdas as event listeners

void EventDispatcher::addEventListener(const std::string& type, EventListener* listener) 

You enforce a virtual base type and also force to pass by pointer. Both of these make it impossible to use lambdas. This makes the code much more noisy forcing users to create classes instead of simple callbacks. Instead of a base class

    struct EventListener {

        virtual void onEvent(Event& event) = 0;

        virtual ~EventListener() = default;
    };

It would be better simply to define

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

and pass and store the callback by value. The problem then becomes how to remove the event. Easy is to use a std::shared_ptr with and return it. So we end up with

    std::shared_ptr<EventListenerHandle> addEventListener(std::function<void(Event&event)>);

The user then hangs onto the EventListenerHandle till they don't need it anymore. As soon as the last copy of this goes out of scope then the listener will be removed. I know you want to make this like threejs but c++ is different and you should take advantage of RAII lifetimes and lambdas.

appModel.listenerSubscription =  mesh.addEventListener(
    [](Event & event){ std::cout << "my mesh was clicked" << std::endl; }
);
markaren commented 8 months ago

Yeah, I totally agree that the EventListener API needs modifications (it was worse before). I'll take your suggestions into consideration.

markaren commented 8 months ago

While I like your idea with a handler, I hope the commits in https://github.com/markaren/threepp/tree/refactor_evt would be a step in the right direction.

markaren commented 8 months ago

I think the main issue with the handle suggestion for this particular API is that internally one listener is re-used N times. I would then have to store the result in a container etc. So internally, the usage becomes messier.

bradphelan commented 8 months ago

This might give you some inspiration though the developer is targetting C++20. I've had a lot of success with the reactive pipeline whilst working in .Net and I use C++ ranges quite often. The observable pattern uses most of the same ideas as range processing except for vales distributed over time instead of space. Events from user interfaces are a perfect match.

On Thu, 29 Feb 2024, 22:01 Lars Ivar Hatledal, @.***> wrote:

I think the main issue with the handle suggestion for this particular API is that internally one listener is re-used N times. I would then have to store the result in a container etc. So internally, the usage becomes messier.

— Reply to this email directly, view it on GitHub https://github.com/markaren/threepp/issues/236#issuecomment-1971955019, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAEJ4XKANDRZEFFH6K7MCDYV6LLXAVCNFSM6AAAAABEACEHC2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNZRHE2TKMBRHE . You are receiving this because you authored the thread.Message ID: @.***>

bradphelan commented 8 months ago

https://github.com/victimsnino/ReactivePlusPlus

Sorry forgot to send the link

On Thu, 29 Feb 2024, 22:14 Brad Phelan, @.***> wrote:

This might give you some inspiration though the developer is targetting C++20. I've had a lot of success with the reactive pipeline whilst working in .Net and I use C++ ranges quite often. The observable pattern uses most of the same ideas as range processing except for vales distributed over time instead of space. Events from user interfaces are a perfect match.

On Thu, 29 Feb 2024, 22:01 Lars Ivar Hatledal, @.***> wrote:

I think the main issue with the handle suggestion for this particular API is that internally one listener is re-used N times. I would then have to store the result in a container etc. So internally, the usage becomes messier.

— Reply to this email directly, view it on GitHub https://github.com/markaren/threepp/issues/236#issuecomment-1971955019, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAEJ4XKANDRZEFFH6K7MCDYV6LLXAVCNFSM6AAAAABEACEHC2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNZRHE2TKMBRHE . You are receiving this because you authored the thread.Message ID: @.***>