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

Modularize the Color Picker via properties and new picker mode. #67741

Merged
merged 1 commit into from
Nov 4, 2022

Conversation

cridenour
Copy link
Contributor

@cridenour cridenour commented Oct 22, 2022

As noted in #67663, the new color picker removed some flexibility. The new options are great, but limited how we could use the control in game.

Inspired by this, I took a stab at giving full flexibility in the color picker. Properties are exposed to hide all elements outside the picker itself, which can also be disabled with a new Picker Mode.

The existing presets_visible property was no longer respected (launching with it enabled still hid the presets until Swatches was clicked) so I used that to hide the Swatches and Recent Colors buttons.

Similarly, after the new UX presets_enabled worked correctly, but was confusing as internally both recent colors and swatches are considered presets. I changed the name of this property to can_add_swatches.

The picker itself is a fantastic control and the hope is to make it helpful in more scenarios with additional customization options.

Bugsquad edit:

@cridenour cridenour requested review from a team as code owners October 22, 2022 03:29
@cridenour cridenour force-pushed the modular-color-picker branch from 518dc6c to 508ec88 Compare October 22, 2022 03:50
@RedMser
Copy link
Contributor

RedMser commented Oct 22, 2022

Seems like the XML documentation is missing the SHAPE_NONE constant.

@cridenour cridenour force-pushed the modular-color-picker branch from 508ec88 to cefb6c7 Compare October 22, 2022 17:45
@cridenour
Copy link
Contributor Author

Seems like the XML documentation is missing the SHAPE_NONE constant.

Thanks for the catch. Updated.

@cridenour
Copy link
Contributor Author

New push to fix property ordering (actually using doctool this time)

@KoBeWi
Copy link
Member

KoBeWi commented Nov 2, 2022

I tested this and it's overall good. But I have a few notes:

  • all something_visible properties should be inside a group, e.g. "Customization"
  • modes_visible should be color_modes_visible
  • I wonder if can_add_swatches is even necessary. I know it was there, but it just disables a button. Kind of pointless if you can just disable presets altogether. Up to discussion I guess
  • it would be nice if the properties were ordered in the order of elements they represent. Right now it seems random

doc/classes/ColorPicker.xml Outdated Show resolved Hide resolved
doc/classes/ColorPicker.xml Outdated Show resolved Hide resolved
doc/classes/ColorPicker.xml Outdated Show resolved Hide resolved
doc/classes/ColorPicker.xml Outdated Show resolved Hide resolved
doc/classes/ColorPicker.xml Outdated Show resolved Hide resolved
scene/gui/color_picker.cpp Outdated Show resolved Hide resolved
scene/gui/color_picker.h Outdated Show resolved Hide resolved
@cridenour cridenour force-pushed the modular-color-picker branch from 8c8777e to 8d385f0 Compare November 3, 2022 04:22
@cridenour
Copy link
Contributor Author

Updated per @KoBeWi's review:

  • Added a Customization group for the properties.
  • Re-ordered properties to match the visually flow of the control
  • Renamed modes_visible to color_modes_visible
  • Updated documentation wording to suggested

Outstanding:

  • Resolution on open comments
  • Discussion about can_add_swatches and the utility it provides.

scene/gui/color_picker.cpp Outdated Show resolved Hide resolved
doc/classes/ColorPicker.xml 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.

Left some minor comments, but otherwise looks ok.

As for the can_add_swatches property, its removal can still be discussed and done in another PR. But with the changes here, it's possible to make a ColorPicker that contains only swatches, so disabling the button might make sense. But maybe it would be better if the property was hiding it, idk

@cridenour cridenour force-pushed the modular-color-picker branch from 8d385f0 to 286e7da Compare November 4, 2022 04:09
@cridenour
Copy link
Contributor Author

Thanks @KoBeWi! All outstanding comments have been resolved.

@akien-mga akien-mga merged commit d48ba54 into godotengine:master Nov 4, 2022
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

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.

[4.0] New ColorPicker UX lacks customization options
5 participants