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

Add WebGPU backend #3231

Merged
merged 1 commit into from
May 15, 2020
Merged

Add WebGPU backend #3231

merged 1 commit into from
May 15, 2020

Conversation

GabrielMajeri
Copy link
Contributor

@GabrielMajeri GabrielMajeri commented Apr 26, 2020

This PR adds a WebGPU backend for gfx-hal, making some progress towards #3027.

I've currently gotten the backend to compile. Further additions will come in future PRs.

Copy link

@monocodus monocodus bot left a comment

Choose a reason for hiding this comment

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

This is an autogenerated code review, only first 25 of 88 new suggestions are shown

The .monocodus config not found in your repo. Default config is used.
Check config documentation here

src/backend/wgpu/src/command.rs Outdated Show resolved Hide resolved
src/backend/wgpu/src/command.rs Outdated Show resolved Hide resolved
src/backend/wgpu/src/command.rs Outdated Show resolved Hide resolved
src/backend/wgpu/src/command.rs Outdated Show resolved Hide resolved
src/backend/wgpu/src/command.rs Outdated Show resolved Hide resolved
src/backend/wgpu/src/command.rs Outdated Show resolved Hide resolved
src/backend/wgpu/src/command.rs Outdated Show resolved Hide resolved
src/backend/wgpu/src/command.rs Outdated Show resolved Hide resolved
src/backend/wgpu/src/command.rs Outdated Show resolved Hide resolved
src/backend/wgpu/src/command.rs Outdated Show resolved Hide resolved
Copy link

@monocodus monocodus bot left a comment

Choose a reason for hiding this comment

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

This is an autogenerated code review, new suggestions: 7

The .monocodus config not found in your repo. Default config is used.
Check config documentation here

src/backend/wgpu/src/device.rs Outdated Show resolved Hide resolved
src/backend/wgpu/src/device.rs Outdated Show resolved Hide resolved
src/backend/wgpu/src/device.rs Outdated Show resolved Hide resolved
src/backend/wgpu/src/device.rs Outdated Show resolved Hide resolved
src/backend/wgpu/src/window.rs Outdated Show resolved Hide resolved
src/backend/wgpu/src/window.rs Outdated Show resolved Hide resolved
src/backend/wgpu/src/window.rs Outdated Show resolved Hide resolved
@kvark
Copy link
Member

kvark commented Apr 26, 2020

Oh wow, you are attacking this task, this is great news!
Please note that #3027 doesn't explain clearly how this needs to be done. We didn't understand all the details ourselves. So let me at least try to do a better job here.

So the main goal of this backend is to target the Web. This means, we don't care in particular about wgpu-core or even wgpu-native (even though that one could go through emscripten to the Web). Instead, we care about the path that wgpu-rs uses for the Web backend, which is web-sys.

Now, if you just try to link to wgpu-rs, you'll be forced to deal with the fact that this is meant to be used by the end users and not intermediate layers. The API can be restrictive, making this backend impossible. Therefore, I believe the only sensible path for gfx-backend-wgpu is to link to web-sys directly and calling the web methods, without linking to any of the wgpu ecosystem crates at all.

Does that make sense? @grovesNL can correct if I said this wrong.

@kvark kvark mentioned this pull request Apr 26, 2020
@GabrielMajeri
Copy link
Contributor Author

GabrielMajeri commented Apr 26, 2020

Yeah, makes sense. I will look into switching to web-sys.

The only problem I've encountered so far, is that some of the WebGPU APIs are async (for example requesting a device). How do we expose that through the gfx-hal API?

@kvark
Copy link
Member

kvark commented Apr 26, 2020

That's one of the architectural blockers to resolve, it could be a serious issue.
I wonder if we can just have an executor blocking on that? I.e. to start, just have an executor being a part of the backend, and call block_on on the wasm_bindgen_futures::JsFuture that is supposed to be async.
@grovesNL ^

@grovesNL
Copy link
Contributor

@kvark we won't be able to use block_on on the web (e.g. rustwasm/wasm-bindgen#1733 (comment))

@GabrielMajeri
Copy link
Contributor Author

Does this mean we'll have to modify the gfx-hal API to be async?

@kvark
Copy link
Member

kvark commented Apr 26, 2020

No, we aren't going to modify gfx-hal API here.
We need to figure out what to...

@grovesNL
Copy link
Contributor

I'm not sure of the best way to handle this, some options could be:

  • modify gfx-hal's API
  • expose async functions as a feature specific to WebGPU, so we'd have separate entry points specific to WebGPU
  • work with promises/futures internally in most functions/types, so functions are implemented behind promise chains (e.g. device_promise.then(device => device.xyz()))
  • try to find some way to yield the browser temporarily at the Rust level (like async/await itself does internally), similar to Emscripten's Asyncify https://emscripten.org/docs/porting/emscripten-runtime-environment.html#using-asyncify-to-yield-to-the-browser

Maybe there are other ways to handle this?

@GabrielMajeri
Copy link
Contributor Author

The easiest solution for now seems to be to expose separate entry points specific for WebGPU (at least temporarily, until a better solution is proposed).

I could make the open function panic when called, telling people to instead use open_async or something like that on WASM.

Would that be acceptable?

Copy link

@monocodus monocodus bot left a comment

Choose a reason for hiding this comment

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

This is an autogenerated code review, new suggestions: 20

The .monocodus config not found in your repo. Default config is used.
Check config documentation here

src/backend/wgpu/src/command.rs Outdated Show resolved Hide resolved
src/backend/wgpu/src/device.rs Outdated Show resolved Hide resolved
src/backend/wgpu/src/device.rs Outdated Show resolved Hide resolved
src/backend/wgpu/src/device.rs Outdated Show resolved Hide resolved
src/backend/wgpu/src/device.rs Outdated Show resolved Hide resolved
src/backend/wgpu/src/window.rs Outdated Show resolved Hide resolved
src/backend/wgpu/src/window.rs Outdated Show resolved Hide resolved
src/backend/wgpu/src/window.rs Outdated Show resolved Hide resolved
src/backend/wgpu/src/window.rs Outdated Show resolved Hide resolved
src/backend/wgpu/src/window.rs Outdated Show resolved Hide resolved
Copy link

@monocodus monocodus bot left a comment

Choose a reason for hiding this comment

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

This is an autogenerated code review, new suggestions: 21

The .monocodus config not found in your repo. Default config is used.
Check config documentation here

src/backend/wgpu/src/command.rs Outdated Show resolved Hide resolved
src/backend/wgpu/src/command.rs Outdated Show resolved Hide resolved
src/backend/wgpu/src/device.rs Outdated Show resolved Hide resolved
src/backend/wgpu/src/device.rs Outdated Show resolved Hide resolved
src/backend/wgpu/src/device.rs Outdated Show resolved Hide resolved
src/backend/wgpu/src/window.rs Outdated Show resolved Hide resolved
src/backend/wgpu/src/window.rs Outdated Show resolved Hide resolved
src/backend/wgpu/src/window.rs Outdated Show resolved Hide resolved
src/backend/wgpu/src/window.rs Outdated Show resolved Hide resolved
src/backend/wgpu/src/window.rs Outdated Show resolved Hide resolved
@kvark
Copy link
Member

kvark commented Apr 27, 2020

@GabrielMajeri this would let you to proceed and get something working. Similar to how we have OpenGL API somewhat working but requiring application changes for a while (but no more). When we find a proper solution for async, we wouldn't need to scrap a lot of code, so there is nothing to lose if we go this direction here.

@GabrielMajeri GabrielMajeri marked this pull request as ready for review April 27, 2020 15:56
@GabrielMajeri
Copy link
Contributor Author

@kvark Alright, I've started work on that. Would it be OK to get this merged as-is (since its just boilerplate code), and add instance creation in the next PR, for easier review?

@kvark
Copy link
Member

kvark commented Apr 27, 2020

Yeah, as long as CI is happy :)

@GabrielMajeri
Copy link
Contributor Author

GabrielMajeri commented Apr 27, 2020

Ok! I've looked at the logs for the failing build. It seems to be an ICE on nightly:

error: internal compiler error: src/librustc_mir/interpret/validity.rs:396:
Unexpected error during ptr inbounds test: cannot read from foreign (extern) static DefId(0:40 ~ dispatch[5c52]::ffi[0]::[0]::_dispatch_queue_attr_concurrent[0])

I'll see if it's already reported.

@kvark
Copy link
Member

kvark commented Apr 27, 2020

@GabrielMajeri also, let's please rename this to gfx-backend-webgpu in folder "webgpu". This matches "vulkan" and "metal" better, since they aren't "vk" and "mtl". We also don't want to confuse people with the idea that this is going through wgpu library, since we are going to target the Web directly.

Copy link

@monocodus monocodus bot left a comment

Choose a reason for hiding this comment

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

This is an autogenerated code review, new suggestions: 20

The .monocodus config not found in your repo. Default config is used.
Check config documentation here

src/backend/webgpu/src/command.rs Show resolved Hide resolved
src/backend/webgpu/src/device.rs Show resolved Hide resolved
src/backend/webgpu/src/device.rs Show resolved Hide resolved
src/backend/webgpu/src/device.rs Show resolved Hide resolved
src/backend/webgpu/src/device.rs Show resolved Hide resolved
src/backend/webgpu/src/window.rs Show resolved Hide resolved
src/backend/webgpu/src/window.rs Show resolved Hide resolved
src/backend/webgpu/src/window.rs Show resolved Hide resolved
src/backend/webgpu/src/window.rs Show resolved Hide resolved
src/backend/webgpu/src/window.rs Show resolved Hide resolved
@GabrielMajeri
Copy link
Contributor Author

@kvark Noted, renamed everything from wgpu to webgpu.

@kvark
Copy link
Member

kvark commented Apr 27, 2020

I'm disabling monocodus warnings in #3234. Sorry about the mess it makes!

@kvark
Copy link
Member

kvark commented May 4, 2020

bors try

bors bot added a commit that referenced this pull request May 4, 2020
@bors
Copy link
Contributor

bors bot commented May 4, 2020

try

Build failed:

@kvark
Copy link
Member

kvark commented May 4, 2020

Looks like the nightly failure will be fixed in the next nightly...

@kvark
Copy link
Member

kvark commented May 15, 2020

bors try

bors bot added a commit that referenced this pull request May 15, 2020
@bors
Copy link
Contributor

bors bot commented May 15, 2020

try

Build failed:

@kvark
Copy link
Member

kvark commented May 15, 2020

@GabrielMajeri sorry, looks like it needs to be updated now that Mesh shaders have landed. It should be fairly straightforward though. Would you mind rebasing and updating it?

error[E0046]: not all trait items implemented, missing: `draw_mesh_tasks`, `draw_mesh_tasks_indirect`, `draw_mesh_tasks_indirect_count`
   --> src/backend/webgpu/src/command.rs:134:1

@GabrielMajeri
Copy link
Contributor Author

No problem, fixed now.

@kvark
Copy link
Member

kvark commented May 15, 2020

bors r+

@bors
Copy link
Contributor

bors bot commented May 15, 2020

Build succeeded:

@bors bors bot merged commit bbfdebe into gfx-rs:master May 15, 2020
@GabrielMajeri GabrielMajeri deleted the wgpu-backend branch May 15, 2020 18:05
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