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

Migrate from LegacyColor to bevy_color::Color #12163

Merged
merged 58 commits into from
Feb 29, 2024

Conversation

alice-i-cecile
Copy link
Member

@alice-i-cecile alice-i-cecile commented Feb 27, 2024

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 bevy_color migration plan #12056.

Solution

I've chosen to use the polymorphic Color type as our standard user-facing API.

  • Migrate bevy_gizmos.
  • Take impl Into<Color> in all bevy_gizmos APIs
  • Migrate sprites
  • Migrate UI
  • Migrate ColorMaterial
  • Migrate MaterialMesh2D
  • Migrate fog
  • Migrate lights
  • Migrate StandardMaterial
  • Migrate wireframes
  • Migrate clear color
  • Migrate text
  • Migrate gltf loader
  • Register color types for reflection
  • Remove LegacyColor
  • 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.

@alice-i-cecile alice-i-cecile added A-Rendering Drawing game state to the screen A-UI Graphical user interfaces, styles, layouts, and widgets C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide A-Gizmos Visual editor and debug gizmos A-Text Rendering and layout for characters C-Code-Quality A section of code that is hard to understand or change C-Usability A simple quality-of-life change that makes Bevy easier to use X-Controversial There is active debate or serious implications around merging this PR labels Feb 27, 2024
@alice-i-cecile alice-i-cecile added this to the 0.14 milestone Feb 27, 2024
crates/bevy_gizmos/src/arcs.rs Outdated Show resolved Hide resolved
crates/bevy_gizmos/src/arrows.rs Outdated Show resolved Hide resolved
crates/bevy_gizmos/src/arrows.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@bushrat011899 bushrat011899 left a comment

Choose a reason for hiding this comment

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

Looking good so far!

