Entt: Scoped connection for signals

[Lawrencemm]

The sink connect() method should return an object which may be cast to a type scoped_connection. This object's destructor should disconnect the handler passed to connect() from the sink.

See https://github.com/fr00b0/nod which is where I saw this implemented.

In my opinion, even better would be to replace entt's signals with the above library "nod". If you use something like Conan, this can be easily achieved, especially with header-only. But that may be a little idealistic on my part :)

[skypjack]

In my opinion, even better would be to replace entt's signals with the above library "nod". If you use something like Conan, this can be easily achieved, especially with header-only. But that may be a little idealistic on my part :)

You know, this won't happen any time soon. :-)
That library dynamically allocates stuff for the connection object and lacks some features I need and that are already implemented in with the built-in solution, so it wouldn't make much sense. Kind of a regression with no chance for improvements.

That said, yeah, the scoped connection part can be introduced here. :+1:
Not as the default solution btw. Most of the times it's just superfluous.

[Kerndog73]

@skypjack internal::invoker doesn't need to have a virtual destructor because you're using private inheritance. You could declare the destructor as protected (and non-virtual) to ensure that internal::invoker is not used as a polymorphic base or you could just remove it completely.

payload_type doesn't seem to be used anywhere. Not in the sigh.hpp file at least.

A scoped connection object sounds like a good idea to me!

[skypjack]

Good point @Kerndog73 I'm pushing these changes right now. Thanks for the heads up. :+1:

[skypjack]

Because I don't want neither to make it the default nor to make it the sole option available, I think the right solution is to add a scoped_connect function aside the standard connect. Do you have any other suggestion?

[Lawrencemm]

That sounds reasonable.

As an aside, I think that the nod library only allocates if you give it a bigger function object, which afaik is not something your signals actually support anyway, so I don't think you lose anything going with that library.

Of course I understand you're not likely to add that dependency, but in general I'm trying to move away from "roll your own" all the time; I think the future is brighter for everyone if we try to make smaller, more focussed libraries, make them excellent, and make them ubiquitous. Just my 2 cents anyway.

[skypjack]

I agree on this but nod isn't a good solution.
Let me explain what's wrong with it.

First of all, it uses std::function to store your listeners, that is an object that can allocate and on which you have no control in terms of allocations. In fact, nod is a kind of container for multiple std::functions with a bunch of issues all around in the rest of the code.
It doesn't support transparent payload for free functions and you cannot even connect functions with a parameter list that is shorter than that of the signal (a la QObject::connect), that is extremely useful in fact.
The object you use to connect listeners is the same you use to publish, that is a tremendous design flaw, because you give by default the possibility to publish things to classes that don't own the signal and aren't responsible for it.
The disconnector is dynamically allocated (see the std::shared_ptr that owns it and the std::weak_ptrs used all around to manage it).
It uses mutexes everywhere, even within the connect member function, that is just a premature pessimization. Something you don't want at all in a single threaded application and something you don't need most of the times in a multithreaded application. The worst thing is that again you have no control over this policy.
And so on...

These are a few things I can see with a quick glance to the implementation (from mobile while going to work actually, therefore I could be wrong on some points).
My conclusion is that it's all but a well implemented tool to be honest. I wouldn't use it in my applications: design issues, random allocations I cannot control, synchronization tools used in a way that is as odd as wrong and I didn't go in details into the actual implementation yet!

Not sure why this guy decided to roll its own solution, but obviously it's nothing more than a proof of concept with several issues at the moment.
I cannot really think to integrate it in a production-ready library that is widely used in real world applications.
I hope you have a grasp of the reasons now, but feel free to ask if you've any doubt. :+1:

---

Of course I understand you're not likely to add that dependency, but in general I'm trying to move away from "roll your own" all the time; I think the future is brighter for everyone if we try to make smaller, more focussed libraries, make them excellent, and make them ubiquitous. Just my 2 cents anyway.

Probably I should extrapolate the signal part from EnTT and provide users that are only interested in it with a dedicated repository.
This would be by far better than using a buggy solution with no plan for further improvements. :wink:

[Lawrencemm]

It has a cool name
Checkmate

All hilarious jokes aside, have you ever seen a standalone signals library
that looks decent/has promise?

On Fri., 21 Jun. 2019, 4:33 pm Michele Caini, [e-mail]
wrote:

I agree on this but nod isn't a good solution.
Let me explain what's wrong with it.

