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 Color::hex fn #362

Merged
merged 5 commits into from
Aug 30, 2020
Merged

Add Color::hex fn #362

merged 5 commits into from
Aug 30, 2020

Conversation

wyhaya
Copy link
Contributor

@wyhaya wyhaya commented Aug 27, 2020

Allows to create color from hex value

// RGB
Color::hex("FFF").unwrap() == Color::rgb(1.0, 1.0, 1.0)
// RGBA
Color::hex("FFFF").unwrap() == Color::rgba(1.0, 1.0, 1.0, 1.0)
// RRGGBB
Color::hex("FFFFFF").unwrap() == Color::rgb(1.0, 1.0, 1.0)
// RRGGBBAA
Color::hex("FFFFFFFF").unwrap() == Color::rgba(1.0, 1.0, 1.0, 1.0)

@karroffel karroffel added C-Code-Quality A section of code that is hard to understand or change C-Feature A new feature, making something new possible A-Rendering Drawing game state to the screen labels Aug 27, 2020
@ncallaway
Copy link
Contributor

Does bevy have a minimum rust version that's targeted, or is latest stable OK? With the new improvements to const fn that came out today (https://blog.rust-lang.org/2020/08/27/Rust-1.46.0.html#const-fn-improvements) I'm pretty sure we could convert this to a const fn, which would be awesome

@cart
Copy link
Member

cart commented Aug 27, 2020

I haven't quite sorted out how I want to handle minimum rust versions. Maybe whatever is currently in stable ubuntu? For now, that feature is fresh enough that I think we should hold off.

@zicklag
Copy link
Member

zicklag commented Aug 27, 2020

What if we had a feature flag for optimizations that are only available on later versions of Rust? It would be disabled by default, but could be enabled to take advantage of later features?

@ncallaway
Copy link
Contributor

I tried playing with it and think even with 1.46 there's not enough str manipulation to easily create a const fn for creating a Color from a hex string.

I think the feature flags for newer rust optimizations is a good idea for optional functionality, but I don't think this will be a place we can add it (sadly)

@cart
Copy link
Member

cart commented Aug 28, 2020

I think in general I would prefer to keep the features as minimal as possible. We can just adopt adopt new features when they hit our MSRV (whatever that ends up being).

Each code path we add introduces maintenance burden and build complexity. I'd rather just support one.

Copy link
Member

@cart cart left a comment

Choose a reason for hiding this comment

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

Awesome work! Just resolve merge conflicts and we're good to go!

(sorry for merging a newer pr first + creating the conflicts. i'll try to give precedence to earlier prs in the future)

@wyhaya wyhaya requested a review from cart August 29, 2020 01:25
Copy link
Member

@CleanCut CleanCut left a comment

Choose a reason for hiding this comment

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

Dependency check:

  • 1 new dependency: hex
    • hex's only runtime dependency is a off-by-default optional serde dependency.

Copy link
Member

@cart cart left a comment

Choose a reason for hiding this comment

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

Great work!

@cart cart merged commit 8106f77 into bevyengine:master Aug 30, 2020
@wyhaya wyhaya deleted the feature/hex_color branch August 31, 2020 02:55
mrk-its pushed a commit to mrk-its/bevy that referenced this pull request Oct 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Code-Quality A section of code that is hard to understand or change C-Feature A new feature, making something new possible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants