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

Add Image constructor specialised for rendering to a texture #17209

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

tigregalis
Copy link
Contributor

@tigregalis tigregalis commented Jan 7, 2025

Objective

Fixes #7358
Redo of #7360

Ergonomics. There's a bunch of enigmatic boilerplate for constructing a texture for rendering to, which could be greatly simplified for the external user-facing API.

Solution

  • Take part of the render_to_target example and turn it into a new constructor for Image, with minimal changes beyond the Default implementation.
  • Update the render_to_target example to use the new API.

Strictly speaking, there are two small differences between the constructor and the example:

1. The example sets the size when initially constructing the Image, then resizes, but resize sets the size anyway so we don't need to do this extra step.

2. The example sets Image.texture_descriptor.format to TextureFormat::Bgra8UnormSrgb, but the default impl sets this to TextureFormat::Rgba8UnormSrgb via wgpu::TextureFormat::bevy_default(). I don't know what sort of impact this has, but it works on my machine.

I've deliberately chosen to only include width and height as parameters, but maybe it makes sense for some of the other properties to be exposed as parameters.


Changelog

Added

Added Image::new_target_texture constructor for simpler creation of render target textures.


Notes:

  • This is a re-do of Add Image constructor specialised for rendering to a texture #7360 - there's some relevant discussion on code style there.
  • The docs for the method want to refer to bevy_render::camera::Camera and bevy_render::camera::RenderTarget::Image. bevy_image used to be part of bevy_render and was split out in the past, and bevy_image doesn't depend on bevy_render. What's the recommendation here?

@tigregalis
Copy link
Contributor Author

@BenjaminBrienen I've "rebased" this

@chompaa chompaa added A-Rendering Drawing game state to the screen C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jan 7, 2025
@JMS55
Copy link
Contributor

JMS55 commented Jan 7, 2025

Shouldn't hard code to default the texture format. HDR cameras need a different format.

Copy link
Contributor

@IceSentry IceSentry left a comment

Choose a reason for hiding this comment

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

Can you also update headless_renderer, gpu_readback, and compute_shader_game_of_life

I think those are all the examples that use this pattern.

(Sorry, that wasn't an approve, just a comment)

@IceSentry IceSentry self-requested a review January 7, 2025 19:59
@IceSentry
Copy link
Contributor

IceSentry commented Jan 7, 2025

Shouldn't hard code to default the texture format. HDR cameras need a different format.

Yeah, this should either take a hdr parameter or just a texture format. A hdr parameter could be enough since this is just for simpler use cases.

@tigregalis
Copy link
Contributor Author

Shouldn't hard code to default the texture format. HDR cameras need a different format.

Yeah, this should either take a hdr parameter or just a texture format. A hdr parameter could be enough since this is just for simpler use cases.

So to clarify, the suggestion is to either:

  1. add a hdr parameter, I assume a bool, but if true, what's the intended TextureFormat variant here? Is it TextureFormat::AstcHdr { block: ???, AstcChannel::Hdr }
  2. have the user pass in the TextureFormat themselves, but same question as 1. what would the user use if they wanted to use a HDR camera?

@tigregalis
Copy link
Contributor Author

tigregalis commented Jan 8, 2025

In the discord there were a few comments:

  1. BevyDefault which is defined within bevy_image and only has a single implementer (TextureFormat) should be removed and any calls to TextureFormat::bevy_default() should be replaced with (now) explicitly bevy_image::TEXTURE_FORMAT_SDR.

  2. Perhaps this new constructor really wants to live in bevy_render, either as a free function or as an extension trait.

  3. Perhaps TEXTURE_FORMAT_SDR and TEXTURE_FORMAT_HDR should really live in bevy_render as associated constants on ViewTarget, but the other constructors on Image also want to use TEXTURE_FORMAT_SDR (as a "default" TextureFormat).

  4. This exact expression is used in many, many places throughout the bevy codebase, with the only change being the predicate (hdr), but will be addressed by Allow specifying intermediate textures for cameras. #13146

if (hdr) {
    ViewTarget::TEXTURE_FORMAT_HDR
} else {
    TextureFormat::bevy_default()
}

For now I have left the constructor in bevy_image, added a hdr: bool parameter, created the two constants, and have ViewTarget::TEXTURE_FORMAT_HDR point to bevy_image::TEXTURE_FORMAT_HDR.

@tigregalis
Copy link
Contributor Author

Can you also update headless_renderer, gpu_readback, and compute_shader_game_of_life

I think those are all the examples that use this pattern.

(Sorry, that wasn't an approve, just a comment)

I've done this but the discrepancies make me think maybe these should have their own helper constructors, possibly with guidance in the doc comment.

  • headless_renderer: additional texture usage COPY_SRC
  • gpu_readback: additional texture usage COPY_SRC + STORAGE_BINDING, different texture format R32Uint, different asset usage RENDER_WORLD
  • compute_shader_game_of_life: different texture usage COPY_DST + STORAGE_BINDING + TEXTURE_BINDING, different texture format R32Float, different asset usage RENDER_WORLD

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-Usability A targeted quality-of-life change that makes Bevy easier to use D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

Provide built-in API for generating "render target texture" and/or simplify "Render to Texture" example
5 participants