First of all, it uses std::function to store your listeners, that is an
object that can allocate and on which you have no control in terms of
allocations. In fact, nod is a kind of container for multiple
std::functions with a bunch of issues all around in the rest of the code.
It doesn't support transparent payload for free functions and you cannot
even connect functions with a parameter list that is shorter than that of
the signal (a la QObject::connect), that is extremely useful in fact.
The object you use to connect listeners is the same you use to publish,
that is a tremendous design flaw, because you give by default the
possibility to publish things to classes that don't own the signal and
aren't responsible for it.
The disconnector is dynamically allocated (see the std::shared_ptr that
owns it and the std::weak_ptrs used all around to manage it.
It uses mutexes everywhere, even within the connect member function, that
is just a premature pessimization. Something you don't want at all in a
single threaded application and something you don't need most of the times
in a multithreaded application. The worst thing is that again you have no
control over this policy.
And so on...

These are a few things I can see with a quick glance to the implementation.
My conclusion is that it's all but a well implemented tool to be honest,l.
I wouldn't use it in my applications: design issues, random allocations I
cannot control, synchronization tools used in a way that is as odd as wrong
and I didn't go in details into the actual implementation yet!

Not sure why this guy decided to roll its own solution, but obviously it's
nothing more than a proof of concept with several issues at the moment.
I cannot really think to integrate it in a production-ready library that
is widely used in real world applications.
I hope you have a grasp of the reasons now, but feel free to ask if you've
any doubt. 👍

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/skypjack/entt/issues/267?email_source=notifications&email_token=ABVOXX3DKWOEXAUJXEFJ77TP3RY47A5CNFSM4HZJD5ZKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODYHSIYI#issuecomment-504308833,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABVOXXZ3P7CWQNIUZNR6LP3P3RY47ANCNFSM4HZJD5ZA
.

[skypjack]

To be honest, none of those I looked into totally satisfied me. There were missing features here and there and I decided to roll my own with all the requirements satisfied from day 0.
I've spent some time reading from the web the hints from some people that implemented something similar for private projects (eg the molecularmatters blog) and tried to figure out something that had both a good API and a decent set of features.
So far, the absence of a connection-like class for auto-disconnection is the only complain I've received. Not sure if that's because nobody used the signal part of EnTT before or because most of the users found it good enough for their purposes.

[skypjack]

See branch wip. I've still to write documentation and tests but it would be great if you can give me a feedback and let me know if it matches the expectation. Thanks.

[Lawrencemm]

I've checked out wip and it works for my project.

I'd love to be able to say auto connection = sink.scoped_connect<... though instead of entt::scoped_connection connection = sink.connect<...

Edit: I'm thinking it might also be safer this way and make scoped_connection's constructor private so only the sink can construct it, else you can construct a scoped connection from an already disconnected connection.

[skypjack]

This would require to duplicate the connect, because I don't necessarily want a scoped connection and it would break the link if ignored. This is the reason for which it's designed the other way around. If you're not interested in the connection object, you can just ignore it and anything will break.

[skypjack]

Updated the documentation with the last commit on wip. It would be great if someone could read it and give me a feedback. Thanks. :+1:

[Kerndog73]

entt::scoped_connection is movable and copyable but this will result in the connection being released every time a temporary is destroyed. It should be move-only and have a proper move constructor and assignment operator.

[Lawrencemm]

Why not make a convenience function scoped_connect that just defers to connect and returns a scoped connection constructed from the result? User will be forced to make this function themselves anyway if they don't want their code to look like Java or something.

[Kerndog73]

@Lawrencemm How often are you actually going to write code that looks like this:

entt::scoped_connection conn = sink.connect<&f>();

It seems unlikely to me. You have a connection that exists until the end of the scope which means that you’re gonna have to publish or collect within that scope.

What would probably make more sense is storing the scoped connection in an object. Something like this:

class Foo {
public:
  // or maybe pass the sink to the constructor...
  void connect(entt::sink sink) {
    conn = sink.connect<&Foo::listener>(this);
  }

private:
  entt::scoped_connection conn;

  void listener() {}
};

I don’t think scoped_connection has a default constructor (it probably should).

Could you tell us about the use case for scoped connections? Is it similar to the way you would use Qt signals and slots? In that case you’ll probably be storing the connection in an object so auto can’t be used.

[Kerndog73]

@skypjack This would make sense if the type of calls was a template parameter but in this case, it’s a std::vector so you can just call std::swap directly.

Why does entt::swap even exist if it’s just going to call std::swap?

[skypjack]

@Kerndog73 I strongly suspect this is something remained from older code. Good point.

[skypjack]

entt::scoped_connection is movable and copyable but this will result in the connection being released every time a temporary is destroyed. It should be move-only and have a proper move constructor and assignment operator.

Isn't enough here to add a default constructor and to use the default move constructor/assignment operator? Ofc it also requires to update the release member function so that it checks if the delegate/signal are null objects.

[Kerndog73]

@skypjack You’d still have to define a proper move constructor for delegate.

[skypjack]

Yep. Actually the default one just copies over the internal pointers and this doesn't fit with the purpose of a scoped connection. However it's fine for the delegate class.
I'll push everything soon btw, thanks for the suggestions.

[skypjack]

@Kerndog73 upstream. :+1: Let me know if you find any flaw, otherwise I'll merge it on master tomorrow. Thanks.

[Kerndog73]

@skypjack Looks good! 👍

[skypjack]

It still needs for a couple of tests, then I can merge everything on master. However @Lawrencemm it should work as expected already. :+1:

[Lawrencemm]

Coolios

BTW I was thinking it'd be good if we had a conan package such as entt/x.y.z@skypjack/testing which would just pull the latest wip branch, bit more convenient than doing the mappings myself locally.

[Lawrencemm]

@Kerndog73 In answer to your question

How often are you actually going to write code that looks like this:

entt::scoped_connection conn = sink.connect<&f>();

See here: https://github.com/Lawrencemm/lmeditor/blob/7e28ae5451d59d0bf9199933173b044171fc3443/src/map_editor/save_load_map.cpp

(at the top).

My editor creates a "render cube" component when it loads a "cube" component from the file, so I can see the thing.

[Kerndog73]

@Lawrencemm Seems like a valid use case. 😉 I see why scoped_connect would make sense.

Interesting use of meta BTW. I've never used it and didn't realise it could be used for serialization.