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

Backport device limits fix to 0.17 #4964

Closed
wants to merge 2 commits into from

Conversation

cshenton-work
Copy link

@cshenton-work cshenton-work commented Jan 3, 2024

Description
This commit in 0.18 and later fixed an issue where limits passed to request_device would be dropped due to an upstream issue with wasm_bindgen and web_sys.

This PR backports that fix to 0.17 so that dependent packages can enjoy correct device initialisation behaviour on the web.

Testing

This has been tested alongside a modified copy of bevy 0.12.0 (updated to depend on wgpu 0.17.2), to confirm that the device limits are now respected. For example device request properly fails on too large a maxBufferSize and no longer errors when buffers less than the requested maxBufferSize, but greater than the default max size, are created.

Checklist

  • Run cargo fmt.
  • Run cargo clippy. If applicable, add:
    • --target wasm32-unknown-unknown
    • [Upstream Failure in noise] --target wasm32-unknown-emscripten
  • [unrecognized subcommand "test"] Run cargo xtask test to run tests.
  • Add change to CHANGELOG.md. See simple instructions inside file.

Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

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

Looks good after nit.

Please re-request my review after it's addressed!

@@ -50,8 +50,8 @@ path = "./wgpu-hal"
version = "0.17"

[workspace.dependencies.naga]
git = "https://github.com/gfx-rs/naga"
rev = "bac2d82a430fbfcf100ee22b7c3bc12f3d593079"
# git = "https://github.com/gfx-rs/naga"
Copy link
Member

Choose a reason for hiding this comment

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

This got accidentally committed

Copy link
Author

Choose a reason for hiding this comment

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

This was actually not an accident. Without removing this, there are naga version conflicts when building against other packages. Is there a reason this particular commit is targeted rather than the version on crates.io?

Copy link
Member

Choose a reason for hiding this comment

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

The commit here is just the commit for the release of 0.17 - the sources are the same. When locally patching, you should be able to patch both wgpu and naga to the same place and it work fine. Something like:

[patch.crates-io]
wgpu = { git = "https://github.com/gfx-rs/wgpu.git", rev = "7b7dfa6" }
wgpu-core = { git = "https://github.com/gfx-rs/wgpu.git", rev = "7b7dfa6" }
wgpu-hal = { git = "https://github.com/gfx-rs/wgpu.git", rev = "7b7dfa6" }
wgpu-types = { git = "https://github.com/gfx-rs/wgpu.git", rev = "7b7dfa6" }
naga = { git = "https://github.com/gfx-rs/naga.git", rev = "bac2d82a430fbfcf100ee22b7c3bc12f3d593079" }

@Wumpf
Copy link
Member

Wumpf commented Mar 24, 2024

@cshenton-work do you still need this or are you on a newer version by now?

@cwfitzgerald
Copy link
Member

As this PR has gone stale, I'm going to close it - please refile it if you have hte need for this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants