Add multiple render targets example (v2) #5313

kaphula · 2024-02-27T22:42:07Z

Connections
This PR depends on #5312

This is a new version of #5297 which had some issues.

Description
This PR adds a new example that demonstrates how to use multiple render targets when you want to render to two or more targets from the fragment shader.

As far as I know, none of the existing examples demonstrate this, and furthermore, finding information about how to do this with wgpu seems to be way too challenging, or at least it was for me. I had to ask help from multiple people who knew how to do this, and before that I was not even sure if this was possible at all.

Testing
I am not sure if this should or can be tested, or if the core feature has already been tested. Suggestions or instructions are welcome on how to implement the required tests.

Checklist

Run cargo fmt.
Run cargo clippy. If applicable, add:
- --target wasm32-unknown-unknown
- --target wasm32-unknown-emscripten
Run cargo xtask test to run tests.
Add change to CHANGELOG.md. See simple instructions inside file.

cwfitzgerald

Looking good, some small comments, then probably one more small scale review, and this can land!

cwfitzgerald · 2024-03-14T00:01:22Z

examples/src/multiple_render_targets/mod.rs

+use glam::Vec2;
+use std::borrow::Cow;
+use wgpu::{
+    Adapter, BindGroup, BindGroupLayout, ColorTargetState, CommandEncoder, Device, Extent3d, Queue,
+    RenderPassColorAttachment, RenderPipeline, Sampler, ShaderModule, SurfaceConfiguration,
+    Texture, TextureAspect, TextureDimension, TextureFormat, TextureUsages, TextureView,
+    TextureViewDescriptor, TextureViewDimension,
+};
+use winit::event::WindowEvent;


Going to be very annoying, and say we probably shouldn't import these names and always qualify with wgpu or the library they're from. It helps readability a ton when looking at a small section of code.

cwfitzgerald · 2024-03-14T00:01:50Z

examples/src/multiple_render_targets/mod.rs

+        fn create_ball_texture_data(width: usize, height: usize) -> Vec<u8> {
+            // Creates black and white pixel data for the texture to sample.
+            let mut img_data = Vec::with_capacity(width * height);
+            let center: Vec2 = Vec2::new(width as f32 * 0.5, height as f32 * 0.5);
+            let half_distance = width as f32 * 0.5;
+            for y in 0..width {
+                for x in 0..height {
+                    let cur_pos = Vec2::new(x as f32, y as f32);
+                    let distance_to_center_normalized =
+                        1.0 - (cur_pos - center).length() / half_distance;
+                    let val: u8 = (u8::MAX as f32 * distance_to_center_normalized) as u8;
+                    img_data.push(val)
+                }
+            }
+            img_data
+        }


Let's make this a normal free function, not a nested one.

cwfitzgerald · 2024-03-14T01:18:39Z

examples/src/multiple_render_targets/mod.rs

+    }
+}
+
+fn create_target_textures(


Lets make this a TextureTarget::new and move it next to the definition of TextureTargets

cwfitzgerald · 2024-03-14T01:19:12Z

examples/src/multiple_render_targets/mod.rs

+            multiview: None,
+        });
+
+        let (ba, bb) =


Suggested change

let (ba, bb) =

let (bg_a, bg_b) =

Or better yet bg_left bg_right.

kaphula · 2024-03-14T06:47:10Z

@cwfitzgerald Your requested changes are now implemented. Let me know if there is anything else.

kaphula · 2024-03-15T05:29:36Z

I added another commit to make all the imports direct as I realized that is probably what you meant and not just the wgpu imports.

cwfitzgerald

A couple issues with the screenshot and such. Additionally in general the code needs a lot more commentary about what's going on and why (adding things like method documentation would go a long way).

Additionally there seems the tests are failing inside the GL backend, we can mark it as failing to not block merging this, but should be filed as a follow up issue.

cwfitzgerald · 2024-04-18T21:07:51Z

examples/src/multiple_render_targets/README.md

+
+## Screenshots
+
+![Multi render target](./screenshot.png)


Did you run the tests locally with cargo xtask test? The screenshot was never generated

The screenshot gets generated just fine for me, although bunch of tests fail on my machine overall. Do you want the generated screenshot to be part of the repository?

Yeah - the fact that the screenshot is generated is how we ensure the screenshot stays up to date with the example

cwfitzgerald · 2024-04-18T21:08:49Z

examples/src/multiple_render_targets/mod.rs

+#[wgpu_test::gpu_test]
+static TEST: crate::framework::ExampleTestParams = crate::framework::ExampleTestParams {
+    name: EXAMPLE_NAME,
+    // Generated on 1080ti on Vk/Windows


These seem to just be blindly copied from other examples

Is that a problem? Can you give me some guidance on how the test should be configured for this specific example?

As for the comments, I am not sure what should be commented better. The readme of the example describes what the example does and how it works. There are also various comments in the code. The render function has inline comments describing what the draw calls of the example do. Should these comments be on the methods instead?

Is the issue with GL failing something related to the internals of wgpu and not this example? If not, is there something I can do about it?

Is that a problem? Can you give me some guidance on how the test should be configured for this specific example?

Basically the idea is that you'll start with the lowest possible tolerance, then increase it as you find issues on specific gpu/api combinations. In your case, this should be listed as whatever card you ran it on to generate the tolerance. This way we have a record of what device is keeping the limit high, so we can lower it if needed.

As for the comments, I am not sure what should be commented better. The readme of the example describes what the example does and how it works. There are also various comments in the code. The render function has inline comments describing what the draw calls of the example do. Should these comments be on the methods instead?

Let me give this another, better look

…update_test

cwfitzgerald · 2024-12-17T20:46:56Z

I don't think I did a very good job of reviewing this the first time around and giving actionable advice, so I'm going to give this another go. It also completely fell through the cracks of reviewing - sorry for all the trouble, we try to be more on top of things than this.

cwfitzgerald

Alright, I have a few small comments, but I think this is largely ready to go! I've pushed a commit which fixes compilation issues that have happened in the interim.

Thanks for the patience :)

