immersive-web / webxr-samples

Samples to demonstrate use of the WebXR Device API
https://immersive-web.github.io/webxr-samples/
MIT License
987 stars 474 forks source link

Add basic layers samples #106

Closed felixtrz closed 3 years ago

felixtrz commented 3 years ago

This PR adds a set of basic WebXR Layers samples, there will be another PR that adds the more advanced samples. @toji

toji commented 3 years ago

Thank you again for breaking these up into smaller PRs! It makes them much easier to review comprehensively. Before looking into the code much I checked this PR out locally and gave the samples a run on my Quest 2. For the most part they look great! The layer rendering is extremely crisp. There's a few questions or points of confusion that I think should be addressed before landing them, though:

felixtrz commented 3 years ago

Thank you again for breaking these up into smaller PRs! It makes them much easier to review comprehensively. Before looking into the code much I checked this PR out locally and gave the samples a run on my Quest 2. For the most part they look great! The layer rendering is extremely crisp. There's a few questions or points of confusion that I think should be addressed before landing them, though:

  • You can remove the "Does my browser support WebXR?" section from the top of the page. We only really need that on the main page.
  • I was a bit confused at first by the projection layer sample, because I kept waiting for something to load. Eventually I realized that the fact that I could see the FPS counter meant that it was working, and that sample had no environment mesh. But for several minutes I thought it was broken. I suggest having that sample load one of the environments we have from the other samples just to avoid users thinking something is wrong. Since you're using a 'local' reference space the space scene is probably a good one.
  • The other samples don't need an environment, as the other layers give you an indication that the sample worked.
  • The stereo image on the quad layer and cylinder layer sample appears to have the left/right views inverted. It's hard to tell. Also, that image (of the bird from Big Buck Bunny) is really blurry, and looks like a screencap of the video. It would be preferable if we could find a higher quality stereo image to demo, since the whole point if this API is improving image quality.
  • The images with the robots are really nice looking, but I want to ensure we have the rights to use them as part of these samples. If they are creatives commons licensed, as are most of the other assets these samples use, then we need to include acknowledgements of where the images were obtained from. Same goes for the equirect test patterns (and if you created them yourself please give yourself credit!) If the images are not under an appropriately open license please find some that we can use freely.
  • The image of the chessboard used for the stereo equirect sample has the left/right views inverted.
  • If I set the radius for the equirect sample to something like 5 or 1 nothing renders. Also, it's difficult to see the effect that value has if it can't be adjusted in realtime, so either finding a way to visualize it better or hiding that option for now may be appropriate.

I've made changes to:

  1. Remove WebXR compatibility check from layers sample page
  2. Remove equirect radius option
  3. Add space environment mesh to projection layer sample

The robot image was added a while ago by a former member of our team, we cannot find the rights information on these images any more, we will try to find images to replace these, as well as the bird image.

felixtrz commented 3 years ago

@toji I have:

  1. replaced the cubemap images and added photo credit
  2. replaced the rectangular stereo/mono images used in cylinder and quad layer sample and added photo credit
  3. corrected the invert stereo image chess-pano-4k and chess-pano-4k180

This should address all the issues you raised in your original feedback comment, please let me know if there is more to be done. Thanks!

felixtrz commented 3 years ago

@toji Thanks for the feedback, I've made changes for all of these issues except for the inverted issue, if you feel that this is not a significant blocker, we can land this first and I will investigate and address this in a follow up PR.

toji commented 3 years ago

Excellent! Thank you!

If you can figure out how to flip the cube maps in a follow-up patch it would be appreciated, and I'll let you know if anything else comes up. This is a really fantastic addition to the samples page, and I really appreciate your patience and perseverance in getting them landed!

felixtrz commented 3 years ago

Excellent! Thank you!

If you can figure out how to flip the cube maps in a follow-up patch it would be appreciated, and I'll let you know if anything else comes up. This is a really fantastic addition to the samples page, and I really appreciate your patience and perseverance in getting them landed!

And thank you very much for bearing with me! This is a lot of code to review and I really appreciate you taking the time! Super excited to land all the samples and hopefully get more people onboard!