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

Update to WGPU 0.10 #2556

Conversation

zicklag
Copy link
Member

@zicklag zicklag commented Jul 27, 2021

Objective

Solution

  • Fix all of the errors caused by minor API changes between 0.9 and 0.10.

Known Issues

  • Currently when closing the window we get a panic: 'Texture[0] does not exist', /home/zicklag/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/wgpu-core-0.10.0/src/hub.rs:129:32.
    This is almost surely due to the drop order of fields related to the new SurfaceFrame that replaces SwapchainFrame in WGPU 0.10 and just needs to be investigated.

Note: The second commit in this PR is a workaround for a buffer size validation error and should probably not be merged. I don't know enough of what the problem is to fix it myself at the moment.

@@ -14,6 +14,7 @@ keywords = ["game", "engine", "gamedev", "graphics", "bevy"]
license = "MIT OR Apache-2.0"
readme = "README.md"
repository = "https://github.com/bevyengine/bevy"
resolver = "2"
Copy link
Member

Choose a reason for hiding this comment

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

This will do nothing for crates depending on Bevy and it doesn't seem needed here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought it was required for WGPU to build, but I'll have to double-check.

Copy link
Member

@mockersf mockersf Jul 27, 2021

Choose a reason for hiding this comment

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

The resolver v2 let's you specify features per target. While this PR doesn't need it, maybe it's needed internally by wgpu? That would be unfortunate...

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes WGPU needs it to compile now. I do remember it was a little difficult to make things work before without the new feature resolver, but it did work. If it is a problem for Bevy, we could discuss it with the WGPU folks, maybe. I don't know if it would be possible to go back to not needing it.

Copy link
Contributor

Choose a reason for hiding this comment

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

resolver = "2" will become the default in the 2021 edition.

Copy link
Member

@mockersf mockersf Jul 28, 2021

Choose a reason for hiding this comment

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

As a dependency can't specify to use the new resolver for the dependent crate, it's a little painful for now for libs to require it... This will change with edition 2021 in October 🎉

@@ -29,7 +29,7 @@ bevy_winit = { path = "../bevy_winit", optional = true, version = "0.5.0" }
bevy_utils = { path = "../bevy_utils", version = "0.5.0" }

# other
wgpu = "0.9"
wgpu = { version = "0.9", features = ["spirv"] }
Copy link
Member

Choose a reason for hiding this comment

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

Could you make that a git dependency to wgpu? While some jobs of CI will fail because of that, the others should work a little better

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated. For the glTF example to run we also need to patch Naga so I added a patch in there too. Since we can't actually merge until WGPU publishes another release, we can just take that and the git dependencies out when WGPU 0.10 comes out.

@zicklag zicklag force-pushed the update-to-wgpu-master branch from efa8a33 to 8a98d72 Compare July 28, 2021 00:44
@alice-i-cecile alice-i-cecile added A-Rendering Drawing game state to the screen C-Dependencies A change to the crates that Bevy depends on labels Aug 16, 2021
@alice-i-cecile
Copy link
Member

0.10 is released! Could you update this PR to that, then we can try and get it merged ASAP?

@cart
Copy link
Member

cart commented Aug 18, 2021

I think we might want to wait until Rust 2021 releases (which should be a matter of days). That way we don't need to worry as much about "weird feature resolver stuff".

@alice-i-cecile alice-i-cecile added this to the Bevy 0.6 milestone Aug 18, 2021
@cart
Copy link
Member

cart commented Aug 18, 2021

I think we might want to wait until Rust 2021 releases (which should be a matter of days).

This is wrong / some people on discord (and the wgpu matrix chat) corrected me. There is a good chance that it will land in rust 1.56, which is scheduled to release on October 21.

Thats far enough out that we might need to rethink our approach / investigate the implications of releasing without the resolver default changes.

@zicklag
Copy link
Member Author

zicklag commented Aug 19, 2021

Thats far enough out that we might need to rethink our approach / investigate the implications of releasing without the resolver default changes.

Yeah, so just to put in my experience here so far. In almost every case I've tried to compile WGPU or a depenant project without resolver = "2" in my Cargo.toml I would run into a compile error specifically saying that I needed resolver = "2" in my Cargo.toml, so it was a little inconvenient, but I was told exactly what to do by the compiler message and it was super easy to fix.

There was one time where for some reason I did not get that message, because I ran into a compiler issue sometime before the crate that had that specific error message or something like that and it just failed some some error in another crate, but I think I was doing some weird dev stuff at the time and may have had patched crates, etc, so that might not happen to anybody else in practice.


Also, as far as progress here, I've got a more up-to-date version of this branch locally, but I'm still trying to figure out how to get around the fact that swapchain textures are now a Texture and not a TextureView and I haven't figured out how to make Bevy internals happy with that yet.

