Color picked from screen doesn't immediately update the button

[KoBeWi]

Godot version

d26442e

System information

Windows 10 x64

Issue description

Probably some signal isn't emitted.

Steps to reproduce

1. Have a ColorPickerButton (can use one from the inspector)
2. Click it and use the pick button to pick color from screen

Minimal reproduction project

No response

[fire]

I can reproduce this and have seen this bug when using the color picker.

[Rindbee]

Because when sampling, the signal is emitted when the LMB is released, but the internal PopupPanel closes when the LMB is pressed. So when sampling, the color_changed signal will not be emitted.

[MinusKube]

I believe the problem appeared with a change to how the popup/viewport works.

Even with the screen Control set as top level, it's still behind the picker's PopupPanel (which was not the case before). So, when you click on the screen, instead of calling the _screen_input method, it's calling the ColorPicker's notification method with a NOTIFICATION_WM_CLOSE_REQUEST, which simply hides screen without updating the color.

This could be worked around by adding a emit_signal(SNAME("color_changed"), color); before the screen->hide(); here:

godot/scene/gui/color_picker.cpp

Lines 103 to 107 in d26442e

	case NOTIFICATION_WM_CLOSE_REQUEST: {
	if (screen != nullptr && screen->is_visible()) {
	screen->hide();
	}
	} break;

But there's probably a better way of doing that?

I tried to fix it more cleanly by creating a new fullscreen popup with a transparent background, but I didn't manage to do this, the background always appeared black.

[KoBeWi]

New popup won't work, because you won't be able to sample what's underneath. Also popups can't be transparent, because they are separate windows (unless embedded).

[Rindbee]

I'm a little curious about how the screenshot tool/screen recording tool works. Get the image behind the window and draw inside the front window to simulate transparent?

[fire]

This describes how the screen tool works.

1. godot/editor/editor_node.cpp

   Lines 2897 to 2898 in d26442e

   	case EDITOR_SCREENSHOT: {
   	screenshot_timer->start();

2. godot/editor/editor_node.cpp

   Lines 2957 to 2968 in d26442e

   	void EditorNode::_request_screenshot() {
   	_screenshot();
   	}
   	void EditorNode::_screenshot(bool p_use_utc) {
   	String name = "editor_screenshot_" + Time::get_singleton()->get_datetime_string_from_system(p_use_utc).replace(":", "") + ".png";
   	NodePath path = String("user://") + name;
   	_save_screenshot(path);
   	if (EditorSettings::get_singleton()->get("interface/editor/automatically_open_screenshots")) {
   	OS::get_singleton()->shell_open(String("file://") + ProjectSettings::get_singleton()->globalize_path(path));
   	}
   	}

3. godot/editor/editor_node.cpp

   Lines 2970 to 2981 in d26442e

   	void EditorNode::_save_screenshot(NodePath p_path) {
   	Control *editor_main_control = EditorInterface::get_singleton()->get_editor_main_control();
   	ERR_FAIL_COND_MSG(!editor_main_control, "Cannot get editor main control.");
   	Viewport *viewport = editor_main_control->get_viewport();
   	ERR_FAIL_COND_MSG(!viewport, "Cannot get editor main control viewport.");
   	Ref<ViewportTexture> texture = viewport->get_texture();
   	ERR_FAIL_COND_MSG(texture.is_null(), "Cannot get editor main control viewport texture.");
   	Ref<Image> img = texture->get_image();
   	ERR_FAIL_COND_MSG(img.is_null(), "Cannot get editor main control viewport texture image.");
   	Error error = img->save_png(p_path);
   	ERR_FAIL_COND_MSG(error != OK, "Cannot save screenshot to file '" + p_path + "'.");
   	}

The screen recording tool uses another path.

[JohanAR]

I'm looking at this right now, I think it happens because _screen_input never gets a valid InputEventMouseButton:

Ref<InputEventMouseButton> bev = p_event;
if (bev.is_valid() && bev->get_button_index() == MouseButton::LEFT && !bev->is_pressed()) {
	emit_signal(SNAME("color_changed"), color);

Which leads to that it never emits the color_changed event.

Couldn't figure out what's causing it, might continue with it tomorrow unless someone else fixes it faster.

edit: Was a bit tired and stressed yesterday, didn't see that other comments were already saying this

[JohanAR]

@MinusKube added your suggested workaround to a PR. I think fixing it for now is preferable to holding off for a proper solution.

[akien-mga]

This appears to be fixed in master according to #63886 (comment).