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

Reimplement ColorPicker presets #51700

Merged
merged 1 commit into from
Aug 22, 2021
Merged

Conversation

Geometror
Copy link
Member

Reimplements the color preset functionality of the ColorPicker:

grafik

Changes:

  • Reimplement color picker presets with custom Button (ColorPresetButton)
    Presets were disabled due to the way they were implemented as stated by reduz's comment: //presets should be shown using buttons or something else, this method is not a good idea (the former method was using a TextureRect for the whole palette)
  • Add the overbright indicator which is already used in other places of the ColorPicker and an alpha checker board background for transparent colors
  • Some small structural improvements to the code for better readability

@Geometror Geometror requested a review from a team as a code owner August 15, 2021 20:09
@Chaosus Chaosus added this to the 4.0 milestone Aug 15, 2021
@fire
Copy link
Member

fire commented Aug 15, 2021

This design is on the right track. Something came up, maybe @groud can review.

@YuriSizov YuriSizov added the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Aug 15, 2021
scene/gui/color_picker.cpp Outdated Show resolved Hide resolved
scene/gui/color_picker.h Outdated Show resolved Hide resolved
@Geometror Geometror force-pushed the fix-color-picker branch 2 times, most recently from 5e589e3 to 334afe3 Compare August 16, 2021 13:39
@KoBeWi
Copy link
Member

KoBeWi commented Aug 16, 2021

I get this crash when I try to remove preset:

CrashHandlerException: Program crashed
Dumping the backtrace. Please include this when reporting the bug on https://github.com/godotengine/godot/issues
[0] StyleBoxFlat::set_bg_color (C:\godot_source\scene\resources\style_box.cpp:315)
[1] ColorPresetButton::_notification (C:\godot_source\scene\gui\color_picker.cpp:1452)
[2] ColorPresetButton::_notificationv (C:\godot_source\scene\gui\color_picker.h:48)
[3] Object::notification (C:\godot_source\core\object\object.cpp:843)
[4] CanvasItem::_update_callback (C:\godot_source\scene\main\canvas_item.cpp:153)
[5] call_with_variant_args_helper<CanvasItem> (C:\godot_source\core\variant\binder_common.h:227)
[6] call_with_variant_args_dv<CanvasItem> (C:\godot_source\core\variant\binder_common.h:370)
[7] MethodBindT<CanvasItem>::call (C:\godot_source\core\object\method_bind.h:287)
[8] Object::call (C:\godot_source\core\object\object.cpp:832)
[9] Callable::call (C:\godot_source\core\variant\callable.cpp:62)
[10] MessageQueue::_call_function (C:\godot_source\core\object\message_queue.cpp:259)
[11] MessageQueue::flush (C:\godot_source\core\object\message_queue.cpp:306)
[12] Main::iteration (C:\godot_source\main\main.cpp:2521)
[13] OS_Windows::run (C:\godot_source\platform\windows\os_windows.cpp:626)
[14] widechar_main (C:\godot_source\platform\windows\godot_windows.cpp:163)
[15] _main (C:\godot_source\platform\windows\godot_windows.cpp:185)
[16] main (C:\godot_source\platform\windows\godot_windows.cpp:199)
[17] __scrt_common_main_seh (D:\a01\_work\26\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288)
[18] BaseThreadInitThunk
-- END OF BACKTRACE --

@EricEzaM
Copy link
Contributor

Looks like the destroctor runs which calls unref on a static property, so the next time draw is called on any button it crashes.

@KoBeWi
Copy link
Member

KoBeWi commented Aug 16, 2021

Is there even a need for cached stylebox? They aren't very expensive. You could create them on demand to prevent having lots of unused styleboxes in every picker. Or maybe put it in theme and add checks that ensure it's StyleboxFlat (or a StyleboxTexture, which you could colorize; that would be interesting option for customization).

scene/gui/color_picker.h Outdated Show resolved Hide resolved
scene/gui/color_picker.cpp Outdated Show resolved Hide resolved
scene/gui/color_picker.cpp Outdated Show resolved Hide resolved
scene/gui/color_picker.cpp Outdated Show resolved Hide resolved
@KoBeWi
Copy link
Member

KoBeWi commented Aug 16, 2021

You could actually avoid adding a new class if you made a flat button and used draw signal to handle custom drawing inside ColorPicker and modulate instead of pick color.

@Geometror
Copy link
Member Author

Fixed the crash. (the cached stylebox was a bad last-minute decision without testing :/)
I chose to implement the presets with a custom button class because a normal button is more expensive to create (even with no text a shaped_text gets created) and it felt less hacky in general. But if adding a new class is undesired, I will change that.
The idea to make the color preset stylebox part of the theme is pretty good, I will look into that tomorrow :)

@KoBeWi
Copy link
Member

KoBeWi commented Aug 17, 2021

Ah, you are right. Having a custom button is more light-weight in this case, so it's ok.

@Geometror
Copy link
Member Author

  • Added theming support for the ColorPresetButton (StyleBoxFlat and StyleBoxTexture are supported)
    • the foreground can be changed (StyleBoxFlat and StyleBoxTexture are supported)
    • the overbright indicator can be changed (default is white corner triangle)
    • the preset background texture can be changed (default is MiniCheckerboard)
      Now something like this is possible when using a ColorPicker in a project:
      grafik
  • Moved splitted ColorPickerButton theming in default_theme.cpp (there was one large codeblock and one line at another place in the file) Now ColorPicker, ColorPresetButton and ColorPickerButton are all at one place.
  • Renamed preset_bg to sample_bg to be more descriptive (because it was only used for the sample area in the ColorPicker)/ preset_bg is now actually used for the presets
  • Optimization: only update the presets when the their desired size has changed during a theme change (NOTIFICATION_THEME_CHANGED is still sent 4-8 times when selecting another node in the scene tree)

@Geometror Geometror requested a review from a team as a code owner August 17, 2021 20:50
@YuriSizov
Copy link
Contributor

With styling support like this I kind of think it could be made into a separate ColorPalette control, so it could be used independent from the ColorPicker.

@KoBeWi
Copy link
Member

KoBeWi commented Aug 17, 2021

I think that could be done in another PR.

scene/gui/color_picker.cpp Outdated Show resolved Hide resolved
Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

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

Looks ok overall, but change the conditions in NOTIFICATION_DRAW like I commented.

scene/gui/color_picker.cpp Outdated Show resolved Hide resolved
@fire fire merged commit 7cbf5a5 into godotengine:master Aug 22, 2021
@fire
Copy link
Member

fire commented Aug 22, 2021

Thanks.

@akien-mga
Copy link
Member

This would need a dedicated backport PR for 3.x if wanted there. Maybe also without the theme item rename to avoid breaking compat? Provided that it was working in 3.3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherrypick:3.x Considered for cherry-picking into a future 3.x release enhancement topic:gui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants