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.

Sign up for GitHub

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

Jump to bottom

Fix UI corruption for AMD gpus with Vulkan #9169

Merged
merged 2 commits into from Jul 19, 2023
Merged

Fix UI corruption for AMD gpus with Vulkan #9169

merged 2 commits into from Jul 19, 2023

Conversation

ickshonpe
Copy link
Contributor

@ickshonpe ickshonpe commented Jul 15, 2023 •
edited

Objective

Fixes #8894
Fixes #7944

Solution

The UI pipeline's MultisampleState::count is set to 1 whereas the MultisampleState::count for the camera's ViewTarget is taken from the Msaa resource, and corruption occurs when these two values are different.

This PR solves the problem by setting MultisampleState::count for the UI pipeline to the value from the Msaa resource too.

I don't know much about Bevy's rendering internals or graphics hardware, so maybe there is a better solution than this. UI MSAA was probably disabled for a good reason (performance?).

Changelog

  • Enabled multisampling for the UI pipeline.

@ickshonpe
Some AMD gpus using the Vulkan wgpu backend see corrupted UI elements…
73fb67f 
…. I know very little about Bevy's rendering internals but this seems to be caused by MSAA. Multisampling is disabled for the UI pipeline but is enabled for the camera's ViewTarget, and this seems to cause some conflict.

This commit fixes the corruption issues by enabling MSAA for the UiPipeline.
@JMS55
Copy link
Contributor

JMS55 commented Jul 16, 2023

Feels like an issue with either wgpu or drivers...

@nicopap
Copy link
Contributor

nicopap commented Jul 16, 2023

Is this related:

@Selene-Amanita Selene-Amanita added C-Bug An unexpected or incorrect behavior A-Rendering Drawing game state to the screen A-UI Graphical user interfaces, styles, layouts, and widgets labels Jul 17, 2023
@Selene-Amanita Selene-Amanita added this to the 0.11.1 milestone Jul 17, 2023
@ickshonpe
Copy link
Contributor Author

ickshonpe commented Jul 17, 2023 •
edited

Feels like an issue with either wgpu or drivers...

Seems like it. The corruption has only been observed on a subset of AMD cards using the Vulkan backend and official AMD drivers (on both Windows and Linux, OS doesn't seem to make a difference):

  • RX 6750 XT
  • RX 6700 XT
  • RX 5600 XT

Didn't reproduce on:

  • Radeon RX 580
  • Radeon RX 6800 XT

@ickshonpe
Copy link
Contributor Author

ickshonpe commented Jul 17, 2023 •
edited

Is this related:

#5721 doesn't look like it's directly related to the corruption bugs, but this PR should fix that issue too.

@nicopap
Copy link
Contributor

nicopap commented Jul 17, 2023

#5752 is already fixed. IIRC, it was fixed by disabling MSAA on UI. Let me check this PR doesn't introduce a regression.

@nicopap
Copy link
Contributor

nicopap commented Jul 17, 2023

Tested with the Minimal reproducible example from #7012 and I don't see a regression, nice!

@JMS55 JMS55 mentioned this pull request Jul 18, 2023
Open
@nicopap nicopap 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 Jul 18, 2023
@nicopap
Copy link
Contributor

nicopap commented Jul 18, 2023

Note that github doesn't recognize that this PR fixes #7944. You should reword the PR description to "Fixes #8894 and fixes #7944" Also again note that #5721 is already fixed. I was just worried about regressions (but found none)

@ickshonpe
Copy link
Contributor Author

ickshonpe commented Jul 18, 2023 •
edited

Note that github doesn't recognize that this PR fixes #7944. You should reword the PR description to "Fixes #8894 and fixes #7944" Also again note that #5721 is already fixed. I was just worried about regressions (but found none)

Ah, got it.

@mockersf mockersf added this pull request to the merge queue Jul 19, 2023
Merged via the queue into bevyengine:main with commit 6f8089d Jul 19, 2023
20 checks passed
@Elabajaba
Copy link
Contributor

Elabajaba commented Jul 21, 2023 •
edited

This seems to have broken bloom.

edit: It also broke tonemapping.

@nicopap nicopap removed this from the 0.11.1 milestone Jul 21, 2023
@Elabajaba Elabajaba mentioned this pull request Jul 21, 2023
Closed
Elabajaba added a commit to Elabajaba/bevy that referenced this pull request Jul 22, 2023
@Elabajaba
Revert "Fix UI corruption for AMD gpus with Vulkan (bevyengine#9169)"
d54fed3
@Elabajaba Elabajaba mentioned this pull request Jul 22, 2023
Merged
github-merge-queue bot pushed a commit that referenced this pull request Jul 25, 2023
@Elabajaba
Revert "Fix UI corruption for AMD gpus with Vulkan (#9169)" (#9237)
774fb56 
# Objective

Fixes #9234

re-breaks: The issues that were linked in #9169 

## Solution

Revert the PR that broke tonemapping/postprocessing/etc.

Any passes that are post msaa resolve need to use the main textures, not
the msaa texture.

## Changelog

Idk what to put here since it's a revert.
This was referenced Jul 25, 2023
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mockersf mockersf mockersf approved these changes

@nicopap nicopap nicopap approved these changes

Assignees
No one assigned
Labels
A-Rendering Drawing game state to the screen A-UI Graphical user interfaces, styles, layouts, and widgets C-Bug An unexpected or incorrect behavior 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Text Glitching [Rendering Issue] Corruption on some UI elements
6 participants
@ickshonpe @JMS55 @nicopap @Elabajaba @mockersf @Selene-Amanita