-
-
Notifications
You must be signed in to change notification settings - Fork 21.5k
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
Add Various ColorPicker shapes #46340
Conversation
9d9a673
to
43a8805
Compare
43a8805
to
e2b5da3
Compare
Do we really need to implement all the ColorPicker shapes out there? This could end up making the ColorPicker a huge class. To me, it seems the most useful/desired ColorPicker shape is the good old HSV wheel. It solves most issues that the old square picker shape has. |
I'm gonna re-think and edit this draft to dump out some TODO. And you just posted recently ^^ I know this is enough for editor usability, Not like painting software. |
A spacing at the bottom of the circles to separate it from the current color would make it look better. |
b40f2fa
to
b7a978f
Compare
You need to convert to lab colour. If you're around we can chat on discord. Edited: Or rocket chat. Edited: I'll be on in 8 hours. |
@fire I prefer to use Discord. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Globally, the code looks ok to me.
I think the feature is good to have, the ColorPicker node is used outside of the editor itself, so I'd say it make sense to allow for such modification if needed.
b7a978f
to
7924b41
Compare
I think the SHAPE_HSV_WHEEL would be a better fit. It's more common and easier to use. Have you seen with OP first? It's not common and practical to force-push changes to someone else's branch. |
I did a rebase and set the Picker to SHAPE_VHS_CIRCLE, but it seems like there's disagreement. So I'll probably go with the consensus. My personal preference is to also add HSY' because it deals with > 1.0 values better as shown earlier. https://wolthera.info/2014/07/hsi-and-hsy-for-kritas-advanced-colour-selector/ |
@gongpha Feel free to take control again. |
7924b41
to
6294507
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
Thanks! |
Note: this PR introduced a regression in the editor (and likely in user projects to a degree) because it creates shaders in the constructor of the ColorPicker control (so 2 new shaders per each created control). In the editor this causes slowdowns/hitching when updating the Inspector dock, likely the more color pickers are involved, the more it hitches. A fix is in works. |
Introduce more choices of ColorPicker. The settings are available in property and editor settings.
Edit : I think this is enough for usability.