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

Support for --target wasm32-unknown-emscripten #2233

Closed
wants to merge 1 commit into from

Conversation

caiiiycuk
Copy link
Contributor

Description
I found that adding support for wasm32-unknown-emscripten is not so hard. Even more implementation is not depended on 3rd party libraries like winit or stdweb. It's because emscripten it self provides complete implementation for creating OpenGL 3.0 context. So crates like glow, khronos-egl works as is. Only visible differences between emscripten and native platform is that emscripten does not support pbuffer (which is not needed) and window will created automatically.

I think supporting wasm32-unknown-emscripten will be a great unique feature of wgpu.

Testing

You can test this changes by calling ./run-wasm-emscripten-example.sh. However you need to install emsdk first, and emcc should be in your path.

@caiiiycuk
Copy link
Contributor Author

Can I just apply cargo fmt to fix formatting errors?

@kvark
Copy link
Member

kvark commented Nov 30, 2021

Can I just apply cargo fmt to fix formatting errors?

yes, that's what we do.
Unless the rustfmt version on CI is very different from yours

Copy link
Member

@kvark kvark left a comment

Choose a reason for hiding this comment

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

Thank you for experimenting with this!
I agree it doesn't look too bad, and we can have this support.
In addition to the notes in the review, we need a CI job to test this configuration.

Cargo.toml Outdated Show resolved Hide resolved
wgpu/examples/hello-triangle-em/main.rs Outdated Show resolved Hide resolved
wasm-resources/index.em.template.html Outdated Show resolved Hide resolved
@caiiiycuk
Copy link
Contributor Author

Blocked by grovesNL/glow#195

@caiiiycuk caiiiycuk force-pushed the emscripten branch 5 times, most recently from e86e194 to cd7723a Compare December 3, 2021 08:23
@caiiiycuk
Copy link
Contributor Author

Done

.github/workflows/ci.yml Show resolved Hide resolved
.github/workflows/ci.yml Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
wasm-resources/index.em.template.html Show resolved Hide resolved
@caiiiycuk caiiiycuk force-pushed the emscripten branch 3 times, most recently from 8a608fb to 6b867dd Compare December 7, 2021 10:28
@caiiiycuk
Copy link
Contributor Author

Updated. Blocker is gone, can be merged now if it's ok

# Emscripten WebGL
- name: WebAssembly emscripten
os: ubuntu-20.04
channel: stable
Copy link
Member

Choose a reason for hiding this comment

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

line no longer applicable

if: matrix.kind == 'webgl-em'
run: |
# check with no features
cargo ${{matrix.tool}} --target ${{ matrix.target }} -p wgpu -p wgpu-core
Copy link
Member

Choose a reason for hiding this comment

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

I think we need to do the same as regulalr webgl - require webgl feature to be passed in. Otherwise, this will be confusing when we get emscripten webgpu target.

unsafe fn init(desc: &crate::InstanceDescriptor) -> Result<Self, crate::InstanceError> {
let egl_result = if cfg!(windows) {
Copy link
Member

Choose a reason for hiding this comment

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

why is this removed?

@@ -613,6 +637,9 @@ impl crate::Instance<super::Api> for Instance {
(display, None, WindowKind::Unknown)
};

#[cfg(target_os = "emscripten")]
let display = egl.get_display(egl::DEFAULT_DISPLAY).unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

wouldn't it fall under this path anyway in the previous if/else chain?

@@ -946,6 +979,8 @@ impl crate::Surface<super::Api> for Surface {
wl_window = Some(window);
window
}
#[cfg(target_os = "emscripten")]
Copy link
Member

Choose a reason for hiding this comment

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

no need to cfg this line

)
}
_ => self.egl.create_window_surface(
#[cfg(not(target_os = "emscripten"))]
Copy link
Member

Choose a reason for hiding this comment

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

no need for this cfg, it will go the regular create_window_surface way naturally, since self.wsi.kind == WindowKind::Unknown

@@ -84,13 +84,13 @@ replay = ["serde", "wgc/replay"]
angle = ["wgc/angle"]
webgl = ["wgc"]

[target.'cfg(not(target_arch = "wasm32"))'.dependencies.wgc]
[target.'cfg(any(not(target_arch = "wasm32"), target_os = "emscripten"))'.dependencies.wgc]
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this configuration needs to change. Instead, it should be aligned with the current webgl path.

#[cfg(all(target_arch = "wasm32", not(feature = "webgl")))]
#[cfg(all(
target_arch = "wasm32",
not(target_os = "emscripten"),
Copy link
Member

Choose a reason for hiding this comment

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

similarly, this configuration doesn't need to change. If emscripten path requires "webgl" feature, it mostly follows the same steps.

@caiiiycuk
Copy link
Contributor Author

Will do new one based on "feature"

@caiiiycuk caiiiycuk closed this Jan 24, 2022
@caiiiycuk caiiiycuk mentioned this pull request Jan 25, 2022
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.

2 participants