Fix legacy color picker crash.

[bruvzg]

• Fixes Crash when user tries to pick a color on Linux #101412 (regression from Fix color picking on Linux/X11. #101266, some changes only relevant for native picker were incorrectly applied to legacy picker as well).
• Fixes [v4.4.dev7] Color picker is unusable and flashes rapidly between black and white #100797.
• Fixes ColorPicker This line is redundant. #101328

[akien-mga]

I confirm this fixes the issue for me with X11/XWayland, multi-window on Fedora 41 KDE Wayland.

There's still room for improvement possibly (weird resizing of the window with black bars on top and bottom, and the preview is either a black or white square with just a tiny preview of the actual color), but these predate #101266. It looks the same in 4.4.dev7.

For comparison, here's how it looks like in 4.3.stable:

[bruvzg]

Reverting the changes I have made to fix another bug is not a good fix.

What bug? Legacy picker is a working by picking pixels from pre-rendered texture, so there's no need to hide zoom preview, it can't obstruct anything. The #101266 only relevant for picking pixels from screen, which is never happening when this branch is used.

[bruvzg]

I see a revert of #101266

It's not reverting it, only a tiny part that was wrong (legacy picker). Fix for real X11 is still there.

[WhalesState]

I see a revert of #101266

It's not reverting it, only a tiny part that was wrong (legacy picker). Fix for real X11 is still there.

Sorry, I was confused, forgot that i have added it in two places :')

[akien-mga]

There seems to be an effect to the code being removed on my config (XWayland multi window, which is likely the most common configuration nowadays as most distros now default to Wayland and we default to X11 multi window).

If I just do what @WhalesState suggests, I get:

Which seems to be marginally better than what I have with this PR.

diff --git a/scene/gui/color_picker.cpp b/scene/gui/color_picker.cpp
index f5be4c59a9..affbd7270a 100644
--- a/scene/gui/color_picker.cpp
+++ b/scene/gui/color_picker.cpp
@@ -1982,9 +1982,11 @@ void ColorPicker::_picker_texture_input(const Ref<InputEvent> &p_event) {
 			picker_preview_style_box_color->set_bg_color(picker_color);
 			picker_preview_style_box->set_bg_color(picker_color.get_luminance() < 0.5 ? Color(1.0f, 1.0f, 1.0f) : Color(0.0f, 0.0f, 0.0f));
 
-			Ref<AtlasTexture> atlas = picker_texture_zoom->get_texture();
-			if (atlas.is_valid()) {
-				atlas->set_region(Rect2i(ofs.x - 8, ofs.y - 8, 17, 17));
+			if (picker_texture_zoom) {
+				Ref<AtlasTexture> atlas = picker_texture_zoom->get_texture();
+				if (atlas.is_valid()) {
+					atlas->set_region(Rect2i(ofs.x - 8, ofs.y - 8, 17, 17));
+				}
 			}
 		}
 	}

It's still imperfect, as the preview square is weirdly offset to the bottom right of the pointer, but it's better.

I think we're starting to get confused by the terminology of legacy, single/multi window, embedded, and FEATURE_SCREEN_EXCLUDE_FROM_CAPTURE. The resulting code is quite a spaghetti mess we'll need to clean up one day...

[bruvzg]

Added missing part for XWayland (different texture setup branch from embedded legacy picker).

[akien-mga]

That works great on XWayland multi-window:

My last nitpick is that there's still an unexpected shrinking of the window with a black bar at the bottom (the window was full screen before). The top black bar is understandable, that's where the window decorations would be, to compare with:

In 4.3 as shown above, I'm not getting this shrinking.

But that's outside the scope of this PR.

I'll test XWayland single-window, Wayland single-window, and real X11 multi/single-window.

[bruvzg]

I think we're starting to get confused by the terminology of legacy, single/multi window, embedded, and FEATURE_SCREEN_EXCLUDE_FROM_CAPTURE. The resulting code is quite a spaghetti mess we'll need to clean up one day...

For reference there are the following cases:

