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] - Support arbitrary RenderTarget texture formats #6380

Closed
wants to merge 4 commits into from

Conversation

cart
Copy link
Member

@cart cart commented Oct 26, 2022

Objective

Currently, Bevy only supports rendering to the current "surface texture format". This means that "render to texture" scenarios must use the exact format the primary window's surface uses, or Bevy will crash. This is even harder than it used to be now that we detect preferred surface formats at runtime instead of using hard coded BevyDefault values.

Solution

  1. Look up and store each window surface's texture format alongside other extracted window information
  2. Specialize the upscaling pass on the current RenderTarget's texture format, now that we can cheaply correlate render targets to their current texture format
  3. Remove the old SurfaceTextureFormat and AvailableTextureFormats: these are now redundant with the information stored on each extracted window, and probably should not have been globals in the first place (as in theory each surface could have a different format).

This means you can now use any texture format you want when rendering to a texture! For example, changing the render_to_texture example to use R16Float now doesn't crash / properly only stores the red component:
image

@cart cart added this to the Bevy 0.9 milestone Oct 26, 2022
@cart cart added A-Rendering Drawing game state to the screen C-Enhancement A new feature labels Oct 26, 2022
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.

These changes make sense to me, code quality is good, and the explanation is solid.

@mockersf mockersf 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 Oct 26, 2022
@cart
Copy link
Member Author

cart commented Oct 26, 2022

bors r+

bors bot pushed a commit that referenced this pull request Oct 26, 2022
# Objective

Currently, Bevy only supports rendering to the current "surface texture format". This means that "render to texture" scenarios must use the exact format the primary window's surface uses, or Bevy will crash. This is even harder than it used to be now that we detect preferred surface formats at runtime instead of using hard coded BevyDefault values.

## Solution

1. Look up and store each window surface's texture format alongside other extracted window information
2. Specialize the upscaling pass on the current `RenderTarget`'s texture format, now that we can cheaply correlate render targets to their current texture format
3. Remove the old `SurfaceTextureFormat` and `AvailableTextureFormats`: these are now redundant with the information stored on each extracted window, and probably should not have been globals in the first place (as in theory each surface could have a different format). 

This means you can now use any texture format you want when rendering to a texture! For example, changing the `render_to_texture` example to use `R16Float` now doesn't crash / properly only stores the red component:
![image](https://user-images.githubusercontent.com/2694663/198140125-c606dd0e-6fdf-4544-b93d-dbbd10dbadd2.png)
@bors bors bot changed the title Support arbitrary RenderTarget texture formats [Merged by Bors] - Support arbitrary RenderTarget texture formats Oct 26, 2022
@bors bors bot closed this Oct 26, 2022
@@ -326,6 +326,22 @@ impl RenderTarget {
}
}

/// Retrieves the [`TextureFormat`] of this render target, if it exists.
pub fn get_texture_format<'a>(
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm slightly too late, but shouldn't this be called something like get_render_target_texture_format? And we could probably drop the get prefixes from this and similar functions.

Copy link
Member

Choose a reason for hiding this comment

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

no firm position on my side on naming, but don't hesitate to open follow up PRs 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

We sometimes use "get" when either: (1) an option is returned or (2) it isn't a direct property of the object. This sort of falls into both categories, but I don't have super strong opinions here.

On the topic of get_render_target_texture_format: the "render target" is implicit as this type is RenderTarget. We wouldn't rename Window::position() to Window::window_position for the same reason.

@mockersf mockersf added the hacktoberfest-accepted A PR that was accepted for Hacktoberfest, an annual open source event label Oct 27, 2022
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 28, 2022
# Objective

Currently, Bevy only supports rendering to the current "surface texture format". This means that "render to texture" scenarios must use the exact format the primary window's surface uses, or Bevy will crash. This is even harder than it used to be now that we detect preferred surface formats at runtime instead of using hard coded BevyDefault values.

## Solution

1. Look up and store each window surface's texture format alongside other extracted window information
2. Specialize the upscaling pass on the current `RenderTarget`'s texture format, now that we can cheaply correlate render targets to their current texture format
3. Remove the old `SurfaceTextureFormat` and `AvailableTextureFormats`: these are now redundant with the information stored on each extracted window, and probably should not have been globals in the first place (as in theory each surface could have a different format). 

This means you can now use any texture format you want when rendering to a texture! For example, changing the `render_to_texture` example to use `R16Float` now doesn't crash / properly only stores the red component:
![image](https://user-images.githubusercontent.com/2694663/198140125-c606dd0e-6fdf-4544-b93d-dbbd10dbadd2.png)
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective

Currently, Bevy only supports rendering to the current "surface texture format". This means that "render to texture" scenarios must use the exact format the primary window's surface uses, or Bevy will crash. This is even harder than it used to be now that we detect preferred surface formats at runtime instead of using hard coded BevyDefault values.

## Solution

1. Look up and store each window surface's texture format alongside other extracted window information
2. Specialize the upscaling pass on the current `RenderTarget`'s texture format, now that we can cheaply correlate render targets to their current texture format
3. Remove the old `SurfaceTextureFormat` and `AvailableTextureFormats`: these are now redundant with the information stored on each extracted window, and probably should not have been globals in the first place (as in theory each surface could have a different format). 

This means you can now use any texture format you want when rendering to a texture! For example, changing the `render_to_texture` example to use `R16Float` now doesn't crash / properly only stores the red component:
![image](https://user-images.githubusercontent.com/2694663/198140125-c606dd0e-6fdf-4544-b93d-dbbd10dbadd2.png)
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-Enhancement A new feature hacktoberfest-accepted A PR that was accepted for Hacktoberfest, an annual open source event 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.

None yet

4 participants