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

Make Viewport::default() return a 1x1 viewport #14372

Merged
merged 1 commit into from
Jul 20, 2024

Conversation

SludgePhD
Copy link
Contributor

Objective

Solution

  • Viewport::default() now returns a 1x1 viewport

Testing

  • I modified the 3d_viewport_to_world example to use Viewport::default(), and it works as expected (only the top-left pixel is rendered)

@TrialDragon TrialDragon added C-Bug An unexpected or incorrect behavior D-Trivial Nice and easy! A great choice to get started with Bevy A-Rendering Drawing game state to the screen S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jul 17, 2024
@BD103 BD103 added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Jul 18, 2024
Copy link
Contributor

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Could you add some context on what users should update when this change get released in a new version of Bevy?
It will be used to help writing the migration guide for the version. Putting it after a ## Migration Guide will help it get automatically picked up by our tooling.

@BD103 BD103 added this to the 0.14.1 milestone Jul 18, 2024
@BD103
Copy link
Member

BD103 commented Jul 18, 2024

This is technically a breaking change, but it should probably be added to 0.14.1 because it had incorrect behavior before.

@SludgePhD
Copy link
Contributor Author

This is technically a breaking change

How so? A change like this wouldn't normally be considered breaking.

@BD103
Copy link
Member

BD103 commented Jul 18, 2024

This is technically a breaking change

How so? A change like this wouldn't normally be considered breaking.

A breaking change doesn't have to be something that would make code fail to compile, it can also be a behavioral change. In this case, someone may depend on the fact that Viewport::default().physical_size is (0, 0). (In practice, this probably will never happen, but it's still good to make a note of it just in case. The worst thing that can happen is that we don't document it, and then someone cannot figure out why their code no longer works.)

@SludgePhD
Copy link
Contributor Author

A breaking change doesn't have to be something that would make code fail to compile, it can also be a behavioral change. In this case, someone may depend on the fact that Viewport::default().physical_size is (0, 0). (In practice, this probably will never happen, but it's still good to make a note of it just in case. The worst thing that can happen is that we don't document it, and then someone cannot figure out why their code no longer works.)

Yeah, but that's pushing the definition of "breaking" too far IMO. Not my project though, so I was mostly asking to know if this requires some action on my part.

@BD103 BD103 removed the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Jul 18, 2024
@BD103
Copy link
Member

BD103 commented Jul 18, 2024

No, you're right. I thought about it some more, and since the behavior was never right in the first place, it should probably not be considered breaking. Sorry for the confusion!

@BD103 BD103 added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jul 20, 2024
@mockersf mockersf added this pull request to the merge queue Jul 20, 2024
Merged via the queue into bevyengine:main with commit c0cebfe Jul 20, 2024
28 checks passed
@SludgePhD SludgePhD deleted the default-viewport branch July 20, 2024 15:57
mockersf pushed a commit that referenced this pull request Aug 2, 2024
# Objective

- The current default viewport crashes bevy due to a wgpu validation
error, this PR fixes that
- Fixes #14355

## Solution

- `Viewport::default()` now returns a 1x1 viewport

## Testing

- I modified the `3d_viewport_to_world` example to use
`Viewport::default()`, and it works as expected (only the top-left pixel
is rendered)
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-Bug An unexpected or incorrect behavior D-Trivial Nice and easy! A great choice to get started with Bevy 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.

Viewport::default() produces a viewport that always crashes
5 participants