• Embedded windows (on any platform) and Wayland (since it's currently singe window), use legacy picker with zoom preview.
• FEATURE_SCREEN_EXCLUDE_FROM_CAPTURE and FEATURE_SCREEN_CAPTURE available (Windows and macOS) - native window picker with zoom preview.
• Only FEATURE_SCREEN_CAPTURE available (native X11) - native window picker without zoom preview.
• Neither is available (XWayland) - legacy picker with zoom preview (but instead of using window texture, it's using a separate window with texture composed of all app windows, that's what causing resizing of the window with black bars).

[akien-mga]

• XWayland single-window: It seems to work fine for the most part:

There's one minor issue though, when clicking the picker button, a full size copy of the editor window is instantiated as a new window. When moving the cursor, it then disappears and is replaced by the proper preview picker with zoom. Here's an example of what it looks like:

Maybe we should send a small mouse event to update the window contents?

• Wayland single-window: Same behavior as XWayland single-window, including the full window copy.

Switching to X11 to test the other cases.

[WhalesState]

Before doing a cleanup, I have those two PRs that fixes other Picker issues

• ColorPicker remove unused children and fix sample bugs. #101100
• Fix ColorPicker's RAW mode colors and values #88072

[bruvzg]

There's one minor issue though, when clicking the picker button, a full size copy of the editor window is instantiated as a new window. When moving the cursor, it then disappears and is replaced by the proper preview picker with zoom. Here's an example of what it looks like:

I can't reproduce it, but I see what can cause it. Added set_region to force preview window size.

[akien-mga]

Switching to X11 to test the other cases.

Tested the last two commits (886d519 and 34ebefd).

• Native X11 single-window:
  • 886d519: Same behavior as XWayland or Wayland single-window, including window copy.
  • 34ebefd: I confirm this fixes the full sized preview window duplicating the editor window. The only remaining annoyance I see is that the preview window is not instantiated where the picker button was clicked, but at its last known position (top-left corner initially, then wherever it was last clicked/canceled). When the mouse cursor is then moved, it warps to where the cursor is.

• Native X11 multi-window: Same behavior in both commits. This has a preview flickering bug, with different offsets based on the mouse cursor. It's hard to capture in a screenshot, so I made a screen recording:

simplescreenrecorder-2025-01-14_00.15.17.mp4

That last bug also seems to exist in 4.4.dev7, albeit in a worst form where the full size preview with zoom flickers black and white rapidly (epilepsy warning if you test). Edit: That seems to be #100797.

[bruvzg]

X11 flicker bug is what #101485 should fix.

[akien-mga]

X11 flicker bug is what #101485 should fix.

Indeed, removing this line solves #100797 on native X11 multi-window:

I'd suggest adding it in this PR so it can fix #101412, #100797 and #101328 altogether. It's going to conflict with #101485 anyway so we may as well keep that PR only for code style improvements and fix the known bugs here.

diff --git a/scene/gui/color_picker.cpp b/scene/gui/color_picker.cpp
index 6024ea7b96..d1cff07a40 100644
--- a/scene/gui/color_picker.cpp
+++ b/scene/gui/color_picker.cpp
@@ -159,7 +159,6 @@ void ColorPicker::_notification(int p_what) {
                        }
                        DisplayServer *ds = DisplayServer::get_singleton();
                        Vector2 ofs = ds->mouse_get_position();
-                       picker_window->set_position(ofs - Vector2(28, 28));
 
                        Color c = DisplayServer::get_singleton()->screen_get_pixel(ofs);

---

With this change, the only remaining issues I've seen, for future issues/PRs:

• Single-window (native X11): The preview window is not instantiated where the picker button was clicked, but at its last known position (top-left corner initially, then wherever it was last clicked/canceled). When the mouse cursor is then moved, it warps to where the cursor is.

• Multi-window (native X11): Even with the flickering fixed, the way the cursor changes offset based on where it is in the window (seemingly based on the quadrant?) seems quite distracting to me. It would probably be better if it stayed always at the bottom right of the cursor, and only flipped to the left and/or top when nearing one of the 3 window corners where it would make sense.

[bruvzg]

I'd suggest adding it in this PR

Done.

Also added initial position to the picker for single-window and changed native X11 preview to always have same offset from cursor except for the bottom and right border of the screen.

[akien-mga]

Tested successfully on Linux with all combinations (on Fedora 41 KDE):

• Native X11 single-window
• Native X11 multi-window
• XWayland single-window
• XWayland multi-window
• Wayland single-window

[akien-mga]

Thanks!

[bruvzg]

is that there's still an unexpected shrinking of the window with a black bar at the bottom (the window was full screen before). The top black bar is understandable, that's where the window decorations would be, to compare with:

For the reference, native portal picker (#101546) should work much better on XWayland and Wayland (also probably the only way to make screen wide color selection on Wayland).