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.

Sign up for GitHub

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

Jump to bottom

Add trait for clamping colors #12525

Merged
merged 33 commits into from
Mar 17, 2024
Merged

Add trait for clamping colors #12525

merged 33 commits into from
Mar 17, 2024

Conversation

pablo-lua
Copy link
Contributor

@pablo-lua pablo-lua commented Mar 17, 2024 •
edited
Loading

Objective

Solution

  • Added ClampColor
    Due to consistency, is_within_bounds is a method of ClampColor, like is_fully_transparent is a method of Alpha

Changelog

Added

  • ClampColor trait

pablo-lua and others added 18 commits March 5, 2024 14:50
@pablo-lua
Update docs.yml
a50f02a
@pablo-lua
Merge branch 'main' of https://github.com/bevyengine/bevy
785538a
@pablo-lua
Merge branch 'main' of github.com:pablo-lua/bevy
2f22e53
@pablo-lua
Merge branch 'main' of github.com:pablo-lua/bevy
d6a674b
@pablo-lua
Merge branch 'main' of github.com:pablo-lua/bevy
e3e0061
@pablo-lua
Merge branch 'main' of github.com:pablo-lua/bevy
03cfb32
@pablo-lua
[add] Added ClampColor trait
6043dd9
@pablo-lua
[add] Added IsWithinBounds trait
49e29fd
@pablo-lua
[add] Added impl for Hsla color
44bf2b2
@pablo-lua
[add] Added impl for Hsva color
9329b99
@pablo-lua
[add] Added impl for Hwba color
bcbdcd5
@pablo-lua
[add] Added impl for Laba color
dc0d475
@pablo-lua
[add] Added impl for Lcha color
a13c817
@pablo-lua
[add] Added impl for LinearRgba color
6cb513d
@pablo-lua
[add] Added impl for Oklaba color
93bbf18
@pablo-lua
[add] Added impl for Oklcha color
89ff414
@pablo-lua
[add] Added impl for Srgba color
5889b81
@pablo-lua
[add] Added impl for Xyza color
53d5cd5
@pablo-lua pablo-lua added C-Feature A new feature, making something new possible A-Rendering Drawing game state to the screen labels Mar 17, 2024
@pablo-lua @alice-i-cecile
[docs] Grammar correction
70c345e 
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
@pablo-lua pablo-lua added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Mar 17, 2024
SolarLiner
SolarLiner suggested changes Mar 17, 2024
View reviewed changes
Copy link
Contributor

@SolarLiner SolarLiner left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Some changes needed in the implementation for some types, as "out of range" does not have a meaning on some color spaces.

crates/bevy_color/src/hsla.rs Show resolved Hide resolved
crates/bevy_color/src/laba.rs Show resolved Hide resolved
crates/bevy_color/src/oklaba.rs Show resolved Hide resolved
@alice-i-cecile alice-i-cecile removed the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Mar 17, 2024
alice-i-cecile
alice-i-cecile requested changes Mar 17, 2024
View reviewed changes
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Can you swap the language on the docs for the Oklab color spaces in this PR?

I'd really like to avoid accidentally desyncing those.

Probably use wording like "typically in the range of -1.5 to 1.5, although these values will not be clamped."

@pablo-lua
Copy link
Contributor Author

pablo-lua commented Mar 17, 2024 •
edited
Loading

Can you swap the language on the docs for the Oklab color spaces in this PR?

At this point I think its better in a review sense undo 527cbe2 and change the docs in another PR? This way we will not have the docs or code wrong here and can be fixed in a more addressed way in another place.

Unless I misunderstood something here, of course.

@pablo-lua
Copy link
Contributor Author

I'm reverting the change, but the problem pointed by @SolarLiner still exists, so we probably should open an issue or a PR as follow up to address this.

@SolarLiner
Copy link
Contributor

I agree with merging this PR as-is and creating an issue to change the docs and implementation, as @alice-i-cecile described.

@SolarLiner SolarLiner added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Mar 17, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Mar 17, 2024
@alice-i-cecile alice-i-cecile mentioned this pull request Mar 17, 2024
Closed
Merged via the queue into bevyengine:main with commit 509a5a0 Mar 17, 2024
27 checks passed
@pablo-lua pablo-lua deleted the clamp-colors branch March 17, 2024 20:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yyogo yyogo yyogo left review comments

@SolarLiner SolarLiner SolarLiner approved these changes

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

@viridia viridia viridia approved these changes

@bushrat011899 bushrat011899 Awaiting requested review from bushrat011899

Assignees
No one assigned
Labels
A-Rendering Drawing game state to the screen C-Feature A new feature, making something new possible S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Milestone
0.14
Development

Successfully merging this pull request may close these issues.

Clamp and Bounds for colors
5 participants
@pablo-lua @SolarLiner @viridia @yyogo @alice-i-cecile