bevyengine / bevy

A refreshingly simple data-driven game engine built in Rust
https://bevyengine.org
Apache License 2.0
36.15k stars 3.57k forks source link

bevy_color migration plan #12056

Closed viridia closed 8 months ago

viridia commented 8 months ago

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:

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 accept impl 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 or BackgroundColor which currently accept a LegacyColor as a property - the new APIs will have constructors that take an impl 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.

alice-i-cecile commented 8 months ago

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.

bushrat011899 commented 8 months ago

I think it would be beneficial to end-users to have at least 1 Bevy release where the LegacyColor is available but depreciated. If we're going as fast as possible:

  1. Complete Phase 1 (bevy_color available but "unused")
  2. Complete Phase 2 (bevy_color used internally)
  3. Mark LegacyColor as deprecated
  4. Release 0.14
  5. Complete Phase 3 (bevy_color fully adopted)
  6. Remove LegacyColor
  7. Release 0.15

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 bevy_color prior to it being "the only choice".

viridia commented 8 months ago

@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.

viridia commented 8 months ago

Also note that LegacyColor will continue to work with the new APIs after migration, but will require an explicit .into(). This might be useful for large projects that use LegacyColor extensively at the app level, and for which changing all of their app code might be a burden. So one option is to keep around LegacyColor for one more release cycle, even though no APIs are actually using it.

cart commented 8 months ago

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 Color to LegacyColor, so we're already breaking peoples' code. I don't think the color-representation change is complicated enough to be significantly more work than the rename operation.

I think we should:

  1. Merge bevy_color as is (without porting)
  2. Create a new PR that fully ports Bevy to bevy_color, using the new Color enum in place of the current Color enum, and making uncontroversial ports (such as using LinearSrgba in the Render App). Merge the PR (and export bevy_color as bevy::color) only when we are confident that bevy_color is ready / an improvement over the status quo.
  3. Create other PRs for specific, more controversial changes (such as replacing StandardMaterial::color with LinearSrgb or Sprite::color with Srgba)
viridia commented 8 months ago

@cart Renaming Color to LegacyColor isn't going to break users code by itself, because we are also at the same time going to add a type alias which defines Color = LegacyColor. You might ask then, what's the point? The answer is that it gives us a way to track which parts of the code base have been migrated. If we have two different types named Color it becomes very hard to search for one and not the other. Renaming solves that problem.

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 Color in the Bevy code base, not counting the implementation of Color itself or documentation files. That's not going to all happen overnight.

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.

cart commented 8 months ago

Renaming Color to LegacyColor isn't going to break users code by itself, because we are also at the same time going to add a type alias which defines Color = LegacyColor

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).

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.

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).

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).

Oooh interesting. I'm semi-familiar with that topic. I didn't know you were involved. Very cool!

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.

Agreed!

For each of those groups, we can then propose a consistent rule for incorporating color arguments

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:

  1. Committing to an API before we know its fitness for our codebase. Aka merging a partial port of the codebase, only to discover a later case isn't well suited to the API we have chosen.
  2. Having a partial port on main for a long period of time, complicating the lives of consumers of main
  3. Hitting the release deadline while still in a "partially ported" situation.
viridia commented 8 months ago

@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 Srgba to Color. I want to quote Albert Einstein here: "Things should be as simple as possible, but no simpler." I'm all for making things simple for new users, but we should avoid hiding important details that they need to know. Color spaces are something that eventually users are going to need some awareness of - maybe not the more esoteric spaces like Oklab, but an understanding of standard vs. linear rgba is pretty fundamental in graphics work. I would rather take the approach of writing really good docs (which I can do) that teaches them what they need to know, rather than sweeping this complexity under the rug. EIBTI.

cart commented 8 months ago

BTW, I want to push back on the idea of renaming Srgba to Color. I want to quote Albert Einstein here: "Things should be as simple as possible, but no simpler." I'm all for making things simple for new users, but we should avoid hiding important details that they need to know. Color spaces are something that eventually users are going to need some awareness of - maybe not the more esoteric spaces like Oklab, but an understanding of standard vs. linear rgba is pretty fundamental in graphics work. I would rather take the approach of writing really good docs (which I can do) that teaches them what they need to know, rather than sweeping this complexity under the rug. EIBTI.

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 Color likely wants a less friendly name if we're going to encourage the use of Srgba / LinearSrgba naming.

cart commented 8 months ago

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.

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.

cart commented 8 months ago

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.

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 bevy_color crate directly.

alice-i-cecile commented 8 months ago

My bikeshed opinions:

  1. We should do the rename to LegacyColor to make the refactor less painful.
  2. We should not rename Srgba to Color
  3. We should rename bevy_color::Color to something less appealing, and exclude it from the prelude.
  4. All component types should store a linear RGB color, and users should be encouraged to use .into() from the color space of their choice.
  5. We should ship this entire change in a single release: deprecating the old Color type will increase confusion without making the migration easier.
  6. We should not migrate in a single PR: each important question should get its own PR to ease reviewability.
alice-i-cecile commented 8 months ago

After doing #12069, I have a pretty good sense of our current uses of color. They are:

  1. Global configurable data to be passed directly to the renderer (e.g. fog, gizmos).
  2. Components that store a configurable color (e.g. UiColor, Sprite).
  3. Special case data hardcoded in rendering internals (White/Black/None clear colors are virtually all of it).
  4. Arbitrary color constants chosen to make the colors look nice. Usually specified in RGB space.

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:

  1. Polymorphic color type (like before): annoying to right, probably slower, requires conversion. Easy to make nice in an editor and in scenes (thanks @JMS55 for pointing this out), since you can quickly see exactly what value you set, work in different color spaces and parse values.
  2. Standard RGBA: plays nice with color pickers online. The "obvious" and familiar choice, but still requires conversions.
  3. Linear RGBA: avoid constant conversions for rendering. Will give subtly different values than expected relative to color pickers.

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 impl Into constructors on the component types.

viridia commented 8 months ago

@alice-i-cecile Do we want to do the migration work in a branch?

alice-i-cecile commented 8 months ago

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.

cart commented 8 months ago

All component types should store a linear RGB color, and users should be encouraged to use .into() from the color space of their choice.

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 (material.color.r) without needing to think about space conversions. From the start to the end of the pipeline, linear color makes sense / will be preferable.

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.

alice-i-cecile commented 8 months ago

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:

  1. No constant reconversions.
  2. Just over half the memory usage of storing the cached form and the polymorphic types.
  3. Still uses the .into() methods, which makes it clear that there's a small cost to constructing non-linear color types.

Cons vs the polymorphic type:

  1. Can't create const colors in other color spaces.
  2. A bit more complex to teach.
  3. Trickier to define helper methods like .r(): does this mean linear or standard red?
  4. Harder to get to the underlying raw color types.

Overall I like the polymorphic and the tagged linear rgb options best.

viridia commented 8 months ago

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.

viridia commented 8 months ago

@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 Color enum type, which lived in bevy_render, was very flexible, but had a number of long-standing problems:

To address these issues, there is now a new bevy_color crate that is dedicated to representing and manipulating colors in a robust way. This crate features distinct Rust types for each different color model, which now includes a grand total of ten color types:

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 Srgba to Hsla is as easy as Srgba::from(hsla_color). The crate supports efficient conversions from any color space to any other.

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, bevy_color also provides a Color type, which is an enum of all of the supported color spaces. However, we anticipate that this "polymorphic" color type won't be needed nearly as much as before.

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, bevy_color::palettes:

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 impl Into, which means you can pass in any color type and it will be converted automatically.

(After this, add a section summarizing the changes to Bevy APIs).