Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a flag to make the connection automatically emit the source object. #60143

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Rindbee
Copy link
Contributor

@Rindbee Rindbee commented Apr 11, 2022

Mainly used to improve the connection dialog.

It seems that the previous connection dialog only supports constant bindings. Though duplicating a node in the editor will also duplicate the existing connections, but we still need to adjust some parameters, and it's not very efficient to frequent operations in a pop-up dialog.

For existing connection-related parameters, these can be divided into three categories, listed in the order of use: the parameters of the signal, the binds arguments in the connection, the bound arguments of the callable.

It is probably well known that the binding parameters in the connection dialog are the bound arguments of the callable.

In the current situation, it seems difficult for the receiver to know the information of the signal sender when only use the connection dialog.

In this commit, add a flag bit ConnectFlags.CONNECT_SOURCE_AUTOBIND to make the connection automatically emit the source object.

If this flag bit is enabled, the source object will be emitted at the end of the emitted parameters, right before the bound arguments of the callable. at first, no matter what the other parameters are.

Hopefully this will make the connection dialog a bit more usable.

https://user-images.githubusercontent.com/30386067/162772481-c046b302-178d-41ba-ae8b-145c67ba3a39.mp4

the.source.emitted.first.mp4

@Rindbee Rindbee requested review from a team as code owners April 11, 2022 14:55
@Calinou Calinou added this to the 4.0 milestone Apr 11, 2022
@Rindbee Rindbee force-pushed the better-connection-dialog branch from 23aac23 to 29786fc Compare April 11, 2022 16:11
@Zireael07
Copy link
Contributor

Would work for the use case in godotengine/godot-proposals#3775 albeit in a different way than proposed

@Mickeon
Copy link
Contributor

Mickeon commented Apr 11, 2022

I somewhat like this new flag, but the fact that it requires so much text to be explained properly concerns me and makes me question its simplicity.

"Auto-bind the source object during emission. Mainly used to improve the usability of the connection dialog. Connection-related parameters can be divided into three categories, listed in the order of use: the parameters of the signal, the [code]binds[/code] arguments in the connection, the bound arguments of the callable. If this flag is enabled, the order of the new parameter in the list is right before the bound arguments of the callable (that is, the last parameter to emit. By the way, the binding parameters in the connection dialog are the bound arguments of the callable.)."

@Rindbee
Copy link
Contributor Author

Rindbee commented Apr 11, 2022

I somewhat like this new flag, but the fact that it requires so much text to be explained properly concerns me and makes me question its simplicity.

"Auto-bind the source object during emission. Mainly used to improve the usability of the connection dialog. Connection-related parameters can be divided into three categories, listed in the order of use: the parameters of the signal, the [code]binds[/code] arguments in the connection, the bound arguments of the callable. If this flag is enabled, the order of the new parameter in the list is right before the bound arguments of the callable (that is, the last parameter to emit. By the way, the binding parameters in the connection dialog are the bound arguments of the callable.)."

The picture will make it clearer. These three categories have existed before. Just rarely notice them.

captrue

This will affect the parameter list of the target method.

Emitting the new source object first may avoid some unnecessary trouble. But the changes could be big.

Now, if the flag bit is enable, the source object will be the first argument to emit. It will be easy to write the parameter list of the target function by hand.

Copy link
Member

@reduz reduz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems good, other than minor changes and variable renames. Great job on this feature!

core/object/object.h Outdated Show resolved Hide resolved
core/object/object.cpp Outdated Show resolved Hide resolved
//handle binds
bind_mem.resize(p_argcount + c.binds.size());
bind_mem.resize(source_count + p_argcount + c.binds.size());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think for clarity the appended object should come at the end, not at the begining.

Copy link
Contributor Author

@Rindbee Rindbee Jul 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If so, then use Callable.bind in code to bind some parameters, it won't go at the end, just before those bound parameters.

Of course, for the Connection dialog, Callable.bind will not be involved. It will be at the end.

Edit:
In the Connection dialog, the source object may be hidden by a non-zero unbind and then invalidated. Therefore, only if it is placed at the beginning, it will not be affected by other parameters.

captrue

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yeah, callable binds will come at the end, this makes sense.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be completely honest, I think we should remove connection binds and only use callable binds but this is quite a lot of work.

That said, I think the instance should come after the regular function arguments (even if it comes before the callable bind). This is because we only have the concept of appending arguments to functions. If we had to start discerning between prepending and appending it would be a mess.

core/object/object.cpp Outdated Show resolved Hide resolved
editor/connections_dialog.cpp Outdated Show resolved Hide resolved
editor/connections_dialog.cpp Outdated Show resolved Hide resolved
@Rindbee Rindbee force-pushed the better-connection-dialog branch from d51edda to c859567 Compare July 27, 2022 17:13
@Rindbee Rindbee requested a review from reduz July 27, 2022 17:15
@Rindbee Rindbee force-pushed the better-connection-dialog branch from c859567 to 29cc676 Compare July 27, 2022 17:18
//handle binds
bind_mem.resize(p_argcount + c.binds.size());
bind_mem.resize(source_count + p_argcount + c.binds.size());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be completely honest, I think we should remove connection binds and only use callable binds but this is quite a lot of work.

That said, I think the instance should come after the regular function arguments (even if it comes before the callable bind). This is because we only have the concept of appending arguments to functions. If we had to start discerning between prepending and appending it would be a mess.

@Rindbee
Copy link
Contributor Author

Rindbee commented Jul 28, 2022

