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

Color: Expose OKHSL properties #82845

Merged
merged 1 commit into from
Nov 11, 2024
Merged

Conversation

Hysterelius
Copy link
Contributor

@Hysterelius Hysterelius commented Oct 5, 2023

I’m proposing a solution to enhance the Color class by adding an OkHSL lightness attribute. This attribute will facilitate the creation of colour palettes based on predefined colour stops. Currently, this PR exposes a previously private function to public gdscript. I’m unsure how to extend this functionality to C#, and would appreciate any help on this matter.

This PR is an implementation of my feature proposal, just aiming to expose pre-existing infrastructure to the user.

I’ve tested these changes locally and everything appears to be working correctly, but I welcome any feedback or suggestions on my PR’s implementation.

Production edit: Closes godotengine/godot-proposals#7954

@Hysterelius Hysterelius requested review from a team as code owners October 5, 2023 11:12
@AThousandShips
Copy link
Member

AThousandShips commented Oct 5, 2023

You haven't exposed the ok_hsl_h/s values, they are not the same as the h/s values

See also:

@@ -505,6 +505,9 @@
<member name="v" type="float" setter="" getter="" default="0.0">
The HSV value (brightness) of this color, on the range 0 to 1.
</member>
<member name="ok_hsl_l" type="double" setter="" getter="" default="0.0">

This comment was marked as resolved.

@fire
Copy link
Member

fire commented Oct 5, 2023

Ok with the proposal but the cicd has to pass and colours are 32 bit floats.

@Hysterelius

This comment was marked as off-topic.

@AThousandShips
Copy link
Member

What use would it be to only have the l value and not the h or s values?

@Hysterelius
Copy link
Contributor Author

I have now added all the values for ok_hsl (s and h) and rebuilt Godot to fix the docs.

@Hysterelius
Copy link
Contributor Author

Can this sneak into the 4.2 release @lostminds?

@lostminds
Copy link

Can this sneak into the 4.2 release @lostminds?

@Hysterelius I have no say or insight regarding that, I'm just a member of the community. Merging of pull requests and which branch they will go into is decided on by the various Godot core contributor teams. You can see below that this particular one will need review and approval by the core and documentation teams.

@Hysterelius
Copy link
Contributor Author

Sorry, I just assumed you would have to finish your review before the other teams could look at it.

@AThousandShips
Copy link
Member

4.2 is in feature freeze so will likely have to wait for 4.3, this PR was opened just around the time of feature freeze

@nonchip
Copy link

nonchip commented Nov 17, 2023

@AThousandShips is this really a feature though? feels more like a fix for forgetting to expose the already existing feature in a non-writeonly manner...

@AThousandShips
Copy link
Member

That falls under features, it's not a bug but missing functionality, in either case we are close to release so it can soon be looked into once the 4.3 cycle starts

@Kermalis

This comment was marked as off-topic.

@AThousandShips
Copy link
Member

@Kermalis Please don't bump without contributing significant new information. Use the 👍 reaction button on the first post instead.

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.

I needed a way to reduce the lightness of an existing red colour.

The theory is I'd from a red colour I'd get the existing okhsl values from this enhancement, then use to half the lightness value. from_ok_hsl(h: float, s: float, l: float, alpha: float = 1.0)

Sorry for the delay!

@AThousandShips
Copy link
Member

Please squash your commits into one, see the interactive rebase

@akien-mga akien-mga changed the title Add public ok_hsl_l value Color: Expose OKHSL properties Nov 11, 2024
@akien-mga
Copy link
Member

Amended the commit message to be clearer, and tweaked the spelling of OKHSL (it was "OkHSL" in the docs).
Should be good to merge.

@akien-mga akien-mga modified the milestones: 4.x, 4.4 Nov 11, 2024
@Repiteo Repiteo merged commit 30d8722 into godotengine:master Nov 11, 2024
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Nov 11, 2024

Thanks!

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.

Create public to_ok_hsl functions
9 participants