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-related convenience methods should be more permissive #13738

Closed
alice-i-cecile opened this issue Jun 7, 2024 · 4 comments · Fixed by #13791
Closed

Color-related convenience methods should be more permissive #13738

alice-i-cecile opened this issue Jun 7, 2024 · 4 comments · Fixed by #13791
Labels
A-Color Color spaces and color math C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Straightforward Simple bug fixes and API improvements, docs, test and examples X-Uncontroversial This work is generally agreed upon
Milestone

Comments

@alice-i-cecile
Copy link
Member

What problem does this solve or what need does it fill?

When constructing a StandardMaterial or MaterialMesh2d, it's frustrating that only Color can be used.

let material = materials.add(StandardMaterial::from(Color::from(PINK)));

Users will rightly expect that any color type can be used.

What solution would you like?

These Color-specific APIs should be generalized, to accept any type that can be converted into a Color.

What alternative(s) have you considered?

We could use anything that can be converted into a LinearRgba instead. While this is perhaps more conceptually correct, it results in implicitly changing the color space of the provided color, which is a large part of why #12212 was rejected.

Additional context

This was brought up by @superdump and spawned a long discussion on related topics, but everyone seems broadly on board with this as a straightforward improvement.

@alice-i-cecile alice-i-cecile added C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled A-Rendering Drawing game state to the screen C-Usability A targeted quality-of-life change that makes Bevy easier to use X-Uncontroversial This work is generally agreed upon D-Straightforward Simple bug fixes and API improvements, docs, test and examples and removed C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Jun 7, 2024
@alice-i-cecile alice-i-cecile added this to the 0.14 milestone Jun 7, 2024
@alice-i-cecile alice-i-cecile added A-Color Color spaces and color math and removed A-Rendering Drawing game state to the screen labels Jun 7, 2024
@NthTensor
Copy link
Contributor

My suggestion was the following. It's kind of gross looking and idk if it will trip up things like rust analyzer.

impl<C: Into<Color>> From<C> for StandardMaterial
    fn from(color: C) -> StandardMaterial {
        StandardMaterial::from(color.into())
    }
}

@cart
Copy link
Member

cart commented Jun 7, 2024

Rust Analyzer would be fine with it. However there is the problem that blanket trait impls block other impls for upstream types:

Screenshot_20240607_121055

@NthTensor
Copy link
Contributor

So either a color space trait or a macro then I guess.

@mintlu8
Copy link
Contributor

mintlu8 commented Jun 7, 2024

I suggest be explicit and provide StandardMaterial::from_color.

github-merge-queue bot pushed a commit that referenced this issue Jun 10, 2024
# Objective

Closes #13738

## Solution

Added `from_color` to materials that would support it. Didn't add
`from_color` to `WireframeMaterial` as it doesn't seem we expect users
to be constructing them themselves.

## Testing

None

---

## Changelog

### Added

- `from_color` to `StandardMaterial` so you can construct this material
from any color type.
- `from_color` to `ColorMaterial` so you can construct this material
from any color type.
mockersf pushed a commit that referenced this issue Jun 11, 2024
# Objective

Closes #13738

## Solution

Added `from_color` to materials that would support it. Didn't add
`from_color` to `WireframeMaterial` as it doesn't seem we expect users
to be constructing them themselves.

## Testing

None

---

## Changelog

### Added

- `from_color` to `StandardMaterial` so you can construct this material
from any color type.
- `from_color` to `ColorMaterial` so you can construct this material
from any color type.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Color Color spaces and color math C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Straightforward Simple bug fixes and API improvements, docs, test and examples X-Uncontroversial This work is generally agreed upon
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants