Topic: RFC1 - Signals

As it is, only delegates of the form void(void) can be attached to an sfg::Signal object. In C++03 without std::function or std::bind support we had to roll our own delegate structure to avoid having to depend on boost. To keep it simple and relatively fast, we decided that instead of being a message with internal state, an sfg::Signal should just be an interface that invokes whatever functions were attached to it. The invocation of those functions was the information that a widget would want to convey back to the end user. Conceptually, a widget should not have to pass back newly created information during the invocation of the attached functions because its state should be easy to query and any information that did not originate from the widget itself should not have to be forwarded by the widget since this information should be available for the user to query as well.

Due to the asynchronous nature of event queuing, the event and input state that triggered the sfg::Signal must be bound to the invocation itself since past states cannot be queried easily by the user. This means that a limited form of message passing is required to ease the use of the interface without any non-trivial workarounds.

Suggested Change

Change the signature of sfg::Signal functions to pass the triggering event, if available.


With the outstanding refactor into C++11, we will probably replace Don Clugston's Fast Delegate implementation with std::function, to enable more flexible usage of the interface and allow the user to use std::bind as well.

When considering a typical scenario where std::bind might be used, one has to take into account that the user might be migrating from another UI library and has become accustomed to the way they handle their events. A very common practice is to provide the invoking widget as part of the message passed to the user code in order to enable easy decoupled access to its state when handling the signal. Because std::bind support will be implicitly available when sfg::Signals make use of std::function, I predict users will start connecting sfg::Signals in the following way:

widget->GetSignal( sfg::Widget::OnLeftClick ).Connect( std::bind( &SomeStruct::SomeFunc, &some_struct, std::placeholders::_1, widget ) );

This would make available the widget when the user's function is called, however, because widget is a shared_ptr, storing it inside the std::bind expression, which is indirectly owned by the widget itself, would create an ownership cycle leading to the widget never being destroyed and leaks occurring. To solve this, the user would need to bind a weak_ptr instead of a shared_ptr, however this would mean they would need to acquire ownership of the widget inside their function every time it is called. This is first of all, very annoying for the user and might lead to unnecessary overhead, and assuming many users might not have much experience with shared_ptr ownership concepts, this might become the source of hard to debug leaks.

Suggested Change

Incorporating the previous change, the signature of the functions attached to sfg::Signal would become:

void func( const sfg::Widget::Ptr& widget, const sf::Event& event )

sf::Event might be replaced by an arbitrary event structure in the future.


Please comment on this idea.

Re: RFC1 - Signals

Is this inspired by my last two posts?

3 (edited by Kojay 2013-09-13 20:36:23)

Re: RFC1 - Signals

Passing the widget would be convenient, though not as pressing. If a widget emits a signal, say OnMouseEnter or OnKeyPress, then I already know its state.
Furthermore, if the planned std::bind support is added, it will be possible to connect multiple signals to a single function, bound to different parameter values, thus not requiring to find out which fired it.

Knowing the event on the other hand would be a real boon. In particular, it would be trivial to know which key was pressed or whether the mouse wheel is moved. And in my view it fits with the design, since the event is passed to sfg::Desktop in the first place. I get that it is doing something more abstract than passing it to a widget, but this is what I see it as doing conceptually (as Meyers would put it big_smile) and therefore writing code to find out what it was (and/or which widgets it was passed to) I consider as duplicating SFGUI's efforts.

Therefore I would be happy to see Suggested Change (i):

Change the signature of sfg::Signal functions to pass the triggering event, if available.

(though it's not clear if we still get std::function to play with)

This is my viewpoint as a user; I would not want to see the library become bloated for convenience's sake.

Re: RFC1 - Signals

FRex wrote:

Is this inspired by my last two posts?

No, this was already being considered and was present in ancient versions of SFGUI before being removed along with boost. This is more of a return from a long period of absence due to the fact that C++11 took over from boost in the last years, and as I stated, is also partly to fix non-trivial cases where the user might erroneously write code that has undesirable side-effects.

Kojay wrote:

Passing the widget would be convenient, though not as pressing. If a widget emits a signal, say OnMouseEnter or OnKeyPress, then I already know its state.

The state of a widget is not limited to its widget state i.e. ACTIVE, NORMAL etc. It is the state of the object in a C++ sense, being the entirety of its owned data, thus the state of an sfg::Button would be rather trivial as you said. However the state of any sfg::Container or e.g. sfg::Entry is not as trivial and cannot be deduced from the signal invocation itself. This almost always requires the user to query the widget for whatever information they need, which more often than not adds additional boilerplate code to the start of every signal handler.

The idea is that new users should not have to learn something new or get used to unexpected non-trivial things happening when trying to use SFGUI. If it can be done without too many or any downsides then it should be done. Even with this new system, users who like the old way of things can just use std::bind to discard the arguments SFGUI will pass, and from a performance point of view, since the user signal handlers shouldn't dominate the execution time, there will be virtually no performance loss.

This way we can cater to any development style while staying as performance-concious as we always were. We don't need to teach old dogs new tricks, they can learn them on their own whenever they want.

Re: RFC1 - Signals

No, this was already being considered and was present in ancient versions of SFGUI before being removed along with boost.

Then we independently came to exactly the same conclusions about problems with binding widget pointer and need for event..
That's.. good?? I guess? Tank said the signals will be std::function<void()>.

Re: RFC1 - Signals

Same conclusions? From what I can tell, you advocated for this mainly because other libraries almost all have some sort of system that is similar to the suggested interface and it would be more convenient for people who are coming from those libraries to get used to this and what not. You were right about the part with the circular ownership, but that is why you can bind a weak_ptr and since not many people know how to use them, we should just give them what they want to prevent them from doing stupid things. (This is the one situation where convenience => fault tolerance as stupid as it sounds)

I stated in the original post that the biggest reason I suggested the change was not because the current interface is inconvenient, but because when sfg::Signal is rewritten to use std::function people probably will become tempted to recreate the interface of those libraries with unintended side-effects that I'm not willing to explain 10000 times over and over.

I myself will probably stick to the old interface unless it saves me at least 20 lines of code because I find it cleaner, but if the new interface makes people happy, good for them. Having fun writing code is one thing, writing code that doesn't contain subtle errors is another thing, and if SFGUI can prevent people from making errors that they don't understand without them even noticing, all the better.

And the "being considered" was a reference to a long discussion we had long ago to drop the arguments in favour of void(void). I managed to convince Tank that it was the cleaner solution at that time, but we also didn't support C++11 back then... Tank wasn't yet aware of the problems with std::function<void(void)>, so he might reconsider now.

Re: RFC1 - Signals

Oh look, the void( void ) discussion again. wink

When I was thinking about bigger changes that I'd like to do, one thing was an own event structure within SFGUI. On the one hand for helping decoupling from SFML and being better integrable with other libraries/games, and on the other hand for solving a lot of usability issues on the user's side.

I especially mean the signal handler's signature, which would become something like this: void( sfg::Event ). Of course the event structure would also contain information about the target widget that fired the signal.

Therefore I'm for the "sfg::Widget::Ptr, sf::Event" approach, just without SFML.

8 (edited by Kojay 2013-11-03 03:46:33)

Re: RFC1 - Signals

Tank wrote:

Therefore I'm for the "sfg::Widget::Ptr, sf::Event" approach, just without SFML.

That's good to hear. big_smile

That said, I 've gone ahead and used std::bind on the c++11 branch with no issue. A raw pointer is sufficient and with sfgui now using shared_ptr it is easy to get it, like so:

void listener(const sfg::Widget* widget){
    //do stuff with it
}

myWidget->GetSignal().Connect( std::bind ( &listener, myWidget.get() );

After my prior post here I also found about sfg::Context::GetActiveWidget() which I 've since succesfully used to obtain the widget from within the listener - and even identify it, since widgets can be given ids - leading me to question, why the fuss about that?
That said, today I ran into problems with it, where it was giving me either an incorrect or invalid widget within the signal handler. (Though I 'm afraid I have no minimal for you). Are we not meant to use it?
As stated above though, c++11 easily solves that. Now looking forward to getting the event. (!)

9 (edited by j.booth 2015-11-16 13:50:18)

Re: RFC1 - Signals

Am I right in my assumption that Signals only accept void(void) signature functions, or has the sfg::Widget::Ptr, sf::Event been implemented and I can't see it?

unsigned int Signal::Connect( std::function<void()> delegate ) {

If so, is there any timeline when this will be implemented?  I had code that was passing arguments in VS2013, but no longer works in VS2015.

Another option is using Variadic Templates to take any number of arguments to provide more flexibility to the user (I'm fairly new to C++, so what I may be suggesting could be ludicrous).

e.g.

Signal::Connect ( std::function<void(...)> delegate )

Using something like this concept.

Re: RFC1 - Signals

The "problem" is that SFGUI has to call your function and pass in the parameters you expect to be filled up. Changing the signature to accept functions with a variable number of arguments makes even less sense in that respect.

You might just be misunderstanding what is required when calling Connect(). You have to make your function "look like" a void() to SFGUI. This doesn't mean that your handlers aren't allowed to take any parameters. You will just have to make sure all of them are already bound when passing them to Connect(). This can be done using std::bind().

There was a similar discussion not long ago about std::bind here: http://sfgui.sfml-dev.de/forum/topic495 … ttons.html

If, for example, you wanted to get information on the event and widget which called the handler, you could just bind in those 2 pieces of information when Connect()ing your handler:

void MyClass::MyHandler( const std::weak_ptr<sfg::Button>& triggering_widget, const sf::Event& triggering_event ) {
  auto triggering_widget_shared = triggering_widget.lock();

  ...
}



my_button->GetSignal( sfg::Widget::OnLeftClick ).Connect( std::bind( &MyClass::MyHandler, &my_class_object, std::weak_ptr<sfg::Button>( my_button ), std::cref( current_event ) ) );



while( window.pollEvent( event ) ) {
  current_event = event;
  m_desktop.HandleEvent( event );
}

Important points to remember:

  • We need to bind a weak_ptr instead of the usual shared_ptr because we don't want the widget to store a strong reference to itself thereby creating an ownership cycle. If this happened, it would never get destroyed causing a leak.

  • We need to bind a reference to the currently processed event in order to be able to get the proper event in the handler. If we bound it by copy (the default), the value stored in the event when .Connect() was called would always be used.

I think this is the main reason why we thought changing the signal handler mechanism wasn't very high priority. Users can use what I just showed to build their own highly customised handling by themselves. Sure, it might not look as "clean" as something that is natively supported by the library, but it doesn't prevent users from getting what they want done.

Re: RFC1 - Signals

I understand what you are saying and was using this method to pass additional attributes, however using this method in VS2015 no longer worked. 

I have implemented a work around, so it's not that pressing for me.