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 gongpha force-pushed the various-color-picker branch from 9d9a673 to 43a8805 Compare February 23, 2021 11:22
@bruvzg bruvzg added this to the 4.0 milestone Feb 23, 2021
@gongpha
Copy link
Contributor Author

gongpha commented Feb 23, 2021

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

@gongpha gongpha force-pushed the various-color-picker branch from 43a8805 to e2b5da3 Compare February 23, 2021 14:04
@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

@fire fire force-pushed the various-color-picker branch from b7a978f to 7924b41 Compare April 6, 2021 14:31
@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 force-pushed the various-color-picker branch from 7924b41 to 6294507 Compare April 6, 2021 15:49
@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!

@gongpha gongpha deleted the various-color-picker branch April 10, 2021 11:32
@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