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

Rename bevy_render::Color to LegacyColor #12069

Merged
merged 29 commits into from
Feb 24, 2024

Conversation

alice-i-cecile
Copy link
Member

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!

@alice-i-cecile alice-i-cecile added A-Rendering Drawing game state to the screen A-UI Graphical user interfaces, styles, layouts, and widgets M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide labels Feb 23, 2024
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.

I'm on board, minus a couple of "higher level concept" renames, which feel unnecessary to me.

@alice-i-cecile
Copy link
Member Author

Yeah, those extra renames were just a mistake. Thanks for catching them!

@alice-i-cecile alice-i-cecile requested a review from cart February 24, 2024 01:55
@alice-i-cecile
Copy link
Member Author

Cleaned up those accidental renames, and did another pass myself in the diff view to catch any other mistakes. I think this should be good now.

Copy link
Contributor

@pablo-lua pablo-lua left a comment

Choose a reason for hiding this comment

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

did a simple pass judging just by names on every file. There is some points that I have a question about, but thats fairly simple :)

Comment on lines -62 to 63
pub const ALICE_BLUE: Color = Color::rgb(0.94, 0.97, 1.0);
pub const ALICE_BLUE: LegacyColor = LegacyColor::rgb(0.94, 0.97, 1.0);
/// <div style="background-color:rgb(98%, 92%, 84%); width: 10px; padding: 10px; border: 1px solid;"></div>
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a reason to not use Self in the type assign of this consts? could simplify further work on this area maybe.

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, it's not a bad choice. I wanted to keep this PR as simple as possible though.

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Feb 24, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Feb 24, 2024
Merged via the queue into bevyengine:main with commit de004da Feb 24, 2024
24 checks passed
msvbg pushed a commit to msvbg/bevy that referenced this pull request Feb 26, 2024
# 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 <alice.i.cecil@gmail.com>
msvbg pushed a commit to msvbg/bevy that referenced this pull request Feb 26, 2024
# 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 <alice.i.cecil@gmail.com>
@BD103 BD103 added the A-Color Color spaces and color math label May 3, 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-Rendering Drawing game state to the screen A-UI Graphical user interfaces, styles, layouts, and widgets M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide 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.

6 participants