examples/src/multiple_render_targets/mod.rs

cwfitzgerald

Very nice!

kaphula · 2024-12-18T01:25:06Z

Hey, no worries. Better prioritize the library itself over examples first.

I applied your suggested changes. Let me know what you think. Should the changelog addition be moved to next release or do we leave it under ## v0.19.4 (2024-04-17)?

cwfitzgerald · 2024-12-18T04:04:07Z

I applied your suggested changes. Let me know what you think.

Nice!

Should the changelog addition be moved to next release or do we leave it under ## v0.19.4 (2024-04-17)?

The next release

…example' into multiple_render_targets_example

cwfitzgerald · 2024-12-18T04:27:43Z

Amazing, lets land this!

kaphula and others added 6 commits February 27, 2024 21:26

Add the initial files of multiple render targets exmaple.

ac4a804

Fix Present

16970ca

Merge branch 'fix-presentation' into multiple_render_targets_example

a0382d7

Add requested PR review changes.

205e22a

Fix cargo clippy warnings.

ceb5e4a

Remove screenshot.

7e994b0

kaphula requested a review from a team as a code owner February 27, 2024 22:42

kaphula added 3 commits February 28, 2024 00:44

Update CHANGELOG.md

d29fa6e

Add empty line to shader.

74328eb

Fix resize crash by storing screen dimensions on resize.

df5aa53

cwfitzgerald requested changes Mar 14, 2024

View reviewed changes

kaphula added 3 commits March 14, 2024 08:30

Make imports to be direct.

4568912

Add cosmetic code style changes.

edc97c0

Rename a/b to left/right.

2d08936

cwfitzgerald self-requested a review March 14, 2024 17:00

Make all imports direct.

359b01b

Merge branch 'trunk' into multiple_render_targets_example

9c3cf48

cwfitzgerald requested changes Apr 18, 2024

View reviewed changes

kaphula added 2 commits May 27, 2024 20:38

Merge branch 'refs/heads/trunk' into multiple_render_targets_example_…

a56355a

…update_test

Update to wgpu 0.20.0

1ba3592

cwfitzgerald self-assigned this Dec 11, 2024

cwfitzgerald added 2 commits December 17, 2024 15:47

Merge branch 'trunk' into multiple_render_targets_example

63547f0

Compile Errors + Screenshots

fbc49b0

cwfitzgerald requested changes Dec 17, 2024

View reviewed changes

examples/src/multiple_render_targets/mod.rs Outdated Show resolved Hide resolved

examples/src/multiple_render_targets/mod.rs Outdated Show resolved Hide resolved

cwfitzgerald and others added 3 commits December 17, 2024 17:14

Adjust Bound

c9f3fe9

Add more consistent names for color targets.

7cd60a5

Simplify fragment state target initialization.

e9bf491

cwfitzgerald approved these changes Dec 18, 2024

View reviewed changes

cwfitzgerald enabled auto-merge (squash) December 18, 2024 01:24

cwfitzgerald disabled auto-merge December 18, 2024 01:40

cwfitzgerald added 2 commits December 17, 2024 22:53

Fix CI

cb7d2e2

Fix CI Better

5839ac9

kaphula added 2 commits December 18, 2024 06:13

Fix changelog for next release.

1f681d1

Merge remote-tracking branch 'origin_kaphula/multiple_render_targets_…

023e56f

…example' into multiple_render_targets_example

cwfitzgerald enabled auto-merge (squash) December 18, 2024 04:27

cwfitzgerald merged commit 0fe2034 into gfx-rs:trunk Dec 18, 2024
27 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add multiple render targets example (v2) #5313

Add multiple render targets example (v2) #5313

kaphula commented Feb 27, 2024 •

edited

Loading

cwfitzgerald left a comment

cwfitzgerald Mar 14, 2024

cwfitzgerald Mar 14, 2024

cwfitzgerald Mar 14, 2024

cwfitzgerald Mar 14, 2024

kaphula commented Mar 14, 2024

kaphula commented Mar 15, 2024

cwfitzgerald left a comment

cwfitzgerald Apr 18, 2024

kaphula Apr 19, 2024

cwfitzgerald Dec 17, 2024

cwfitzgerald Apr 18, 2024

kaphula Apr 19, 2024 •

edited

Loading

kaphula Apr 19, 2024

cwfitzgerald Dec 17, 2024

cwfitzgerald Dec 17, 2024

cwfitzgerald commented Dec 17, 2024

cwfitzgerald left a comment •

edited

Loading

cwfitzgerald left a comment

kaphula commented Dec 18, 2024

cwfitzgerald commented Dec 18, 2024

cwfitzgerald commented Dec 18, 2024

Add multiple render targets example (v2) #5313

Add multiple render targets example (v2) #5313

Conversation

kaphula commented Feb 27, 2024 • edited Loading

cwfitzgerald left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaphula commented Mar 14, 2024

kaphula commented Mar 15, 2024

cwfitzgerald left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaphula Apr 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwfitzgerald commented Dec 17, 2024

cwfitzgerald left a comment • edited Loading

Choose a reason for hiding this comment

cwfitzgerald left a comment

Choose a reason for hiding this comment

kaphula commented Dec 18, 2024

cwfitzgerald commented Dec 18, 2024

cwfitzgerald commented Dec 18, 2024

kaphula commented Feb 27, 2024 •

edited

Loading

kaphula Apr 19, 2024 •

edited

Loading

cwfitzgerald left a comment •

edited

Loading