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

Updated to wgpu 0.16.0, wgpu-hal 0.16.0 and naga 0.12.0 #8446

Merged
merged 9 commits into from Apr 26, 2023

Conversation

airingursb
Copy link
Contributor

Objective

  • Updated to wgpu 0.16.0 and wgpu-hal 0.16.0

Changelog

  1. Upgrade wgpu to 0.16.0 and wgpu-hal to 0.16.0
  2. Fix the error in native when using a filterable TextureSampleType::Float on a multisample BindingType::Texture. (reject binding type multisampled when on a float filterable sample type gfx-rs/wgpu#3686)

Rollback TextureSampleType::Float filterable setter

Fix Error when using a filterable  on a multisample
@github-actions
Copy link
Contributor

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

@mockersf
Copy link
Member

mockersf commented Apr 20, 2023

Could you also update naga to 0.12?

You may have to change a few other things for examples to work, don't hesitate to run them all on your computer (bash ./tools/example_showcase.sh). Example shader_prepass is failing for me without additional changes.

@airingursb
Copy link
Contributor Author

airingursb commented Apr 20, 2023

Could you also update naga to 0.12?

You may have to change a few other things for examples to work, don't hesitate to run them all on your computer (bash ./tools/example_showcase.sh). Example shader_prepass is failing for me without additional changes.

Okay, I will update naga and then run the examples to verify.

Emmm, but shader_prepass work successful for me without additional changes.

20230420164659@2x

SystemInfo { os: "MacOS 13.3 ", kernel: "22.4.0", cpu: "Intel(R) Core(TM) i7-10700K CPU @ 3.80GHz", core_count: "8", memory: "96.0 GiB" }

Copy link
Contributor

@RobWalt RobWalt left a comment

Choose a reason for hiding this comment

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

Hey cool! I was working on the same changes. I don't have any knowledge at all in the specifics of that domain and I was struggling at a few places. Can you maybe clarify a few changes?

Feel free to ignore my comments if they get in the way.

crates/bevy_render/src/texture/fallback_image.rs Outdated Show resolved Hide resolved
crates/bevy_render/src/texture/image.rs Outdated Show resolved Hide resolved
crates/bevy_render/src/texture/ktx2.rs Outdated Show resolved Hide resolved
@Azorlogh
Copy link
Contributor

Just a heads up for the changelog, this update also fixes this issue 5732 🙂

@airingursb airingursb changed the title Updated to wgpu 0.16.0 and wgpu-hal 0.16.0 Updated to wgpu 0.16.0, wgpu-hal 0.16.0, naga 0.12.0 Apr 20, 2023
@airingursb airingursb changed the title Updated to wgpu 0.16.0, wgpu-hal 0.16.0, naga 0.12.0 Updated to wgpu 0.16.0, wgpu-hal 0.16.0 and naga 0.12.0 Apr 20, 2023
@BD103
Copy link
Member

BD103 commented Apr 20, 2023

Be warned that 0.16.0 is a breaking change, so the release notes will need to include some sort of migration guide for changing keywords like type to alias etc. I know that Bevy isn't against breaking changes, but it still should be mentioned.

@JMS55 JMS55 added A-Rendering Drawing game state to the screen C-Dependencies A change to the crates that Bevy depends on labels Apr 20, 2023
Copy link
Contributor

@irate-devil irate-devil left a comment

Choose a reason for hiding this comment

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

LGTM. Changes match what's in the wgpu changelog.

@james7132 james7132 added the C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide label Apr 21, 2023
@github-actions
Copy link
Contributor

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Could you add some context on what users should update when this change get released in a new version of Bevy?
It will be used to help writing the migration guide for the version. Putting it after a ## Migration Guide will help it get automatically picked up by our tooling.

@mockersf
Copy link
Member

There are some code to fix behind features, and some formatting to update.

@RobWalt
Copy link
Contributor

RobWalt commented Apr 22, 2023

FYI, just a small warning:

We found some crashes with the update to wgpu 0.16. It only happens when compiling to wasm, running the app on MacOS and changing the perspective of the camera. It crashes not only the app, but the browser. We tested this on several browsers and it happened everywhere everytime. The App was running normally when compiled for native use. It's most likely some issue with MacOS + the new version of wgpu and isn't caused by bevy directly.

We are still investigating it and are working on a minimal repro example to test if it might be related to that specific machine or if it's really related to MacOS in general.

Update: Apparently it also happens with wgpu 0.15, so this PR here shouldn't be an issue 😅 I'll link to an issue just for documentation purposes once we've filed one.

Update 2: There is an issue for this now, it's #8481

@alice-i-cecile alice-i-cecile added this to the 0.11 milestone Apr 22, 2023
@superdump
Copy link
Contributor

Please run with --features zstd,ktx2,dds to catch a couple more errors. And then it needs to be run through cargo fmt. Other than that, if it's working then it looks good to me.

@airingursb
Copy link
Contributor Author

Please run with --features zstd,ktx2,dds to catch a couple more errors. And then it needs to be run through cargo fmt. Other than that, if it's working then it looks good to me.

@superdump I have revised your question over these commits. Please see if there are any other items that need to be modified.

@mockersf
Copy link
Member

Could you also compile with feature basis-universal? CI is failing on another decribe()

@airingursb
Copy link
Contributor Author

Could you also compile with feature basis-universal? CI is failing on another decribe()

@mockersf Thanks to suggestion, the related CI problem has been fixed. I need your help to take a look at the version of the Cargo.lock file. The check-bans CI failed.

@mockersf
Copy link
Member

I've looked at the check-bans failures, they can be ignored 👍

@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 Apr 26, 2023
@mockersf mockersf added this pull request to the merge queue Apr 26, 2023
Merged via the queue into bevyengine:main with commit 4d54ce1 Apr 26, 2023
24 of 25 checks passed
DJMcNab added a commit to DJMcNab/vello that referenced this pull request Apr 26, 2023
DJMcNab added a commit to linebender/vello that referenced this pull request Apr 27, 2023
* Update the bevy example to wgpu 0.16

After bevyengine/bevy#8446

* Remove inaccurate README warning
alice-i-cecile pushed a commit that referenced this pull request May 29, 2023
# Objective


Since #8446, example `shader_prepass` logs the following error on my mac
m1:
```
ERROR bevy_render::render_resource::pipeline_cache: failed to process shader:
error: Entry point fragment at Fragment is invalid
 = Argument 1 varying error
 = Capability MULTISAMPLED_SHADING is not supported
```
The example display the 3d scene but doesn't change with the preps
selected

Maybe related to this update in naga:
gfx-rs/naga@cc3a8ac

## Solution

- Disable MSAA in the example, and check if it's enabled in the shader
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-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide C-Dependencies A change to the crates that Bevy depends on 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

10 participants