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

ColorPicker: add an intensity slider in raw mode for HDR #99366

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

beicause
Copy link
Contributor

@beicause beicause commented Nov 17, 2024

This PR adopts a relatively simple approach, without storing intensity or modifying the Color class. Intensity is inferred from raw RGB, and will be 0 and not be retained if RGB does not exceed 1.

Add a intensity slider and reduce the max value of RGB to 1 in ColorPicker raw mode.
Keep the hex LineEdit always visiable and switch it to code if RGB exceeds the valid range.

Resolve: godotengine/godot-proposals#1031

@beicause beicause requested a review from a team as a code owner November 17, 2024 18:32
@AThousandShips AThousandShips changed the title ColorPicker: add a intensity slider in raw mode for HDR ColorPicker: add an intensity slider in raw mode for HDR Nov 17, 2024
@AThousandShips AThousandShips added this to the 4.x milestone Nov 17, 2024
Copy link
Member

@fire fire left a comment

Choose a reason for hiding this comment

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

I support colour intensity (hdr), but we want to consult the rendering team and reduz since they vetoed the change during the colour picker rework.

@QbieShay
Copy link
Contributor

What change was veto'd? I see no harm in an intensity slider. I would see harm in adding an intensity field in Color, but that's not what this PR does

@Calinou
Copy link
Member

Calinou commented Nov 19, 2024

The way it's exposed in this PR seems less risky than what godotengine/godot-proposals#1031 originally envisioned, since it's only available in RAW mode and does not impact the RGB/HSV modes in any way.

The downside is that you can't use HSV editing while having an HDR intensity slider on the same page, but you can switch to RAW mode after editing your HSL color and use the intensity slider there.

I also agree with keeping the hexadecimal field visible whenever the color isn't overbright too.

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally, I noticed some issues:

  • Switching to RAW mode increases the dialog's height, which can cause the bottom of the dialog to be unreachable if it spawns at the bottom of a window:
colorpicker_at_bottom.mp4
  • The icon when the color is overbright doesn't display correctly in the running project:

Screenshot_20241119_194447

In contrast, it displays correctly in the editor:

image

  • Switching to the OKHSL Circle picker will result in broken rendering if you were in the RAW mode and intensity is over 1 (this isn't a new issue IIRC, but it's easier to achieve now):

Screenshot_20241119_194624

@beicause
Copy link
Contributor Author

Switching to RAW mode increases the dialog's height, which can cause the bottom of the dialog to be unreachable if it spawns at the bottom of a window:

I think it's acceptable, clicking "Swatches" or "Recent colors" increases the dialog's height as well.

The icon when the color is overbright doesn't display correctly in the running project:

I have fixed it in the running project. However, in editor the 2d screen it still can't display correctly, for which I don't have a solution at the moment.

Switching to the OKHSL Circle picker will result in broken rendering if you were in the RAW mode and intensity is over 1 (this isn't a new issue IIRC, but it's easier to achieve now):

It seems due to the incorrect value of HSV. #99461 should fix it.

@Calinou
Copy link
Member

Calinou commented Nov 20, 2024

I've tested the latest revision of this PR and noticed the color picker can have its hex/code toggle button permanently locked if you raise the intensity above 1 then lower it below 1. Closing and reopening the ColorPicker doesn't fix the issue, you need to reload the saved scene:

image

colorpicker_hex_lockout.mp4

The # symbol is also not clickable anymore in the running project, and no symbol appears when the color is overbright:

image

image

@Calinou
Copy link
Member

Calinou commented Nov 22, 2024

The button to switch between hexadecimal and code works now, but the button becomes invisible once you switch to code (it's still clickable):

image

@beicause
Copy link
Contributor Author

The button to switch between hexadecimal and code works now, but the button becomes invisible once you switch to code (it's still clickable):

It's as expected, because the "Script" icon is only available in the editor. To make it available at runtime, we may add a theme property for it and move the "Script" icon to the default theme. I don't know if it's worth. Originally, this button is only enabled in the editor.

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.

Add an HDR Color toggle and a separated slider for power
5 participants