Nabla icon indicating copy to clipboard operation
Nabla copied to clipboard

Example event callbacks need a way to access example members

Open deprilula28 opened this issue 3 years ago • 4 comments

Description

With our current event handling design we need an instance of the event handler in order to get the events, the default one worked well until we needed to handle some events, like we have with the resize event on http://github.com/Devsh-Graphics-Programming/Nabla-Examples-and-Tests/pull/8

Now the problem statement is "How can the event handler access the members of our example class" i.e the relation between the two. This might get used for other event handlers in examples in the future.

Description of the related problem

http://github.com/Devsh-Graphics-Programming/Nabla-Examples-and-Tests/pull/8

Solution proposal

Solution 1 - examples inherit CommonAPIEventHandler

Let the examples that use window inherit from CommonAPIEventHandler and then user passes the event handler itself in InitParams instead of letting CommonAPI construct it for them. this way user can override the callbacks it needs.

I'm not so sure about this one because it has a dependency on logger and input system, which common API provides for us, but we can simply have setter functins for them.

If we make it work, this solution would be convenient and better than the currentnt one and we wouldn't even need to have ~~Resize~~EventHandler

Solution 2 - register std::function

Our examples don't inherit from anything but ~~Resize~~EventHandler contains a bunch of std::functions with correct signatures like std::function<void(uint32_t,uint32_t)>

unless there is some weird reason not to use a std::function this is a pretty good choice.

Solution 3 - the event functions in ApplicationFramework

This is the currently implemented mitigation for resize event callbacks on the afforementioned PR.

This solution has the GraphicalApplication (or maybe IGraphicalApplicationFramework) define virtual functions like onResize, and the CommonAPI::CommonAPIEventCallback stores a IGraphicalApplicationFramework* and has a function like setGraphicalApp and we can so something like:

bool onWindowResized_impl(uint32_t w, uint32_t h) override
{
	m_logger.log("Window resized to { %u, %u }", nbl::system::ILogger::ELL_DEBUG, w, h);
        if (graphicalApp)
           graphicalApp->onResize(w,h);
	return true;
}

The issue with this solution is we're re-defining some function like onResize similar to onWindowResized_impl again as opposed to Solution#1 where we straight up inherit from CommonAPI::CommonAPIEventCallback and the example will be its own event handler, if we had more callbacks we'd need to duplicate them in IGraphicalApplicationFramework as well.

The problem with Solution#1 is having to decide at compile time whether our example will need window and events but It's not a big deal.

We can combine this with Solution#1 and Have GraphicalApplication inherit ICommonAPIEventCallback or even one step further and nbl::ui::IGraphicalApplicationFramework inherit nbl::ui::IWindow::IEventCallback -- this has the same downside of Solution#1 where we have to move the callback creation out of CommonAPI::Init and probably put it in InitParams.

Additional context

deprilula28 avatar Aug 03 '22 12:08 deprilula28

See new comments on your PR, esp: https://github.com/Devsh-Graphics-Programming/Nabla-Examples-and-Tests/pull/8/files#r940222261

Description

With our current event handling design we need an instance of the event handler in order to get the events, the default one worked well until we needed to handle some events, like we have with the resize event on http://github.com/Devsh-Graphics-Programming/Nabla-Examples-and-Tests/pull/8

Now the problem statement is "How can the event handler access the members of our example class" i.e the relation between the two. This might get used for other event handlers in examples in the future.

Description of the related problem

http://github.com/Devsh-Graphics-Programming/Nabla-Examples-and-Tests/pull/8

Solution proposal

Solution 1 - examples inherit CommonAPIEventHandler

Let the examples that use window inherit from CommonAPIEventHandler and then user passes the event handler itself in InitParams instead of letting CommonAPI construct it for them. this way user can override the callbacks it needs.

I'm not so sure about this one because it kinda has dependancy on logger and input system which common apu provides for us, but we can simply have setter functins for them.

IF we make it work This solution would be convenient and better than the currentnt one and we wouldn't even need to have ~Resize~EventHandler

Solution 2 - register std::function

Our examples don't inherit from anything but ~Resize~EventHandler contains a bunch of std::functions with correct signatures like std::function<void(uint32_t,uint32_t)>

unless there is some weird reason not to use a std::function this is a pretty good choice.

Solution 3 - the event functions in ApplicationFramework

This is the currently implemented mitigation for resize event callbacks on the afforementioned PR.

This solution has the GraphicalApplication (or maybe IGraphicalApplicationFramework) define virtual functions like onResize, and the CommonAPI::CommonAPIEventCallback stores a IGraphicalApplicationFramework* and has a function like setGraphicalApp and we can so something like:

bool onWindowResized_impl(uint32_t w, uint32_t h) override
{
	m_logger.log("Window resized to { %u, %u }", nbl::system::ILogger::ELL_DEBUG, w, h);
        if (graphicalApp)
           graphicalApp->onResize(w,h);
	return true;
}

The issue with this solution is we're re-defining some function like onResize similar to onWindowResized_impl again as opposed to Solution#1 where we straight up inherit from CommonAPI::CommonAPIEventCallback and the example will be it's own event handler, if we had more callbacks we'd need to duplicate them in IGraphicalApplicationFramework as well.

The problem with Solution#1 is having to decide at compile time whether our example will need window and events but It's not a big deal.

We can combine this with Solution#1 and Have GraphicalApplication inherit ICommonAPIEventCallback or even one step further and nbl::ui::IGraphicalApplicationFramework inherit nbl::ui::IWindow::IEventCallback -- this has the same downside of Solution#1 where we have to move the callback creation out of CommonAPI::Init and probably put it in InitParams.

Additional context

Solution 1

Is fair idea, if you need special/overriden beehaviour, then inherit and override.

Solution 2

A plain no, function signatures are ugly and provide no encapsulation

Solution 3

Its a pain to read, I don't think this idea was thought out.

Solution 1 is the only one I'll accept.

@deprilula28 Please put this on your TODO as Solution 3 which was the easiest at the moment was implemented

Erfan-Ahmadi avatar Aug 08 '22 13:08 Erfan-Ahmadi

Solution 1 is the only one I'll accept.

I misread the original description, what I meant is..

If you want the handler to do something else define a custom EventHandler which inherits from CommonAPIEventHandler in the application main.cpp then it can be aware of all the extra stuff and the actual type of the application which itself inherits from IGraphicalApplication

UNDER NO CIRCUMSTANCE have the application inherit from event handler