@zicklag zicklag force-pushed the update-to-wgpu-master branch from 8a98d72 to e258981 Compare August 19, 2021 18:51
@zicklag
Copy link
Member Author

zicklag commented Aug 19, 2021

Finished updating to WGPU 0.10! The pipelined 2D renderer also works on OpenGL now, as long as you don't resize the screen, in which case the scale get's funky, but that's progress!

The old renderer sprite and text examples don't work anymore because of a shader error, but the 3d_scene example works on both Vulkan and OpenGL. The old renderer load_gltf example also doesn't work saying that an asset for glTF files wasn't found, but it doesn't that without this PR on the pipelined-rendering branch.

Do we want to fix the old renderer to work with WGPU 0.10, or is that not worth it?

@zicklag zicklag force-pushed the update-to-wgpu-master branch from e258981 to 11bbf8d Compare August 19, 2021 19:09
@zicklag zicklag changed the title Update to wgpu master Update to WGPU 0.10 Aug 19, 2021
@alice-i-cecile
Copy link
Member

Do we want to fix the old renderer to work with WGPU 0.10, or is that not worth it?

I don't think this is worth the effort; we're about to migrate off it and this is the last release with the old renderer.

@zicklag
Copy link
Member Author

zicklag commented Aug 19, 2021

OK, that's what I was thinking. Technically the old renderer is still compiling, and with WGPU 0.10, I'm not sure if we should leave it like that. For some reason CI fails on android and the examples fail to run, but those are both for the old renderer. Not sure exactly what our approach for this is, but let me know if there's anything else you want me to to do to this!

@cart
Copy link
Member

cart commented Aug 19, 2021

By the time we pick up these changes, the old renderer will almost certainly be gone :)

@billyb2
Copy link
Contributor

billyb2 commented Aug 20, 2021

Just out of curiosity, what's happening with this PR?

@zicklag
Copy link
Member Author

zicklag commented Aug 20, 2021

I'm using it to test Bevy with the latest WGPU changes so that I can work on getting OpenGL and WebGL2 support working.

When we are ready, probably after Bevy's new renderer is ready to replace the old one, we will probably use this PR to update to the latest WGPU.

@billyb2
Copy link
Contributor

billyb2 commented Aug 20, 2021

Alright, thank you!

@Neo-Zhixing
Copy link
Contributor

Regarding wgpu HEAD I have a few comments:

  1. When I tried running bevy's 3d_scene_pipelined example with wgpu HEAD I get the following error:
Sep 05 23:39:52.543 ERROR wgpu_core::device: Shader error: using sampled cube array images requires at least one of the capabilities [SampledCubeArray], but none are available
Sep 05 23:39:52.544 ERROR wgpu::backend::direct: wgpu error: Validation Error

This is due to a change in naga and it has been addressed in gfx-rs/wgpu#1911.

  1. There seems to be other changes in naga that caused compilation issues with wgsl. It seems that trailing f in floating point literals aren't supported anymore. If you run into a problem like that feel free to cherry pick Neo-Zhixing@998683f

@alice-i-cecile alice-i-cecile added the S-Blocked This cannot move forward until something else changes label Sep 22, 2021
@cart
Copy link
Member

cart commented Oct 7, 2021

In anticipation of the upcoming wgpu 0.11 release, I created a branch that updates pipelined-rendering to the latest wgpu master: https://github.com/cart/bevy/tree/wgpu-master.

@zicklag
Copy link
Member Author

zicklag commented Oct 7, 2021

Yay! 🎉

You want me to close this then? There's no real reason to use it as-is.

@cart
Copy link
Member

cart commented Oct 7, 2021

You want me to close this then? There's no real reason to use it as-is.

I'll leave that up to you. My branch builds on top of your work and does include you as a co-author on the commit. But I want to merge this on your terms (whatever that means).

I also just realized that the branch I linked to isn't the right one. Here is the actual branch: https://github.com/cart/bevy/tree/upgrade-rebase

@zicklag
Copy link
Member Author

zicklag commented Oct 7, 2021

Co-author on the commit is good enough for me. :) I'll close this and we can just worry about merging your new branch when the time comes.

@zicklag zicklag closed this Oct 7, 2021
bors bot pushed a commit that referenced this pull request Oct 8, 2021
Upgrades both the old and new renderer to wgpu 0.11 (and naga 0.7). This builds on @zicklag's work here #2556.

Co-authored-by: Carter Anderson <[email protected]>
bors bot pushed a commit that referenced this pull request Oct 8, 2021
Upgrades both the old and new renderer to wgpu 0.11 (and naga 0.7). This builds on @zicklag's work here #2556.

Co-authored-by: Carter Anderson <[email protected]>
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-Dependencies A change to the crates that Bevy depends on S-Blocked This cannot move forward until something else changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants