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

Improve WebXR Layers example #22144

Merged
merged 3 commits into from
Jul 27, 2021
Merged

Improve WebXR Layers example #22144

merged 3 commits into from
Jul 27, 2021

Conversation

sigmaxipi
Copy link
Contributor

@sigmaxipi sigmaxipi commented Jul 17, 2021

Improve WebXR Layers example.

  • Add 4 eye charts to demonstrate the effects of mipmaps on text.
  • Add a GUI to demonstrate interacting with layers. (Mentioned in Support for multiple XRProjectionLayers #22120)
  • Improve info text and add more comments.
  • Add error if layers fail to initialize.
  • Add a preview screenshot.

Tested:

Sameer Padala and others added 3 commits July 15, 2021 21:30
* Add 4 eye charts to demonstrate the effects of mipmaps on text.
* Add a GUI to demonstrate interacting with layers.
* Improve info text and add more comments.
* Add a preview screenshot.

Tested:
  * Oculus Quest 2 shows layers properly.
  * Chrome on Windows shows error message.
@sigmaxipi
Copy link
Contributor Author

I'm not sure how to fix the screenshot test. It's failing because the screenshotting system doesn't use layers to render the video or eye charts. I manually took a screenshot which is more informative than the automatic screenshot.

Should I just use the automatic screenshot?

renderer.state.bindTexture( gl.TEXTURE_2D, glayer.colorTexture );
gl.pixelStorei( gl.UNPACK_FLIP_Y_WEBGL, true );
const canvas = guiMesh.material.map.image;
gl.texSubImage2D( gl.TEXTURE_2D, 0, 0, 0, canvas.width, canvas.height, gl.RGBA, gl.UNSIGNED_BYTE, canvas );
Copy link
Collaborator

@Mugen87 Mugen87 Jul 17, 2021

Choose a reason for hiding this comment

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

Sorry but I think all of this overcomplicates the example. We should try to keep it as simple as possible (that includes no WebGL API calls in the example code).

Related: #10269 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would it be clearer if I pulled the boilerplate code into a utility library at examples/jsm/webxr/layers.js? There is no way to demonstrate text rendering to layers without the gl.texSubImage2D call. And one of the major use cases of layers is for high quality text and 2D UI rendering.

Copy link
Collaborator

@Mugen87 Mugen87 Jul 18, 2021

Choose a reason for hiding this comment

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

Would it be clearer if I pulled the boilerplate code into a utility library at examples/jsm/webxr/layers.js?

That sounds like a step in the right direction. Ideally the code becomes some sort of reusable component.

@sigmaxipi sigmaxipi closed this Jul 21, 2021
@mrdoob
Copy link
Owner

mrdoob commented Jul 27, 2021

I agree that it over complicates the example, but I think it's better than the current example.

@mrdoob mrdoob reopened this Jul 27, 2021
@mrdoob mrdoob added this to the r131 milestone Jul 27, 2021
@mrdoob mrdoob merged commit 09236fb into mrdoob:dev Jul 27, 2021
@mrdoob
Copy link
Owner

mrdoob commented Jul 27, 2021

However, we should definitely make an abstraction of all this code in examples/jsm/webxr/ 👍

@sigmaxipi
Copy link
Contributor Author

I'm still working on trying to cleanly abstract the layer code into a separate file. I'm working on my own layer utility, but it's too complex to add into the examples. Once I'm satisfied with the design of my utility lib, I'll extract the minimal bits and create a new PR to update this example.

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