-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
bevy_color migration plan #12056
Comments
I like phase one a lot. The enhancements and iterative migration seems great. I personally think we should skip the intermediate APIs for bundle types: they're messy and inconsistent and won't make the migration any easier. I would like to ship LegacyColor in one release though, with conversions into the new color types. This will give users time to complain about missing APIs. |
I think it would be beneficial to end-users to have at least 1 Bevy release where the
This will make it clear to users that a new color API is coming, and will be a call to action for feature suggestions/contributions which will refine |
@bushrat011899 The only issue I have with your proposal is that it will be problematic to mark LegacyColor as deprecated while there are APIs still using it. Other than that, it seems fine. |
Also note that LegacyColor will continue to work with the new APIs after migration, but will require an explicit |
I strongly prefer a full hard switch PR where we can tweak / polish / evaluate the new approach in full. "Half ports" risk optimizing for the wrong things because we aren't using the new types in contexts that might inform their design. Likewise, I think a "middle ground / support both" release would just be confusing and introduce more churn than ripping off the bandaid right away. It risks churning our users more because by not fully using a given API internally, we may learn we need to break it again when a new scenario turns up. In the migration scenario, we're renaming I think we should:
|
@cart Renaming I also think that you may be underestimating the degree of user unhappiness generated by introducing a breaking change like this, with no prior warning. I know that there were some angry comments on Reddit about some previous change to Bevy (don't remember the details). Also, I was one of the people involved with the Python 2.x -> 3.0 language evolution, which was far more painful than anyone had predicted - what we thought would take a year ended up taking ten years (it's a long story). However, I don't really want to get into a long debate about this so that's all I have to say for the moment. This is a big migration: there are 852 uses of It would be helpful, I think, to have a survey of all the uses of Color in the current bevy code base, grouped into a dozen or so categories - I've identified a few already: bundle properties and gizmo methods, both of which make for a fairly large chunk. For each of those groups, we can then propose a consistent rule for incorporating color arguments. |
Oops I glossed over that / didn't fully process the implications of the type alias (allowing Color to continue to resolve to the old type).
This may be true, but note that I am expecting users to be unhappy with this breakage. From my perspective deprecation only serves to confuse and prolong the situation more:
That being said, I'm less opposed to the "deprecation" part of the proposal than I am to the "partial port across releases" part of the proposal. I'd still be reasonably happy with the approach: "full port, with a soft landing by keeping the old Color type around for a release". I just expect the "support both / soft landing" approach to introduce additional weirdness and comparable levels of discomfort (and likely greater amounts of discomfort, when the porting process is considered as a whole).
Oooh interesting. I'm semi-familiar with that topic. I didn't know you were involved. Very cool!
Agreed!
Seems reasonable to me! If we can identify every case ahead of time (and develop a "port strategy" that we agree is a net-win ahead of time), I'm more willing to break this up across PRs (especially if we commit to doing the full port before the next release). The concerns I would like mitigated:
|
@cart One thing to bear in mind is that there are a number of people who are gung-ho to start migrating Bevy right now. You can expect PRs to start showing up within the next couple days. This is a good thing, but we may want to exercise some restraint - once those PRs get merged we are pretty much committed unless we plan to revert them later. For example, one strategy might be to release bevy_colors in 0.13.1, with no changes to Bevy APIs - a "preview" of what's coming, which will then give users several months to get used to the idea that the Bevy APIs will change in 0.14. BTW, I want to push back on the idea of renaming |
Good points. If we go that route, it would likely need to be Srgba->Color and LinearSrgba->LinearColor to create the appropriate parity (especially if we're using LinearSrgba/LinearColor for StandardMaterial). Terminology-wise I personally don't see a fundamental problem with designating Srgba "Color", given that the rest of the industry does that, and people will gravitate toward that name for obvious reasons. But I'm largely undecided atm. I currently feel more strongly about the reverse: that |
Fully agreed. This is one of the reasons I'm advocating for a consolidated port. If we decide to break it up, it should be organized and extremely intentional about the patterns / apis we are adopting + encouraging. |
I'm a bit dubious about the effectiveness of this. I strongly suspect that the majority of people will not see or care about these changes until they are surfaced at the API level. The people that are interested in "opting in" to being early adopters can already use the |
My bikeshed opinions:
|
After doing #12069, I have a pretty good sense of our current uses of color. They are:
That's pretty much it. 1 and 2 are the most controversial: we should pick a single standard and use it everywhere. As I see it, our options:
3 should clearly just be linear RGB. 4 is the most lines of code, but should be trivial to migrate regardless of the choice we take. If we pick a polymorphic color type it's trivial, otherwise just call .into() or add |
@alice-i-cecile Do we want to do the migration work in a branch? |
No: long-lived branches are the work of the devil :p We tried that with the stageless rewrite and it was much more painful (merge conflicts, stalled work) than just incrementally porting things over and dealing with main living in a weird hybrid state for a while. |
For things like UI I think this risks being very confusing for people working with the r/g/b/a fields directly, given that Srgba is what a high percentage of people will be expecting. I expect this to be a common source of bugs: // ui_node.color is LinearSrgb
ui_node.color = Srgba::rgb(0.5, 0.0, 0.0).into();
// some time later
ui_node.color.r = 0.5; // this is not the same value Can we expect every user that wants to read the "red" value of their UI node color (in srgba, which will be the common case) to remember to do something like: println!("{}", ui_node.color.to_srgba().r); Or to remember when setting that value that they must do: ui_node.color.r = Srgba::r(0.5).to_linear(); Not being able to rely on reading / writing individual color components directly seems like a pretty big loss, both from an ergonomics perspective and (more importantly) a "protecting people from easy mistakes" perspective. Using linear color on components for things like StandardMaterial makes sense to me, given that "srgb color exactness" is very rarely a thing in 3D (and goes out the window the second we introduce tonemapping, shading, exposure, etc). I think people can / generally should just use linear srgb directly (without the need for into() conversions). This has the benefit of being able to directly read/ write the individual color components ( For sprites srgb color exactness is likely to be more relevant, as people will be composing their assets in 2D software that generally uses srgb (and exported to formats that also use srgb). Using totally made up numbers, I'm guessing something like 80-90% of 2D games will expect srgb color space in public apis. If we use linear color on those components, we are making the common case harder and more error prone. However some games (ex: 2D games that use lighting or HDR) will benefit from the linear representation. In the interest of nicely supporting 2D games with more complicated lighting, I can see the argument to use linear, even if it means making the common case harder. Especially if we plan on blurring the lines between 2D and 3D. But I think we would be sacrificing the common case and thats worth discussing. UI is the same story as 2D sprites (functionally), but I suspect the "color expectation" split to be more like 99% for srgba and 1% for "linear HDR / dynamically lit" UI. I expect a very high percentage of confused people if we used linear color for the components there. A consistent "everything in Bevy is linear" story is compelling because it is easy to document and to wrap your head around. But I think in practice it would cause a lot of people on the 2D / UI side serious headaches. |
Yep, I think those are very real concerns. I think that means that we should store in polymorphic format, look into caching with benchmarks, and just eat the conversion costs for now. SRGB is not meaningfully better than the polymorphic design IMO (it doesn't stop conversion costs), and it is trickier to work with for inspectors and scenes. Another point against using SRGB: I think that HSL or OKLAB are a better choice for most "manually specifying colors" work: the better interpolation properties and easier to unify parameterization is really useful when designing palettes. A fourth option actually: let's call it "tagged linear rgb": pub struct Color {
/// The cached ground truth color, used for rendering
pub rgba_representation: LinearRgba,
/// HSL, OKLAB, sRGB, linear RGB etc
pub original_color_model: ColorModel,
} Pros vs the polymorphic model:
Cons vs the polymorphic type:
Overall I like the polymorphic and the tagged linear rgb options best. |
# Objective The migration process for `bevy_color` (#12013) will be fairly involved: there will be hundreds of affected files, and a large number of APIs. ## Solution To allow us to proceed granularly, we're going to keep both `bevy_color::Color` (new) and `bevy_render::Color` (old) around until the migration is complete. However, simply doing this directly is confusing! They're both called `Color`, making it very hard to tell when a portion of the code has been ported. As discussed in #12056, by renaming the old `Color` type, we can make it easier to gradually migrate over, one API at a time. ## Migration Guide THIS MIGRATION GUIDE INTENTIONALLY LEFT BLANK. This change should not be shipped to end users: delete this section in the final migration guide! --------- Co-authored-by: Alice Cecile <[email protected]>
I want to add one more phase to our migration plan, which is a post-mortem: write up a summary of what was done, and an assessment of how well the new types worked. This will be important for users who will be wanting to understand both the justification and impact of the new changes. The 0.14 release notes will have a description of the new crate (which I can provide), and a description of how the rest of Bevy has changed. |
# Objective The migration process for `bevy_color` (bevyengine#12013) will be fairly involved: there will be hundreds of affected files, and a large number of APIs. ## Solution To allow us to proceed granularly, we're going to keep both `bevy_color::Color` (new) and `bevy_render::Color` (old) around until the migration is complete. However, simply doing this directly is confusing! They're both called `Color`, making it very hard to tell when a portion of the code has been ported. As discussed in bevyengine#12056, by renaming the old `Color` type, we can make it easier to gradually migrate over, one API at a time. ## Migration Guide THIS MIGRATION GUIDE INTENTIONALLY LEFT BLANK. This change should not be shipped to end users: delete this section in the final migration guide! --------- Co-authored-by: Alice Cecile <[email protected]>
# Objective The migration process for `bevy_color` (bevyengine#12013) will be fairly involved: there will be hundreds of affected files, and a large number of APIs. ## Solution To allow us to proceed granularly, we're going to keep both `bevy_color::Color` (new) and `bevy_render::Color` (old) around until the migration is complete. However, simply doing this directly is confusing! They're both called `Color`, making it very hard to tell when a portion of the code has been ported. As discussed in bevyengine#12056, by renaming the old `Color` type, we can make it easier to gradually migrate over, one API at a time. ## Migration Guide THIS MIGRATION GUIDE INTENTIONALLY LEFT BLANK. This change should not be shipped to end users: delete this section in the final migration guide! --------- Co-authored-by: Alice Cecile <[email protected]>
# Objective This PR arose as part of the migration process for `bevy_color`: see #12056. While examining how `bevy_gizmos` stores color types internally, I found that rather than storing a `Color` internally, it actually stores a `[f32;4]` for a linear RGB, type aliased to a `ColorItem`. While we don't *have* to clean this up to complete the migration, now that we have explicit strong typing for linear color types we should use them rather than replicating this idea throughout the codebase. ## Solution - Added `LinearRgba::NAN`, for when you want to do cursed rendering things. - Replaced the internal color representation in `bevy_gizmo`: this was `ColorItem`, but is now `LinearRgba`. - `LinearRgba` is now `Pod`, enabling us to use the same fast `bytemuck` bit twiddling tricks that we were using before. This requires: 1. Forcing `LinearRgba` to be `repr(C)` 2. Implementing `Zeroable`, and unsafe trait which defines what the struct looks like when all values are zero. 3. Implementing `Pod`, a marker trait with stringent safety requirements that is required for "plain old data". --------- Co-authored-by: Alice Cecile <[email protected]>
# Objective As we start to migrate to `bevy_color` in earnest (#12056), we should make it visible to Bevy users, and usable in examples. ## Solution 1. Add a prelude to `bevy_color`: I've only excluded the rarely used `ColorRange` type and the testing-focused color distance module. I definitely think that some color spaces are less useful than others to end users, but at the same time the types used there are very unlikely to conflict with user-facing types. 2. Add `bevy_color` to `bevy_internal` as an optional crate. 3. Re-export `bevy_color`'s prelude as part of `bevy::prelude`. --------- Co-authored-by: Alice Cecile <[email protected]>
@cart @alice-i-cecile Here's a rough draft of some release notes: In Bevy 0.14, we've revamped the way that Bevy defines and represents colors. The old
To address these issues, there is now a new
In addition, these color types support a selection of useful methods for manipulating colors:
The new types also support idiomatic Rust conventions for converting between different color spaces. For example, converting from Of course, there will still be cases where you need to represent a color which might be in one of several different color models, such as when parsing an asset or stylesheet that allows color to be specified in various ways. To support this, The "standard" color constants have also been revamped. The old color constants were a random subset of the standard X11 / CSS3 color names. The new constants live in their own module,
We realize that this change will have a major impact on user code, since color shows up in a lot of places. Fortunately, the actual migration should be fairly easy. A lot of Bevy APIs which accept a color argument now take an (After this, add a section summarizing the changes to Bevy APIs). |
# Objective - As part of the migration process we need to a) see the end effect of the migration on user ergonomics b) check for serious perf regressions c) actually migrate the code - To accomplish this, I'm going to attempt to migrate all of the remaining user-facing usages of `LegacyColor` in one PR, being careful to keep a clean commit history. - Fixes #12056. ## Solution I've chosen to use the polymorphic `Color` type as our standard user-facing API. - [x] Migrate `bevy_gizmos`. - [x] Take `impl Into<Color>` in all `bevy_gizmos` APIs - [x] Migrate sprites - [x] Migrate UI - [x] Migrate `ColorMaterial` - [x] Migrate `MaterialMesh2D` - [x] Migrate fog - [x] Migrate lights - [x] Migrate StandardMaterial - [x] Migrate wireframes - [x] Migrate clear color - [x] Migrate text - [x] Migrate gltf loader - [x] Register color types for reflection - [x] Remove `LegacyColor` - [x] Make sure CI passes Incidental improvements to ease migration: - added `Color::srgba_u8`, `Color::srgba_from_array` and friends - added `set_alpha`, `is_fully_transparent` and `is_fully_opaque` to the `Alpha` trait - add and immediately deprecate (lol) `Color::rgb` and friends in favor of more explicit and consistent `Color::srgb` - standardized on white and black for most example text colors - added vector field traits to `LinearRgba`: ~~`Add`, `Sub`, `AddAssign`, `SubAssign`,~~ `Mul<f32>` and `Div<f32>`. Multiplications and divisions do not scale alpha. `Add` and `Sub` have been cut from this PR. - added `LinearRgba` and `Srgba` `RED/GREEN/BLUE` - added `LinearRgba_to_f32_array` and `LinearRgba::to_u32` ## Migration Guide Bevy's color types have changed! Wherever you used a `bevy::render::Color`, a `bevy::color::Color` is used instead. These are quite similar! Both are enums storing a color in a specific color space (or to be more precise, using a specific color model). However, each of the different color models now has its own type. TODO... - `Color::rgba`, `Color::rgb`, `Color::rbga_u8`, `Color::rgb_u8`, `Color::rgb_from_array` are now `Color::srgba`, `Color::srgb`, `Color::srgba_u8`, `Color::srgb_u8` and `Color::srgb_from_array`. - `Color::set_a` and `Color::a` is now `Color::set_alpha` and `Color::alpha`. These are part of the `Alpha` trait in `bevy_color`. - `Color::is_fully_transparent` is now part of the `Alpha` trait in `bevy_color` - `Color::r`, `Color::set_r`, `Color::with_r` and the equivalents for `g`, `b` `h`, `s` and `l` have been removed due to causing silent relatively expensive conversions. Convert your `Color` into the desired color space, perform your operations there, and then convert it back into a polymorphic `Color` enum. - `Color::hex` is now `Srgba::hex`. Call `.into` or construct a `Color::Srgba` variant manually to convert it. - `WireframeMaterial`, `ExtractedUiNode`, `ExtractedDirectionalLight`, `ExtractedPointLight`, `ExtractedSpotLight` and `ExtractedSprite` now store a `LinearRgba`, rather than a polymorphic `Color` - `Color::rgb_linear` and `Color::rgba_linear` are now `Color::linear_rgb` and `Color::linear_rgba` - The various CSS color constants are no longer stored directly on `Color`. Instead, they're defined in the `Srgba` color space, and accessed via `bevy::color::palettes::css`. Call `.into()` on them to convert them into a `Color` for quick debugging use, and consider using the much prettier `tailwind` palette for prototyping. - The `LIME_GREEN` color has been renamed to `LIMEGREEN` to comply with the standard naming. - Vector field arithmetic operations on `Color` (add, subtract, multiply and divide by a f32) have been removed. Instead, convert your colors into `LinearRgba` space, and perform your operations explicitly there. This is particularly relevant when working with emissive or HDR colors, whose color channel values are routinely outside of the ordinary 0 to 1 range. - `Color::as_linear_rgba_f32` has been removed. Call `LinearRgba::to_f32_array` instead, converting if needed. - `Color::as_linear_rgba_u32` has been removed. Call `LinearRgba::to_u32` instead, converting if needed. - Several other color conversion methods to transform LCH or HSL colors into float arrays or `Vec` types have been removed. Please reimplement these externally or open a PR to re-add them if you found them particularly useful. - Various methods on `Color` such as `rgb` or `hsl` to convert the color into a specific color space have been removed. Convert into `LinearRgba`, then to the color space of your choice. - Various implicitly-converting color value methods on `Color` such as `r`, `g`, `b` or `h` have been removed. Please convert it into the color space of your choice, then check these properties. - `Color` no longer implements `AsBindGroup`. Store a `LinearRgba` internally instead to avoid conversion costs. --------- Co-authored-by: Alice Cecile <[email protected]> Co-authored-by: Afonso Lage <[email protected]> Co-authored-by: Rob Parrett <[email protected]> Co-authored-by: Zachary Harrold <[email protected]>
This ticket outlines a plan for integrating bevy_color into the rest of Bevy. The plan consists of several phases.
Phase 1
In Phase 1, the public-facing Bevy APIs will not be changed, but the bevy_color crate will be integrated in ways that do not impact end users. During this phase, the following tasks will be performed:
Color
type toLegacyColor
, and then provide a type aliasColor = LegacyColor
. The purpose of this change is to allow measurement of the migration progress by searching for the symbolLegacyColor
. Because of the type alias, no end-user code will be broken. This step should be performed by an SME so that it can be done atomically, minimizing merge conflicts with any in-flight PRs that might be impacted.LegacyColor
that are not exposed to end users, especially constant color values (I counted 601 color constants in the current code base).From
implementations forLegacyColor
into the same module asLegacyColor
.Phase 2
In Phase 2, we will modify Bevy APIs, but only the ones that can be modified in a backwards-compatible way. For example, the Gizmo APIs which accept a
LegacyColor
argument can be changed to acceptimpl Into<LinearRgba>
, allowing either the old or new color types to be passed in without needing to call.into()
.Also during this phase, we'll add new APIs for bundle types such as
Fog
orBackgroundColor
which currently accept aLegacyColor
as a property - the new APIs will have constructors that take animpl Into<LinearRgba>
. This means that users who want to use the new color types can do so in a forward-compatible way.Phase 3
In Phase 3, the remaining APIs - the ones that cannot be made backwards compatible - will be migrated. This includes things like bundle properties. In most cases, low-level color attributes will use the
LinearRgba
type.Users will need to update their code, at minimum adding a call to
.into()
when passing a legacy color into an API that accepts the new color types.Timing
I'm assuming that these three phases will be spread out over several Bevy release cycles. The reason for this is that we'll want to give users plenty of warning that this breaking change is coming. However, this decision - whether to try and fit all of the migration into a single Bevy release cycle or not - is above my pay grade.
The text was updated successfully, but these errors were encountered: