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 an example that uses WebXR's estimated AR lighting in feature. #20876

Merged
merged 3 commits into from
Feb 23, 2021

Conversation

toji
Copy link
Contributor

@toji toji commented Dec 10, 2020

This PR adds an example and utility class for showing how to take advantage of estimated lighting in AR.

The XREstimatedLight class can be added to the scene as if it were a regular light and any time an AR session with the appropriate permissions is started it will begin updating the lighting estimates automatically. It also has an environment property that automatically updates with an estimated cube map of the user's environment, but this is not automatically applied to the scene. We could easily make it do so, but I'm not sure that's the right choice for developers.

Internally the "light" is a group that contains both a LightProbe and a DirectionalLight, as well as the aformentioned environment cube map. I didn't see a need to protect these lights from being inspected or modified by users, since they may conceivably want to set them to non-AR-provided defaults at some point or evaluate the AR-provided values for some other purpose.

For scenarios where developers want to switch between a default lighting setup and AR lighting when appropriate the XREstimatedLight also provides a estimationstart and estimationend event to let developers know when the lights values are actively being updated. The example shows how to swap between lighting configurations when these events occur. It may also be beneficial to provide an event that indicates whenever the environment cube map is updates, in case developers want to do some processing on it (such as generating more accurate roughness mipmaps). I haven't added that yet in the interest of keeping API surface limited, but I'd like to know what developers need.

CC: @elalish, since I think he would like to use this in <model-viewer> and I'd appreciate feedback on if this would work for that use case.

Copy link
Contributor

@elalish elalish left a comment

Choose a reason for hiding this comment

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

Thanks! I'll need to look into the reflections for model-viewer, but this utility looks like it'll ease things considerably.

// approximately how frequently the event would fire anyway).
setInterval( () => {

this.updateReflection();
Copy link
Contributor

Choose a reason for hiding this comment

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

Does updating the environment have a cost in the event we aren't using it? And is there cost on the JS side, or only on the implementation side?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! There is a cost to updating it, which shouldn't be frame-droppingly severe but if your app wasn't using it (like you suggest below) then it's also wasted effort. We could turn the environment map into a getter attrib and only start updating it if you query it as a start, but that doesn't provide an easy way to stop using it, and it could accidentally get triggered by things like iterating over the object properties.

Copy link
Contributor

Choose a reason for hiding this comment

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

Might be worth just setting an input parameter to use it or not.


}

onXRFrame( time, xrFrame ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this mean the ARCore updates the directional light and SH on every frame but only updates the environment roughly every second? That seems odd; I would have expected all of this to be in the slow update function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ARCore probably only updates those values once in a while, but updating these values is pretty low-overhead (it's just changing which values will be set for a couple of uniforms, which happens per-frame anyway in most cases.) As a result on the API side we didn't consider it a burden to just treat the values as if they're new each frame. We could add an event to indicate concretely when changes happen if needed, but I'd like to see some real-world uses before determining if that's useful or not.

The environment maps are a different story, since not only do they involve creating a texture but also may also involve a behind-the-scenes format conversion. As such they're much heavier weight, relatively, to produce and we structured the API to encourage a sparser query.

scene.remove(defaultLight);

// The estimated lighting also provides an environment cubemap, which we can apply here.
scene.environment = xrLight.environment;
Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed that it's nice this is optional. Since high-res reflection with PMREM is so important for PBR, I'm thinking I might skip the estimated environment and use a static PMREM'ed one, but maybe a non-HDR like a jpg, so that most of the intense lighting comes from the directional and SH. I'm not a big fan of SH, but it's probably better than the alternatives.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd be really interested in seeing the results you can get out of that! There's no question that the low res environment map has limits. Everything tends to look rougher than intended above a certain point. That said if you have a perfectly crisp reflection that doesn't match your environment that will probably stand out too.

I wonder if there's a middle ground where you tone map a high res generic environment with the colors coming out of the low res one. 🤔 Gonna guess that falls into the "More trouble than it's worth" category, though.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm hoping the SH will tend to provide that tone map. As for the perfectly crisp reflection; no one will notice except on a spherical mirror like you have here. Most real objects are complex enough that you really can't tell what's reflected anyway. All that matters is how sharp the features are, which we need resolution to represent.

@mrdoob
Copy link
Owner

mrdoob commented Dec 21, 2020

I'm trying this on Chrome Beta (88.0.4324.51) and Chrome Dev (89.0.4356.5) but I only see the spheres reflecting the equirect. Does this need a flag?

@mrdoob mrdoob modified the milestones: r124, r125 Dec 21, 2020
@mrdoob mrdoob modified the milestones: r125, r126 Jan 27, 2021
@toji
Copy link
Contributor Author

toji commented Feb 22, 2021

Sorry for the very long delay following up!

Rebased this change and made a couple of small adjustments based on the feedback above:

  • Discovered that some recently added Three.js code was preventing user-supplied optionalFeatures in the ARButton from applying unless a DOM overlay was also explicitly set. Added a fix for that here.
  • Added a flag to the light constructor that allows developers to specify whether or not they want environment map estimation (defaulted to true), and did some minor shuffling of the code to better support the case where they don't.

And @mrdoob: Yes, you do currently need a flag ("XR Incubations" in about:flags) but it's expected to ship enabled by default fairly soon. There is an origin trial available which we could apply to this example, but I'm not sure if it's something you want to bother with?

@carstenschwede
Copy link
Contributor

carstenschwede commented Feb 23, 2021

@toji Awesome feature, on Android it crashes Chrome 88 but it seems to work at least for light intensity on Canary 90 (should colours be estimated as well?). Do any current MR headsets support it?

@mrdoob mrdoob merged commit a9112d5 into mrdoob:dev Feb 23, 2021
@mrdoob
Copy link
Owner

mrdoob commented Feb 23, 2021

Thanks!

@toji
Copy link
Contributor Author

toji commented Feb 23, 2021

Android it crashes Chrome 88

Sorry, that was a known issue that has now been fixed.

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.

4 participants