crates/bevy_gizmos/src/gizmos.rs Show resolved Hide resolved
examples/2d/bounding_2d.rs Show resolved Hide resolved
crates/bevy_gizmos/src/aabb.rs Show resolved Hide resolved
) -> Arc2dBuilder<'_, 'w, 's, T> {
Arc2dBuilder {
gizmos: self,
position,
direction_angle,
arc_angle,
radius,
color,
color: color.into(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Might need to do some performance testing to confirm, but we probably need to go back over the various From implementations an annotate them with #[inline]. I'm not sure how good the Rust compiler is at inlining these tiny functions.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah agreed. I want to do perf testing towards the end of this process.

github-merge-queue bot pushed a commit that referenced this pull request Feb 28, 2024
# Objective

As suggested in #12163 by @cart, we should add convenience constructors
to `bevy_color::Color` to match the existing API (easing migration pain)
and generally improve ergonomics.

## Solution

- Add `const fn Color::rgba(red, green, blue, alpha)` and friends, which
directly construct the appropriate variant.
- Add `const fn Color::rgb(red, green, blue)` and friends, which impute
and alpha value of 1.0.
- Add `const BLACK, WHITE, NONE` to `Color`. These are stored in
`LinearRgba` to reduce pointless conversion costs and inaccuracy.
- Changed the default `Color` from `Srgba::WHITE` to the new linear
equivalent for the same reason.

---------

Co-authored-by: Alice Cecile <alice.i.cecil@gmail.com>
crates/bevy_gizmos/src/gizmos.rs Show resolved Hide resolved
github-merge-queue bot pushed a commit that referenced this pull request Mar 1, 2024
# Objective

- bloom is not working anymore in 3d after
#12163

## Solution

- Fix a copy paste mistake and use emissive for the emissive
github-merge-queue bot pushed a commit that referenced this pull request Mar 2, 2024
# Objective

- Copy paste error in #12163

## Solution

- Fix it
spectria-limina pushed a commit to spectria-limina/bevy that referenced this pull request Mar 9, 2024
)

# Objective

- bloom is not working anymore in 3d after
bevyengine#12163

## Solution

- Fix a copy paste mistake and use emissive for the emissive
spectria-limina pushed a commit to spectria-limina/bevy that referenced this pull request Mar 9, 2024
@NthTensor NthTensor mentioned this pull request Mar 19, 2024
@BD103 BD103 added the A-Color Color spaces and color math label May 3, 2024
@alice-i-cecile alice-i-cecile removed the C-Needs-Release-Note Work that should be called out in the blog due to impact label May 27, 2024
ChristopherBiscardi added a commit to ChristopherBiscardi/bevy_ecs_tilemap that referenced this pull request Jun 7, 2024
[12163](bevyengine/bevy#12163) bevy_color was created and Color handling has changed. Specifically Color::as_linear_rgba_f32 has been removed.

LinearRgba is now its own type that can be accessed via [`linear()`](https://docs.rs/bevy/0.14.0-rc.2/bevy/color/enum.Color.html#method.linear) and then converted.
zhaop added a commit to zhaop/bevy_editor_pls that referenced this pull request Jun 24, 2024
zhaop added a commit to zhaop/bevy_editor_pls that referenced this pull request Jun 24, 2024
zhaop added a commit to zhaop/bevy_editor_pls that referenced this pull request Jun 25, 2024
rparrett added a commit to StarArawn/bevy_ecs_tilemap that referenced this pull request Jul 5, 2024
* Update to 0.14.0-rc.2

* [12997](bevyengine/bevy#12997): rename `multi-threaded` to `multi_threaded`

* RenderAssets<Image> is now RenderAssets<GpuImage>

Implemented in [12827](bevyengine/bevy#12827)

* FloatOrd is now in bevy_math

implemented in [12732](bevyengine/bevy#12732)

* convert Transparent2d::dynamic_offset to extra_index

[12889](bevyengine/bevy#12889) Gpu Frustum Culling removed the dynamic_offset of Transparent2d and it became `extra_index` with the special value `PhaseItemExtraIndex::NONE`, which indicates the `None` that was here previously

* RenderPhase<Transparent2d> -> ViewSortedRenderPhases<Transparent2d>

[12453](https://github.com/StarArawn/bevy_ecs_tilemap/pull/bevyengine/bevy#12453): Render phases are now binned or sorted.

Following the changes in the `mesh2d_manual` [example](https://github.com/bevyengine/bevy/blob/ecdd1624f302c5f71aaed95b0984cbbecf8880b7/examples/2d/mesh2d_manual.rs#L357-L358): use the `ViewSortedRenderPhases` resource.

* get_sub_app_mut is now an Option

in [9202](https://github.com/StarArawn/bevy_ecs_tilemap/pull/bevyengine/bevy/pull/9202) SubApp access has changed

* GpuImage::size f32 -> u32 via UVec2

[11698](bevyengine/bevy#11698) changed `GpuImage::size` to `UVec2`.

Right above this, `Extent3d` does the same thing, so I'm taking a small leap and assuming can `as`.

* GpuMesh::primitive_topology -> key_bits/BaseMeshPipeline

[12791](bevyengine/bevy#12791) the `primitive_topology` field on `GpuMesh` was removed in favor of `key_bits` which can be constructed using `BaseMeshPipeline::from_primitive_topology`

* RenderChunk2d::prepare requires &mut MeshVertexBufferLayouts now

[12216](bevyengine/bevy#12216) introduced an argument `&mut MeshVertexBufferLayouts` to `get_mesh_vertex_buffer_layout`, which bevy_ecs_tilemap calls in `RenderChunk2d::prepare`

* into_linear_f32 -> color.0.linear().to_f32_array(),

[12163](bevyengine/bevy#12163) bevy_color was created and Color handling has changed. Specifically Color::as_linear_rgba_f32 has been removed.

LinearRgba is now its own type that can be accessed via [`linear()`](https://docs.rs/bevy/0.14.0-rc.2/bevy/color/enum.Color.html#method.linear) and then converted.

* Must specify type of VisibleEntities when accessing

[12582](bevyengine/bevy#12582) divided `VisibleEntities` into separate lists. So now we have to specify which kind of entity we want. I think we want the Mesh here, and I think we can get rid of the `.index` calls on Entity since Entity [already compares bits](https://docs.rs/bevy_ecs/0.14.0-rc.2/src/bevy_ecs/entity/mod.rs.html#173) for optimized codegen purposes. Waiting to do that until the other changes are in though so as to not change functionality until post-upgrade.

* app.world access is functions now

- [9202](bevyengine/bevy#9202) changed world access to functions. [relevent line](https://github.com/bevyengine/bevy/pull/9202/files#diff-b2fba3a0c86e496085ce7f0e3f1de5960cb754c7d215ed0f087aa556e529f97fR640)
- This also surfaced [12655](bevyengine/bevy#12655) which removed `Into<AssetId<T>>` for `Handle<T>`. using a reference or .id() is the solution here.

* We don't need `World::cell`, and it doesn't exist anymore

In [12551](bevyengine/bevy#12551) `WorldCell` was removed.

...but it turns out we don't need it or its replacement anyway.

* examples error out unless this bevy bug is addressed with these features being added

bevyengine/bevy#13728

* check_visibility is required for the entity that is renderable

As a result of [12582](bevyengine/bevy#12582) `check_visibility` must be implemented for the "renderable" tilemap entities. Doing this is trivial by taking advantage of the
existing `check_visibility` type arguments, which accept a [`QF: QueryFilter + 'static`](https://docs.rs/bevy/0.14.0-rc.2/bevy/render/view/fn.check_visibility.html).

The same `QueryFilter`` is used when checking `VisibleEntities`. I've chosen `With<TilemapRenderSettings` because presumably if the entity doesn't have a `TilemapRenderSettings` then it will not be rendering, but this could be as sophisticated or simple as we want.

For example `WithLight` is currently implemented as

```rust
pub type WithLight = Or<(With<PointLight>, With<SpotLight>, With<DirectionalLight>)>;
```

* view.view_proj -> view.clip_from_world

[13289](bevyengine/bevy#13489) introduced matrix naming changes, including `view_proj` which becomes `clip_from_world`

* color changes to make tests runnable

* clippy fix

* Update Cargo.toml

Co-authored-by: Rob Parrett <robparrett@gmail.com>

* Update Cargo.toml

Co-authored-by: Rob Parrett <robparrett@gmail.com>

* final clippy fixes

* Update Cargo.toml

Co-authored-by: Rob Parrett <robparrett@gmail.com>

* Simplify async loading in ldtk/tiled helpers

See Bevy #12550

* remove second allow lint

* rc.3 bump

* bump version for major release

* remove unused features

---------

Co-authored-by: Rob Parrett <robparrett@gmail.com>
zhaop added a commit to zhaop/transform-gizmo that referenced this pull request Jul 6, 2024
zhaop added a commit to zhaop/transform-gizmo that referenced this pull request Jul 6, 2024
zhaop added a commit to zhaop/bevy_editor_pls that referenced this pull request Jul 7, 2024
jakobhellermann pushed a commit to jakobhellermann/bevy_editor_pls that referenced this pull request Aug 11, 2024
jakobhellermann pushed a commit to jakobhellermann/bevy_editor_pls that referenced this pull request Aug 11, 2024
jakobhellermann pushed a commit to jakobhellermann/bevy_editor_pls that referenced this pull request Aug 11, 2024
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 A-Gizmos Visual editor and debug gizmos A-Rendering Drawing game state to the screen A-Text Rendering and layout for characters A-UI Graphical user interfaces, styles, layouts, and widgets C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide C-Code-Quality A section of code that is hard to understand or change C-Usability A simple quality-of-life change that makes Bevy easier to use D-Complex Quite challenging from either a design or technical perspective. Ask for help! 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
Development

Successfully merging this pull request may close these issues.

bevy_color migration plan