[Merged by Bors] - get proper texture format after the renderer is initialized, fix #3897 #5413

VitalyAnkh · 2022-07-21T08:17:50Z

Objective

There is no Srgb support on some GPU and display protocols with winit (for example, Nvidia's GPUs with Wayland). Thus TextureFormat::bevy_default() which returns Rgba8UnormSrgb or Bgra8UnormSrgb will cause panics on such platforms. This patch will resolve this problem. Fix #3897.

Solution

Make initialize_renderer expose wgpu::Adapter and first_available_texture_format, use the first_available_texture_format by default.

Changelog

Fixed OS-independent texture format setting in swapchain #3897.

superdump

We need to think about the consequences of adding and exposing the Adapter.

This is a good change that needs to happen.

I think further than this, probably bevy_default should be removed and replaced when the tonemapping PR lands. @cart

crates/bevy_render/src/lib.rs

crates/bevy_render/src/view/window.rs

VitalyAnkh · 2022-07-21T14:21:02Z

@superdump Thanks for your kind reply! Now I have removed the unused dbg!. The latest code now add AvailableTextureFormat resource to the World and use it for future use. I know the available TextureFormat should be computed per Surface, but it's still better than hard-coded values. And then we don't need to expose Adapter now but the code still does so now (if Bevy developers think it's useful, could I do it in another PR?). And I add a GpuImage field to UiPipeline to specify correct TextureFormat in impl<S: SpecializedRenderPipeline> SpecializedRenderPipelines<S>. The code still needs much tuning, but it could fix #3897 correctly now.

VitalyAnkh · 2022-07-21T14:35:10Z

Oops... It fails on x11 now. I will push more fixes.

VitalyAnkh · 2022-07-22T11:58:29Z

I think this PR could pass all checks now and with this, there won't be any "no available texture formats" error anymore. But the design still needs discussion. For example, should I expose Adapter in the World? And to make MeshPipe UiPipeline and SpritePipeline get the right TextureFormat, I add a GpuImage field to them, should I do this? Are there better ways to solve this problem?

bemyak · 2022-09-05T09:00:37Z

I've just run into the issue that this PR addresses. Any plans on getting it merged?

VitalyAnkh · 2022-09-05T09:47:28Z

@superdump @alice-i-cecile hi, what do you think about this PR? What should I do to get this merged?

crates/bevy_render/src/renderer/mod.rs

alice-i-cecile · 2022-09-05T15:22:29Z

crates/bevy_sprite/src/render/mod.rs

@@ -79,10 +87,57 @@ impl FromWorld for SpritePipeline {
            ],
            label: Some("sprite_material_layout"),
        });
+        let dummy_white_gpu_image = {


The code duplication of this section strikes me as a bit suspicious. Is there a good way to refactor this to avoid needing to remember to add this to each pipeline?

alice-i-cecile

First round of feedback left :) This is definitely a real issue, and the fix makes sense.

I'm not familiar enough with the details of the rendering machinery here to suggest exactly how you might fix that code duplication issue, but the current strategy makes me suspect that any custom implementations will run into the same problem.

superdump · 2022-09-06T03:29:19Z

I’ll give this another look over soon. I would like to have reviewed this and other things already but life is what it is. I appreciate your patience.

VitalyAnkh · 2022-09-30T06:14:58Z

@alice-i-cecile @devil-ira Hi! Following your kind reviews, I have updated this PR. All the problems you proposed should be resolved. Please let me know if there is anything that should be improved, thanks!

There is no Srgb support on some GPU and display protocols with `winit` (for example, Nvidia's GPUs with Wayland). Thus `TextureFormat::bevy_default()` which returns `Rgba8UnormSrgb` or `Bgra8UnormSrgb` will cause panics on such platforms. This patch will resolve this problem. Fix bevyengine#3897 ## Solution Make `initialize_renderer` expose `wgpu::Adapter` and `first_available_texture_format`, use the `first_available_texture_format` by default. ## Changelog * Fixed bevyengine#3897

VitalyAnkh · 2022-09-30T13:23:24Z

The latest force push makes prepare_view_targets use Res<RenderTextureFormat> other than Res<AvailableTextureFormats> to get proper texture format. It doesn't touch codes that have been reviewed.

tim-blackbird · 2022-10-07T16:42:52Z

I had some trouble with refresh rates with a multi-monitor setup, so I switched to wayland and then bumped into this issue myself.
I tried this PR, and while it does run, the colors aren't correct.

VitalyAnkh · 2022-10-07T19:02:51Z

@devil-ira This works as expected. The texture format in your image should be Bgra8Unorm other than Bgra8UnormSrgb, so it's definitely not correct. The real problem is winit (v0.26.0) on Nvidia GPU and Wayland can't get surface with Srgb colors, but this problem has been resolved in rust-windowing/glutin#1435. After bevy migrates to the new winit the color should be correct. Bevy should work on Nvidia and Wayland with the new winit, even without this PR, but I think it's reasonable to get texture format at runtime other than pre-defined.

VitalyAnkh · 2022-10-07T20:06:28Z

After bevy migrates to the new winit the color should be correct. Bevy should work on Nvidia and Wayland with the new winit, even without this PR

Correction: Bevy also needs wgpu v0.14.0(https://github.com/gfx-rs/wgpu/releases/tag/v0.14.0) to have the winit Srgb issues fixed.

alice-i-cecile

Not a complete fix, but a necessary one.

bors r+

#5413) # Objective There is no Srgb support on some GPU and display protocols with `winit` (for example, Nvidia's GPUs with Wayland). Thus `TextureFormat::bevy_default()` which returns `Rgba8UnormSrgb` or `Bgra8UnormSrgb` will cause panics on such platforms. This patch will resolve this problem. Fix #3897. ## Solution Make `initialize_renderer` expose `wgpu::Adapter` and `first_available_texture_format`, use the `first_available_texture_format` by default. ## Changelog * Fixed #3897.

bors · 2022-10-10T16:33:42Z

Pull request successfully merged into main.

Build succeeded:

…engine#3897 (bevyengine#5413) # Objective There is no Srgb support on some GPU and display protocols with `winit` (for example, Nvidia's GPUs with Wayland). Thus `TextureFormat::bevy_default()` which returns `Rgba8UnormSrgb` or `Bgra8UnormSrgb` will cause panics on such platforms. This patch will resolve this problem. Fix bevyengine#3897. ## Solution Make `initialize_renderer` expose `wgpu::Adapter` and `first_available_texture_format`, use the `first_available_texture_format` by default. ## Changelog * Fixed bevyengine#3897.

VitalyAnkh changed the title ~~get proper texture format after the renderer is initialized, fix #3897~~ [draft] get proper texture format after the renderer is initialized, fix #3897 Jul 21, 2022

VitalyAnkh marked this pull request as draft July 21, 2022 08:19

VitalyAnkh force-pushed the fix_texture_format_error branch from 44b9ac1 to 5b6acfa Compare July 21, 2022 08:33

superdump requested changes Jul 21, 2022

View reviewed changes

crates/bevy_render/src/lib.rs Outdated Show resolved Hide resolved

crates/bevy_render/src/view/window.rs Outdated Show resolved Hide resolved

VitalyAnkh force-pushed the fix_texture_format_error branch 2 times, most recently from b9a7032 to 06166d3 Compare July 21, 2022 14:17

alice-i-cecile added C-Bug An unexpected or incorrect behavior A-Rendering Drawing game state to the screen labels Jul 21, 2022

VitalyAnkh force-pushed the fix_texture_format_error branch from 06166d3 to 9088038 Compare July 22, 2022 11:40

VitalyAnkh changed the title ~~[draft] get proper texture format after the renderer is initialized, fix #3897~~ get proper texture format after the renderer is initialized, fix #3897 Jul 22, 2022

VitalyAnkh marked this pull request as ready for review July 22, 2022 11:58

VitalyAnkh force-pushed the fix_texture_format_error branch 3 times, most recently from d0709dc to 728e885 Compare July 29, 2022 08:09

VitalyAnkh force-pushed the fix_texture_format_error branch from 728e885 to d0a17ea Compare August 16, 2022 11:06

VitalyAnkh requested a review from superdump August 16, 2022 11:25

VitalyAnkh force-pushed the fix_texture_format_error branch from d0a17ea to 19af01d Compare August 20, 2022 06:52

VitalyAnkh force-pushed the fix_texture_format_error branch from 19af01d to faea439 Compare September 5, 2022 06:57

alice-i-cecile reviewed Sep 5, 2022

View reviewed changes

crates/bevy_render/src/renderer/mod.rs Show resolved Hide resolved

alice-i-cecile reviewed Sep 5, 2022

View reviewed changes

crates/bevy_render/src/renderer/mod.rs Outdated Show resolved Hide resolved

alice-i-cecile reviewed Sep 5, 2022

View reviewed changes

crates/bevy_render/src/renderer/mod.rs Show resolved Hide resolved

alice-i-cecile reviewed Sep 5, 2022

View reviewed changes

alice-i-cecile requested changes Sep 5, 2022

View reviewed changes

VitalyAnkh force-pushed the fix_texture_format_error branch from faea439 to 36f6dfb Compare September 9, 2022 11:24

VitalyAnkh requested review from alice-i-cecile and tim-blackbird and removed request for superdump and alice-i-cecile September 30, 2022 06:29

tim-blackbird approved these changes Sep 30, 2022

View reviewed changes

alice-i-cecile approved these changes Sep 30, 2022

View reviewed changes

alice-i-cecile requested a review from superdump September 30, 2022 11:41

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 Sep 30, 2022

VitalyAnkh force-pushed the fix_texture_format_error branch from e145114 to f32e152 Compare September 30, 2022 13:19

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 Oct 7, 2022

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 Oct 7, 2022

alice-i-cecile added this to the Bevy 0.9 milestone Oct 7, 2022

VitalyAnkh added a commit to VitalyAnkh/bevy that referenced this pull request Oct 7, 2022

try new winit, proof of concept for bevyengine#5413 (comment)

43393c2

alice-i-cecile approved these changes Oct 10, 2022

View reviewed changes

bors bot changed the title ~~get proper texture format after the renderer is initialized, fix #3897~~ [Merged by Bors] - get proper texture format after the renderer is initialized, fix #3897 Oct 10, 2022

bors bot closed this Oct 10, 2022

mockersf mentioned this pull request Oct 11, 2022

index out of bounds when running any Bevy example #6231

Closed

cart mentioned this pull request Oct 24, 2022

[Merged by Bors] - separate tonemapping and upscaling passes #3425

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - get proper texture format after the renderer is initialized, fix #3897 #5413

[Merged by Bors] - get proper texture format after the renderer is initialized, fix #3897 #5413

VitalyAnkh commented Jul 21, 2022 •

edited

Loading

superdump left a comment

VitalyAnkh commented Jul 21, 2022

VitalyAnkh commented Jul 21, 2022

VitalyAnkh commented Jul 22, 2022

bemyak commented Sep 5, 2022

VitalyAnkh commented Sep 5, 2022

alice-i-cecile Sep 5, 2022

alice-i-cecile left a comment

superdump commented Sep 6, 2022

VitalyAnkh commented Sep 30, 2022

VitalyAnkh commented Sep 30, 2022

tim-blackbird commented Oct 7, 2022

VitalyAnkh commented Oct 7, 2022 •

edited

Loading

VitalyAnkh commented Oct 7, 2022 •

edited

Loading

alice-i-cecile left a comment

bors bot commented Oct 10, 2022

[Merged by Bors] - get proper texture format after the renderer is initialized, fix #3897 #5413

[Merged by Bors] - get proper texture format after the renderer is initialized, fix #3897 #5413

Conversation

VitalyAnkh commented Jul 21, 2022 • edited Loading

Objective

Solution

Changelog

superdump left a comment

Choose a reason for hiding this comment

VitalyAnkh commented Jul 21, 2022

VitalyAnkh commented Jul 21, 2022

VitalyAnkh commented Jul 22, 2022

bemyak commented Sep 5, 2022

VitalyAnkh commented Sep 5, 2022

alice-i-cecile Sep 5, 2022

Choose a reason for hiding this comment

alice-i-cecile left a comment

Choose a reason for hiding this comment

superdump commented Sep 6, 2022

VitalyAnkh commented Sep 30, 2022

VitalyAnkh commented Sep 30, 2022

tim-blackbird commented Oct 7, 2022

VitalyAnkh commented Oct 7, 2022 • edited Loading

VitalyAnkh commented Oct 7, 2022 • edited Loading

alice-i-cecile left a comment

Choose a reason for hiding this comment

bors bot commented Oct 10, 2022

VitalyAnkh commented Jul 21, 2022 •

edited

Loading

VitalyAnkh commented Oct 7, 2022 •

edited

Loading

VitalyAnkh commented Oct 7, 2022 •

edited

Loading