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 camera "priority" to "order" #6908

Closed
wants to merge 4 commits into from

Conversation

Aceeri
Copy link
Member

@Aceeri Aceeri commented Dec 10, 2022

Objective

The documentation for camera priority is very confusing at the moment, it requires a bit of "double negative" kind of thinking.

Solution

Flipping the wording on the documentation to reflect more common usecases like having an overlay camera and also renaming it to "order", since priority implies that it will override the other camera rather than have both run.

Changelog

Migration

  • Rename priority to order in usage of Camera.

@alice-i-cecile alice-i-cecile added C-Docs An addition or correction to our documentation A-Rendering Drawing game state to the screen labels Dec 11, 2022
@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 Dec 11, 2022
@tim-blackbird
Copy link
Contributor

tim-blackbird commented Dec 11, 2022

It's actually "lower number = higher priority", e.g. priority: -1 renders before priority: 0.
The cameras with lower priority(higher number) are rendered later, and thus on top of higher priority cameras (lower number).

There current docs are correct, but easy to misinterpret. I think a more verbose explanation is needed.

Feel free to yoink the first paragraph of this comment :)

@Aceeri
Copy link
Member Author

Aceeri commented Dec 11, 2022

I thought it would be that way given the docs, but the behavior I've seen from messing around is higher number is higher priority

@Aceeri
Copy link
Member Author

Aceeri commented Dec 11, 2022

Ohhh, never mind I understand, yeah I was very confused by that.

@alice-i-cecile alice-i-cecile removed 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 Dec 11, 2022
@Aceeri
Copy link
Member Author

Aceeri commented Dec 21, 2022

Maybe it should be renamed to ordering? instead of priority? Priority gives me the feeling it would override other cameras rather than they both run with one being ontop of the other.

@Aceeri Aceeri changed the title Fix camera priority documentation Rename camera "priority" to "ordering" Dec 21, 2022
Copy link
Contributor

@kurtkuehnert kurtkuehnert left a comment

Choose a reason for hiding this comment

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

Other than the small naming nit, LGTM.

crates/bevy_render/src/camera/camera.rs Outdated Show resolved Hide resolved
@Aceeri Aceeri changed the title Rename camera "priority" to "ordering" Rename camera "priority" to "order" Dec 24, 2022
@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 M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide labels Dec 24, 2022
@alice-i-cecile
Copy link
Member

This is a breaking change, and will need a simple migration guide entry. @Aceeri, ideally you can add that to your PR description.

bors r+

bors bot pushed a commit that referenced this pull request Dec 25, 2022
# Objective
The documentation for camera priority is very confusing at the moment, it requires a bit of "double negative" kind of thinking.

# Solution
Flipping the wording on the documentation to reflect more common usecases like having an overlay camera and also renaming it to "order", since priority implies that it will override the other camera rather than have both run.
@bors
Copy link
Contributor

bors bot commented Dec 25, 2022

Build failed (retrying...):

bors bot pushed a commit that referenced this pull request Dec 25, 2022
# Objective
The documentation for camera priority is very confusing at the moment, it requires a bit of "double negative" kind of thinking.

# Solution
Flipping the wording on the documentation to reflect more common usecases like having an overlay camera and also renaming it to "order", since priority implies that it will override the other camera rather than have both run.
@bors
Copy link
Contributor

bors bot commented Dec 25, 2022

Build failed (retrying...):

bors bot pushed a commit that referenced this pull request Dec 25, 2022
# Objective
The documentation for camera priority is very confusing at the moment, it requires a bit of "double negative" kind of thinking.

# Solution
Flipping the wording on the documentation to reflect more common usecases like having an overlay camera and also renaming it to "order", since priority implies that it will override the other camera rather than have both run.
@bors bors bot changed the title Rename camera "priority" to "order" [Merged by Bors] - Rename camera "priority" to "order" Dec 25, 2022
@bors bors bot closed this Dec 25, 2022
alradish pushed a commit to alradish/bevy that referenced this pull request Jan 22, 2023
# Objective
The documentation for camera priority is very confusing at the moment, it requires a bit of "double negative" kind of thinking.

# Solution
Flipping the wording on the documentation to reflect more common usecases like having an overlay camera and also renaming it to "order", since priority implies that it will override the other camera rather than have both run.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective
The documentation for camera priority is very confusing at the moment, it requires a bit of "double negative" kind of thinking.

# Solution
Flipping the wording on the documentation to reflect more common usecases like having an overlay camera and also renaming it to "order", since priority implies that it will override the other camera rather than have both run.
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-Docs An addition or correction to our documentation 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.

5 participants