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

Fix WASM build #312

Merged
merged 13 commits into from
Apr 25, 2023
Merged

Fix WASM build #312

merged 13 commits into from
Apr 25, 2023

Conversation

armansito
Copy link
Collaborator

  • Rolled wgpu to 0.16.
  • Incorprated the instant crate in lieu of std::time::Instant which works on WASM and native builds.
  • Fixed the issue with window scaling by setting the canvas size based on winit Window dimensions.

This resolves #276

crates.io/crates/instant provides a std::time::Instant implementation
that works on both WASM and non-wasm builds.
This prevents the scaling caused by the hardcoded canvas dimensions on
high dpi platforms.
@armansito armansito requested review from raphlinus and DJMcNab April 23, 2023 19:06
Copy link
Member

@DJMcNab DJMcNab left a comment

Choose a reason for hiding this comment

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

Code seems reasonable to me, although I can't test it without a WebGPU compatible browser.

@@ -16,6 +16,7 @@ vello_svg = { path = "../../integrations/vello_svg" }
anyhow = { workspace = true }
clap = { workspace = true, features = ["derive"] }
image = "0.24.5"
instant = "0.1.12"
Copy link
Member

Choose a reason for hiding this comment

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

According to instant's documentation, one of the stdweb or wasm-bindgen features need to be updated.

I still don't have a working browser with WebGPU, so maybe this works? Perhaps instant's docs need updating

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch. I went ahead and enabled wasm-bindgen though AFAICT all the animated scenes still worked without it.

src/engine.rs Outdated
bytes_per_row: NonZeroU32::new(
image_proxy.width * format.describe().block_size as u32,
bytes_per_row: Some(
image_proxy.width * format.block_size(None).unwrap(),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
image_proxy.width * format.block_size(None).unwrap(),
image_proxy.width * format.block_size(None).expect("All supported ImageFormats have a valid block size"),

or words to that effect

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

The potential division by zero in this line led to visible visual
artifacts when running against WebGPU in Chrome.
@armansito
Copy link
Collaborator Author

Unfortunately this PR breaks the bevy example since bevy_render currently depends on wgpu 0.15.

Copy link
Member

@DJMcNab DJMcNab left a comment

Choose a reason for hiding this comment

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

All the changes here are good, there are just a few additions I think should be included in this PR.

I think this can probably incorporate the changes from #301. In particular, that's an update of cargo-run-wasm, including the new support for the -p shorthand; adding the WASM target to the instructions is optional but would be nice.

Also, I'd like us to add a Warning to the Bevy section of the README that the example temporarily doesn't compile. (For context, we decided on Zulip that the Bevy example shouldn't block needed changes, so long as there's a reasonable expectation it won't be broken for long)

Warning
This example currently does not compile. We expect to resolve this as soon as bevy's has updated to wgpu 0.16

Additionally, we specify the wgpu version in a badge in README.md. This also needs to be updated. There should probably also be a comment to this effect in the root Cargo.toml.

@armansito
Copy link
Collaborator Author

armansito commented Apr 24, 2023

All the changes here are good, there are just a few additions I think should be included in this PR.

I think this can probably incorporate the changes from #301. In particular, that's an update of cargo-run-wasm, including the new support for the -p shorthand; adding the WASM target to the instructions is optional but would be nice.

Also, I'd like us to add a Warning to the Bevy section of the README that the example temporarily doesn't compile. (For context, we decided on Zulip that the Bevy example shouldn't block needed changes, so long as there's a reasonable expectation it won't be broken for long)

Warning
This example currently does not compile. We expect to resolve this as soon as bevy's has updated to wgpu 0.16

Additionally, we specify the wgpu version in a badge in README.md. This also needs to be updated. There should probably also be a comment to this effect in the root Cargo.toml.

Done. I cherry-picked your change from #301 into the PR and updated the WASM build instructions to say cargo run_wasm -p with_winit --bin with_winit_bin (as the --bin part is still necessary for it to work correctly).

Copy link
Member

@DJMcNab DJMcNab left a comment

Choose a reason for hiding this comment

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

So long as the most recent changes have been tested (i.e. in a browser), this seems all good

README.md Outdated Show resolved Hide resolved
@armansito armansito merged commit 8b2ea01 into linebender:main Apr 25, 2023
@armansito armansito deleted the fix-wasm branch April 25, 2023 07:40
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.

WASM crashes
3 participants