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 the OVR_multiview2 WebGL extension #1933

Merged
merged 22 commits into from
Jun 30, 2022

Conversation

expenses
Copy link
Contributor

The WebGL OVR_multiview2 extension is implemented in:

  • Chrome on Windows, Linux and Android
  • Firefox on Android

but NOT on Firefox on Linux afaik.

This is the main (there's also an Occulus extension: https://developer.mozilla.org/en-US/docs/Web/API/OVR_multiview2) extension used for multiview rendering in WebGL.

Registry link: https://www.khronos.org/registry/webgl/extensions/OVR_multiview2/
MDN link: https://developer.mozilla.org/en-US/docs/Web/API/OVR_multiview2

The number of views needs to be passed in as a pipeline option and then set in the vertex shader with layout(num_views = X). I have a working (but messy) integration with wgpu here: expenses/wgpu@7da24f3. Note that this relies on rustwasm/wasm-bindgen#2903 and grovesNL/glow@main...expenses:multiview.

We need a good way to check whether the target API is WebGL so that we can determine whether to use this extension or the regular GL_EXT_multiview one.

@expenses expenses marked this pull request as ready for review June 3, 2022 23:05
@expenses
Copy link
Contributor Author

expenses commented Jun 3, 2022

We need a good way to check whether the target API is WebGL so that we can determine whether to use this extension or the regular GL_EXT_multiview one.

I've solved this by using Embedded { version: u16, is_webgl: bool } in the version. Not sure if this is the best way.

@expenses expenses changed the title WIP support for the OVR_multiview2 WebGL extension Support for the OVR_multiview2 WebGL extension Jun 4, 2022
@jimblandy jimblandy requested a review from JCapucho June 14, 2022 18:48
@jimblandy
Copy link
Member

@JCapucho, if this isn't something you can review feel free to turn down the review request - I just wanted to put this on someone's plate to see if we could make some progress.

Copy link
Collaborator

@JCapucho JCapucho left a comment

Choose a reason for hiding this comment

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

I'll be honest, I don't have a lot of experience with webgl and have almost 0 experience with multiview, but I've looked at the extensions and overall everything looks mostly fine.

But I think there are some problems.

src/back/glsl/mod.rs Outdated Show resolved Hide resolved
src/back/glsl/features.rs Show resolved Hide resolved
@expenses expenses requested a review from JCapucho June 15, 2022 15:23
@expenses
Copy link
Contributor Author

@JCapucho I've re-done this a little bit to make selecting which multiview extension to use explicit. I've put the multiview options on the glsl options for convenience, but wgpu creates these when creating a pipeline layout so we need to set it to None there:

https://github.com/expenses/wgpu/blob/dbab152093dd9ca1ca255a57df4048a92b857325/wgpu-hal/src/gles/device.rs#L881-L888

Should I be putting the multiview options on the pipeline options instead?

Also the nightly test seems to be broken by something outside of this PR.

@JCapucho
Copy link
Collaborator

JCapucho commented Jun 15, 2022

@JCapucho I've re-done this a little bit to make selecting which multiview extension to use explicit.

What's the benefit of doing it this way? It seems to add a lot of code with no real benefit

I've put the multiview options on the glsl options for convenience, but wgpu creates these when creating a pipeline layout so we need to set it to None there:

https://github.com/expenses/wgpu/blob/dbab152093dd9ca1ca255a57df4048a92b857325/wgpu-hal/src/gles/device.rs#L881-L888

Should I be putting the multiview options on the pipeline options instead?

If they are only available in the pipeline section they should be added in pipeline options.

Also the nightly test seems to be broken by something outside of this PR.

I noticed, seems like some PR didn't update the snapshots, I'll see if I can fix it
Nevermind seems like nightly ron as defaulted to always add the decimal part of floating point numbers.

@expenses
Copy link
Contributor Author

What's the benefit of doing it this way? It seems to add a lot of code with no real benefit

I think it's nicer to only specify which extension to use if the multiview options are set, as opposed to always having is_webgl in the version. It could be streamlined though but I don't think that it adds that much code.

@JCapucho
Copy link
Collaborator

I think it's nicer to only specify which extension to use if the multiview options are set, as opposed to always having is_webgl in the version. It could be streamlined though but I don't think that it adds that much code.

My worry is that we are opening ourselves up to have the backend mismatch extensions and version, with this approach it's possible to set the version to core and produce a webgl extension, and if we end up doing this for other extensions the problem will quickly get worse.

@expenses
Copy link
Contributor Author

My worry is that we are opening ourselves up to have the backend mismatch extensions and version, with this approach it's possible to set the version to core and produce a webgl extension, and if we end up doing this for other extensions the problem will quickly get worse.

So my thinking is that we'd only need to check for WebGL with this extension, so doing this would be okay. Unfortunately https://www.khronos.org/registry/webgl/extensions/WEBGL_multi_draw/ also needs a specific WebGL extension: GL_ANGLE_multi_draw, so this would cause problems if we wanted to support that.

So I think I'll go and revert these changes to have is_webgl on the version again. Sorry about that.

@JCapucho
Copy link
Collaborator

So my thinking is that we'd only need to check for WebGL with this extension, so doing this would be okay. Unfortunately https://www.khronos.org/registry/webgl/extensions/WEBGL_multi_draw/ also needs a specific WebGL extension: GL_ANGLE_multi_draw, so this would cause problems if we wanted to support that.

So I think I'll go and revert these changes to have is_webgl on the version again. Sorry about that.

No worries, sometimes we gotta try stuff to realize it's not the best solution.

Copy link
Collaborator

@JCapucho JCapucho left a comment

Choose a reason for hiding this comment

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

I think everything implementation wise is good (I left just a small nit), but in terms of testing I think it's best to use wgsl tests instead of spirv, since wgsl is the "blessed" frontend and it allows us to quickly see what a test should produce especially when reviewing.

}

impl Version {
/// Create a new non-WebGL embedded version
pub const fn new_embedded(version: u16) -> Self {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think a better name for this would be gles since embedded is the name of the glsl profile and in this case we are trying to distinguish the platform.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@expenses expenses requested a review from JCapucho June 21, 2022 14:59
Copy link
Collaborator

@JCapucho JCapucho left a comment

Choose a reason for hiding this comment

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

Just one more small thing and we are good to go

tests/snapshots.rs Outdated Show resolved Hide resolved
@expenses expenses requested a review from JCapucho June 30, 2022 16:29
@JCapucho
Copy link
Collaborator

Brilliant work, just needs the auto formatter to run and it's good to merge

@expenses
Copy link
Contributor Author

@JCapucho okay, lets try that!

@JCapucho JCapucho merged commit e2d6880 into gfx-rs:master Jun 30, 2022
bors bot added a commit to grovesNL/glow that referenced this pull request Jul 15, 2022
220: Add a `framebuffer_texture_multiview_ovr` function to the web-sys context r=grovesNL a=expenses

Required for multiview rendering. See also: gfx-rs/naga#1933 and rustwasm/wasm-bindgen#2903.

Registry link: https://www.khronos.org/registry/webgl/extensions/OVR_multiview2/
MDN link: https://developer.mozilla.org/en-US/docs/Web/API/OVR_multiview2

~~Blocked until a new wasm-bindgen release.~~

Co-authored-by: Ashley Ruglys <[email protected]>
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