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

Add Various ColorPicker shapes #46340

Merged
merged 1 commit into from
Apr 10, 2021

Conversation

gongpha
Copy link
Contributor

@gongpha gongpha commented Feb 23, 2021

Introduce more choices of ColorPicker. The settings are available in property and editor settings.

ภาพ

Edit : I think this is enough for usability.

@gongpha
Copy link
Contributor Author

gongpha commented Feb 23, 2021

The thing that makes me surprised is when color is overbright . . .
ภาพ

@Calinou
Copy link
Member

Calinou commented Feb 23, 2021

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.

@gongpha
Copy link
Contributor Author

gongpha commented Feb 23, 2021

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.

@gongpha gongpha marked this pull request as ready for review February 23, 2021 14:23
@gongpha gongpha requested review from a team as code owners February 23, 2021 14:23
@YeldhamDev
Copy link
Member

A spacing at the bottom of the circles to separate it from the current color would make it look better.

@gongpha gongpha force-pushed the various-color-picker branch 2 times, most recently from b40f2fa to b7a978f Compare February 23, 2021 15:35
@fire
Copy link
Member

fire commented Feb 23, 2021

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.

@gongpha
Copy link
Contributor Author

gongpha commented Feb 24, 2021

@fire I prefer to use Discord. gongpha#0238

Copy link
Member

@groud groud left a 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.

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

fire commented Apr 6, 2021

I have updated the PickerShapeType to default to SHAPE_VHS_CIRCLE and rebased on master.

Screen Shot 2021-04-06 at 7 30 19 AM

@groud
Copy link
Member

groud commented Apr 6, 2021

I have updated the PickerShapeType to default to SHAPE_VHS_CIRCLE and rebased on master.

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.

@fire
Copy link
Member

fire commented Apr 6, 2021

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.

image

image

https://wolthera.info/2014/07/hsi-and-hsy-for-kritas-advanced-colour-selector/

@fire
Copy link
Member

fire commented Apr 6, 2021

@gongpha Feel free to take control again.

@gongpha gongpha requested review from groud and removed request for a team April 7, 2021 06:32
Copy link
Member

@groud groud left a comment

Choose a reason for hiding this comment

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

LGTM

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.

Looks good to me.

@akien-mga akien-mga merged commit 8b6e3d6 into godotengine:master Apr 10, 2021
@akien-mga
Copy link
Member

Thanks!

@YuriSizov
Copy link
Contributor

YuriSizov commented May 13, 2021

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.

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.

8 participants