The current feeling is that signal and connection binds know the parameters of the original callable, so their parameters are combined to be the parameters of the original callable; while callable.unbind hides the parameters of the original callable that are not needed, and callable.bind will append some. Now it seems to be expected to append a source object after the unbind/bind processing is completed.

The actual callable will usually be the function that receives all these arguments, which means that the function exists on demand, instead of an existing function with a known parameter list (that way, you need to use unbind to discard redundant parameters, which behaves like using an adapter). Well, I'm a little confused about the cause and effect.

IMHO, right now, it actually feels a bit confusing about Callable, even without adding this new parameter:

  1. The result of Callable.bind and Callable.unbind is that we can't find the original Callable, only the Callable after these operations, so it seems that bind/unbind behaves like +=/resize.
  2. bind/unbind behaves more like an insert than an append. Because the calling order and the argument list order of the new callable are reversed. c.bind(A).bind(B) => func c(B, A).

On the other hand, it might make sense if we thought that the callable (the Receiver Method) was trying to receive these parameters. This is how I understand it.:

First, its parameter list size is 0, use bind/unbind to adjust the parameter list, receives (insert) parameters for connection binds, receives (insert) parameters for singal, and now also receives (insert) the source object. It becomes what we see.

Of course, it would be better if the order could be reversed.

@Rindbee
Copy link
Contributor Author

Rindbee commented Jul 29, 2022

I seem to be stuck in a chicken-and-egg dilemma.

Let's go back to the beginning:

  1. This patch is to improve the flexibility of the connection dialog, making it possible to pass the source object.
    If it is through a script, there is no need to introduce it, just use bind().
    Moreover, I personally feel that many users will not change the default connection flags when using the connect() function in most cases, at least I do.
    In complex cases, it is not pleasant to manually write the parameter list of the Receiver Method. I wrote the example in the picture several times, and finally did it without using type hints.
  2. If connection is finished through the connection dialog, I recommend automatically generating the Receiver Method.
    It will solve the problem of parameter arrangement.
    As long as there are no errors, the user doesn't have to care about the intermediate process and why.
    Compared to Callable.bind/Callable.unbind, this flag is simple. I guess users who understand Callable should have no trouble understanding this.
  3. Ease of use is more important.
    Compared to the original, when this flag is enabled, just add the parameter source to the beginning of the parameter list.
    Instead of bothering to check the result of callable bind/unbind and figure out where it is. In that case, it is better to use bind to bind source object.

Honestly, this patch doesn't introduce much complexity.
I just briefly introduced the existing situation, and some people have already complained that it is too complicated. I believe they are the users who trust the connection dialog. In the process of use, they will not come into contact with these complicated things, before and after.

@reduz
Copy link
Member

reduz commented Aug 6, 2022

Heyo, I already removed the connection binds in #63595 so feel free to rebase your PR. I think for merging, as was discussed, the source object should be appended (right after the original signal arguments, before the callable binds) and not prepended. Other than that it looks good!

@Rindbee Rindbee force-pushed the better-connection-dialog branch from 29cc676 to 56ebec0 Compare August 7, 2022 00:34
@Rindbee Rindbee force-pushed the better-connection-dialog branch from 56ebec0 to b467253 Compare August 7, 2022 00:48
@Rindbee
Copy link
Contributor Author

Rindbee commented Aug 7, 2022

Now use a new commit to solve the callable.unbind problem: by changing the order of bind.

The source object will be appended, right before the callable binds (if it exists), after the original signal arguments, after the callable unbind (if it exists). This will ensure that the last unbind still only work on the original signal arguments.

@Rindbee Rindbee requested a review from reduz August 7, 2022 02:22
@Rindbee Rindbee force-pushed the better-connection-dialog branch from 9897987 to 31db271 Compare August 7, 2022 03:10
core/object/object.cpp Outdated Show resolved Hide resolved
@Rindbee Rindbee force-pushed the better-connection-dialog branch 2 times, most recently from a051fdd to 9465dd5 Compare August 22, 2022 00:59
@Rindbee Rindbee requested a review from reduz August 24, 2022 13:45
core/object/object.cpp Outdated Show resolved Hide resolved
@Rindbee Rindbee force-pushed the better-connection-dialog branch from 9465dd5 to f464dcd Compare September 2, 2022 12:39
@Rindbee
Copy link
Contributor Author

Rindbee commented Sep 2, 2022

Okay, don't really agree, but following your advice. Let's wait for the feedback from the users.

@kleonc
Copy link
Member

kleonc commented Sep 11, 2022

2. bind/unbind behaves more like an insert than an append. Because the calling order and the argument list order of the new callable are reversed. c.bind(A).bind(B) => func c(B, A).

For reference: #62891.

@Rindbee Rindbee force-pushed the better-connection-dialog branch from f464dcd to 0b0d580 Compare September 12, 2022 06:07
@YuriSizov YuriSizov modified the milestones: 4.0, 4.x Feb 10, 2023
@Rindbee Rindbee force-pushed the better-connection-dialog branch 2 times, most recently from d7c7ca7 to 440ff85 Compare March 16, 2023 02:53
@Rindbee Rindbee force-pushed the better-connection-dialog branch from 440ff85 to aac14cc Compare March 16, 2023 02:58
@Rindbee
Copy link
Contributor Author

Rindbee commented Mar 16, 2023

I modified the usage condition of this option in connection dialog.

  • Using unbind in the connection dialog will automatically disable this option, in the same way that "Extra Call Argument" is disabled.

It is up to the user when used in scripts. It is more convenient to directly use bind() to bind the source object than to use this enumeration constant in scripts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Objects should send themselves as first argument to any signal
7 participants