ColorPickerButton with color_changed signal attached won't show color on button

[Animus-Surge]

Godot version:
3.2.3 stable, also happens in 3.2.2 stable

OS/device including version:
Windows 10 20.04, Intel HD graphics 4600 with GLES 2 and 3 have this problem

Issue description:
When changing the color of a ColorPickerButton with a script that connects to the button's color_changed signal, the button will not show the selected color.

Steps to reproduce:
In the project I provided, simply change the color of the button without the provided script attached.
Then attach the script and do the same thing.

Minimal reproduction project:
testerproject.zip

[dominiks]

You have to call ._color_changed(color) in your color_changed when you override the function to not overwrite the behaviour of the ColorPickerButton. Alternatively you have to reproduce the code yourself:

void ColorPickerButton::_color_changed(const Color &p_color) {
    color = p_color;
    update();
    emit_signal("color_changed", color);
}

[Calinou]

@dominiks Is there a way we could better document this?

[dominiks]

At the moment there is no mention of the _color_changed() function in the documentation and auto complete of ColorPickerButton.

It would be interesting to know how @Animus-Surge came to this point. There is no mention of _color_changed() as a function in the documentation of ColorPickerButton.

a script that connects to the button's color_changed signal,

It seems there as been a misunderstanding at some point because overriding the function is not connecting to and handling a signal. If we can find out where this comes from we could improve the documentation.

[Animus-Surge]

It would be interesting to know how @Animus-Surge came to this point. There is no mention of _color_changed() as a function in the documentation of ColorPickerButton.

I figured that it was already a thing, because, like the Button class, you can just add a method called _pressed() to see if the button's been clicked. Never thought much of it until recently.

You have to call ._color_changed(color) in your color_changed when you override the function to not overwrite the behaviour of the ColorPickerButton ... Overriding the function is not connecting to and handling a signal

That makes perfect sense to me. I guess didn't I remember that connecting the signal is different than attaching a script with the overridden function in it. (I'm doing it so I can just use one function to handle six different ColorPickerButtons.)

I can probably see about adding something to the documentation, at least a placeholder until it can be better documented by someone with far more experience than me in these matters.

[Animus-Surge]

Alright, so I did a little test. I made a script that looks like this

extends ColorPickerButton

func _pressed():
   print("This was pressed!")

func _color_changed(color):
   ._color_changed(color)
   print(color)

and attached it to a ColorPickerButton (edit: forgot this whoops)

The thing that I find interesting is the _pressed function does execute, and also shows the popup without me explicitly calling ._pressed() I'm not too sure, but it might have something to do with the pressed function being virtual? (I have zero experience in C++ so please do correct me if I'm wrong)

(edit 2): Yes I did explicitly call ._color_changed(color), and it did work properly this time.

[dominiks]

Just tested it with current master and this issue does not occur in v4. Creating a func _color_changed(color) in the script attached to the ColorPickerButton does not override anything and the function does not even get called.

Cannot determine what fixed that, though.

_color_changed and _modal_closed are not made available for GDScript in master. This can also be done for 3.2 if we want to prevent issues like this.

[Xrayez]

The thing that I find interesting is the _pressed function does execute, and also shows the popup without me explicitly calling ._pressed() I'm not too sure, but it might have something to do with the pressed function being virtual? (I have zero experience in C++ so please do correct me if I'm wrong)

_color_changed and _modal_closed are not made available for GDScript in master. This can also be done for 3.2 if we want to prevent issues like this.

For reference, even when those methods are no longer available in GDScript, #40670 might also impact this in master.

[Xrayez]

I think this issue is not specific to just ColorPickerButton, but with connecting signals to callbacks which are not exposed to GDScript in general. If you prepend an underscore to a callback in ClassDB::bind_method, then the method will not appear in the documentation/editor, but still remain functional.

Also, it seems like it's not required to ClassDB::bind_method for method callbacks in master, because I think that's handled by Callable now, but still required in 3.2.

That said, relying on "semi-public" API like this should likely be discouraged in the documentation somewhere. The problem is that I don't see a way to figure out whether a callback is already exposed/reserved by C++.

[Xrayez]

To demonstrate, this works (without defining _color_changed via script at all):

extends ColorPickerButton

func _ready():
	assert(has_method("_color_changed"))

As I'm thinking about this, perhaps there might be a way to check whether a script method conflicts with C++ private callback while parsing GDScript, and simply emit a warning if this is the case. This shouldn't affect virtual methods because they are registered via BIND_VMETHOD instead.

[Xrayez]

I've come up with a way to show a warning for cases like in this issue with #42968.

I figured that it was already a thing, because, like the Button class, you can just add a method called _pressed() to see if the button's been clicked. Never thought much of it until recently.

I've just checked, this is because _pressed is registered as virtual method explicitly, unlike _color_changed which is a private C++ callback which happens to be accessible from within GDScript.