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

[Merged by Bors] - Rename CameraUi #5234

Closed
wants to merge 1 commit into from

Conversation

nicopap
Copy link
Contributor

@nicopap nicopap commented Jul 6, 2022

Objective

In bevy 0.7, CameraUi was a component specifically added to cameras
that display the UI. Since camera-driven rendering was merged, it
actually does the opposite! This will make it difficult for current
users to adapt to 0.8.

Solution

To avoid unnecessary confusion, we rename CameraUi into
UiCameraConfig.


Changelog

  • Rename CameraUi to UiCameraConfig

@alice-i-cecile alice-i-cecile added A-Rendering Drawing game state to the screen C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Jul 6, 2022
@alice-i-cecile alice-i-cecile added this to the Bevy 0.8 milestone Jul 6, 2022
@nicopap nicopap force-pushed the cameraui-clarification branch 2 times, most recently from 00aff22 to aa9593d Compare July 7, 2022 05:43
@nicopap nicopap force-pushed the cameraui-clarification branch 2 times, most recently from bef8285 to 9608cb9 Compare July 7, 2022 10:51
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

I would probably prefer UiCameraConfig, but I won't block on that.

@nicopap
Copy link
Contributor Author

nicopap commented Jul 7, 2022

I much prefer UiCameraConfig

In bevy 0.7, `CameraUi` was a component specifically added to cameras
that display the UI. Since camera-driven rendering was merged, it
actually does the opposite! This will make it difficult for current
users to adapt to 0.8.

To avoid unnecessary confusion, we rename `CameraUi` into
`UiCameraConfig`.
@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide labels Jul 7, 2022
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.

Works for me! Not a huge fan of swapping if let Some(...) = foo {} for if matches!(foo, Some(...)) {}. if let is both more ergonomic and a "built in language feature" instead of a "built in macro". All other things equal, if I ever have the option not to use a macro, I take it.

That being said. Not worth blocking on.

@cart
Copy link
Member

cart commented Jul 8, 2022

bors r+

bors bot pushed a commit that referenced this pull request Jul 8, 2022
# Objective

In bevy 0.7, `CameraUi` was a component specifically added to cameras
that display the UI. Since camera-driven rendering was merged, it
actually does the opposite! This will make it difficult for current
users to adapt to 0.8.

## Solution

To avoid unnecessary confusion, we rename `CameraUi` into
`UiCameraConfig`.

---

## Changelog

- Rename `CameraUi` to `UiCameraConfig`
@bors bors bot changed the title Rename CameraUi [Merged by Bors] - Rename CameraUi Jul 8, 2022
@bors bors bot closed this Jul 8, 2022
@nicopap
Copy link
Contributor Author

nicopap commented Jul 8, 2022

@cart I feel matches! is the most appropriate when the pattern has no variable bindings. let implies a binding, and it's always surprising (at least to me) to see a if let followed by no variable being declared. Additionally if let allows the potential of adding new variables to the scope, so it's extra overhead to have to process them when reading through code. matches! solves that, because you cannot bind variables in it. But that can be pinned down to personal preferences and I wasn't aware it was a conscious choice and I'll keep that in mind for future PRs.

inodentry pushed a commit to IyesGames/bevy that referenced this pull request Aug 8, 2022
# Objective

In bevy 0.7, `CameraUi` was a component specifically added to cameras
that display the UI. Since camera-driven rendering was merged, it
actually does the opposite! This will make it difficult for current
users to adapt to 0.8.

## Solution

To avoid unnecessary confusion, we rename `CameraUi` into
`UiCameraConfig`.

---

## Changelog

- Rename `CameraUi` to `UiCameraConfig`
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 28, 2022
# Objective

In bevy 0.7, `CameraUi` was a component specifically added to cameras
that display the UI. Since camera-driven rendering was merged, it
actually does the opposite! This will make it difficult for current
users to adapt to 0.8.

## Solution

To avoid unnecessary confusion, we rename `CameraUi` into
`UiCameraConfig`.

---

## Changelog

- Rename `CameraUi` to `UiCameraConfig`
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective

In bevy 0.7, `CameraUi` was a component specifically added to cameras
that display the UI. Since camera-driven rendering was merged, it
actually does the opposite! This will make it difficult for current
users to adapt to 0.8.

## Solution

To avoid unnecessary confusion, we rename `CameraUi` into
`UiCameraConfig`.

---

## Changelog

- Rename `CameraUi` to `UiCameraConfig`
@nicopap nicopap deleted the cameraui-clarification branch September 5, 2023 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide C-Usability A targeted quality-of-life change that makes Bevy easier to use 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.

3 participants