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

Use S, V in hue bar of ColorPicker #60561

Closed
wants to merge 1 commit into from
Closed

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Apr 27, 2022

godot windows tools 64_dgO9R8HTlf

@KoBeWi KoBeWi added this to the 4.0 milestone Apr 27, 2022
@KoBeWi KoBeWi requested a review from a team as a code owner April 27, 2022 13:53
@groud
Copy link
Member

groud commented Apr 27, 2022

Hmm, are other software doing it like this ? I suppose it's a more adequate way to visualize what will happen when you change the slider, but on the other hand it kind of make it impossible to set the hue first when the color defaults to white.

@KoBeWi
Copy link
Member Author

KoBeWi commented Apr 27, 2022

Hmm, are other software doing it like this ?

I've seen this in GIMP

it kind of make it impossible to set the hue first when the color defaults to white.

Yeah, that could be an issue. On the other hand, setting hue first has no effect, so displaying it as white is more accurate.

@groud
Copy link
Member

groud commented Apr 27, 2022

On the other hand, setting hue first has no effect

Yeah, of course, but with the colorful slider, you can still set an approximate hue first, which is very difficult without it.

I think the issue is quite important for the hue slider, as, unlike the other ones, it's a little bit harder a to "map" a given value to what it corresponds to. I mean that it is easy to understand what a value of 200 means for saturation (it's an "high but not to the max" saturation), but I have no clue what a 200 in hue means.

I am not sure how we can solve that though. I think that, ideally, we would have a way to display both at the same time? But I guess the two bars would need to be separated one way or another, as, otherwise, it might get hard to read.

@KoBeWi
Copy link
Member Author

KoBeWi commented Jun 24, 2022

btw OKHSL color mode has similar behavior (and issues...):
godot windows tools 64_TfFh0hijLQ

@fire
Copy link
Member

fire commented Oct 31, 2022

Does #67534 resolve any of these issues?

@KoBeWi
Copy link
Member Author

KoBeWi commented Nov 2, 2022

Not really. Not sure if it's even fixable, because technically the colors are displayed correctly. So it's just sliders are not very useful for OKHSL.

@KoBeWi
Copy link
Member Author

KoBeWi commented Nov 2, 2022

Rebased. I also added the same for OKHSL. This mode is weird, because changing hue seems to affect brightness, but well 🤔

GnsH8MXB8M.mp4

tbh I'm not 100% sure about this change either. It makes the displayed color more accurate at a cost of some usability. I opened the PR because it bothered me that hue bar doesn't change.

@YuriSizov YuriSizov modified the milestones: 4.0, 4.x Feb 6, 2023
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.

On principle, I like the idea of this 🙂

I've tested a version locally rebased against master, it looks like this:

HSV

image

OKHSL

image

The hue slider is rotated. I just accepted the incoming changes as-is when rebasing, which I guess is incorrect due to 84d6549.

@Calinou
Copy link
Member

Calinou commented Jun 9, 2023

For reference, GIMP's color picker behaves this way too – it displays previews on all sliders when in HSV mode:

image

@KoBeWi
Copy link
Member Author

KoBeWi commented Jun 10, 2023

Rebased. I also fixed OKHSL mode (it was using HSV functions).

EDIT:
GitHub did not pickup my push for whatever reason 🤔

@Calinou
Copy link
Member

Calinou commented Jun 10, 2023

GitHub did not pickup my push for whatever reason 🤔

That's strange, I see your latest commit on your fork: https://github.com/KoBeWi/Godot/tree/%F0%9F%8D%9D

That said, static checks are failing.

@KoBeWi
Copy link
Member Author

KoBeWi commented Jun 10, 2023

Static checks are outdated.

@KoBeWi
Copy link
Member Author

KoBeWi commented Jun 10, 2023

I am still unable to revive this. I'll reopen a new PR with a new branch.

EDIT:
Seems like I can't delete branch nor reopen the PR now 🤔

Calinou

This comment was marked as outdated.

@YuriSizov YuriSizov removed this from the 4.x milestone Dec 2, 2023
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.

5 participants