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

Examples: Use PBR material in webxr_ar_lighting again. #23171

Merged
merged 1 commit into from
Jan 12, 2022

Conversation

Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented Jan 7, 2022

Related issue: -

Description

This PR restores the PBR materials support of XREstimatedLight . I have some concerns though:

The WebXR API returns an environment map which seems to be an irradiance environment map. I'm not sure but is this type of map the intended input of PMREMGenerator?

Wouldn't it be more correct to transform the irradiance environment map to an instance of LightProbe (since we can't directly sample irradiance environment maps right now) and add it to the scene?

Besides, XREstimatedLight uses an instance of LightProbe and DirectionalLight for representing the estimated lighting. Does it make sense to add an instance of XREstimatedLight and the irradiance environment map to the scene? Isn't the irradiance added twice now?

/cc @toji @elalish

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Jan 7, 2022

https://raw.githack.com/Mugen87/three.js/dev19/examples/webxr_ar_lighting.html

@optimus007
Copy link

optimus007 commented Jan 7, 2022

I can't see the blurry reflection in the sphere. Is this correct ?

148534452-140fcaf1-6340-4147-b356-4e71ae362970

148534459-9e95aa47-0375-43ac-9709-1194b3437aa1

@optimus007
Copy link

optimus007 commented Jan 7, 2022

148537672-05434dec-5a64-4c49-9e6a-07d336aedf01

Phong seems to showing some kind of blurry reflection which the standard does not.

& There were no errors in the console
(Just some renderer.resize warnings)

Might be my phone .

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Jan 7, 2022

Phong seems to showing some kind of blurry reflection which the standard does not.

Like I said above I'm not sure if it is correct to process an irradiance environment map with PMREMGenerator. This needs to be clarified so we know if the visual result is actually right.

@mrdoob mrdoob added this to the r137 milestone Jan 7, 2022
@WestLangley
Copy link
Collaborator

...an irradiance environment map. I'm not sure but is this type of map the intended input of PMREMGenerator?

No.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Jan 10, 2022

Good! Then I don't think the following line should be part of the example:

scene.environment = xrLight.environment;

It's probably sufficient to just add the instance of XREstimatedLight to the scene.

@WestLangley
Copy link
Collaborator

...an irradiance environment map. I'm not sure but is this type of map the intended input of PMREMGenerator?

No.

Good! Then I don't think the following line should be part of the example:

scene.environment = xrLight.environment;

@Mugen87 I am not inclined to think xrLight.environment is an irradiance map.

I think xrLight.environment is a radiance cube map of size 16px. (or more likely, a luminance map)

If so, that size is not compatible with PMREM.

/ping @toji
/ping @elalish

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Jan 10, 2022

Unfortunately, the type of the map is not clear to me from the spec: https://www.w3.org/TR/2021/WD-webxr-lighting-estimation-1-20210909/

@toji
Copy link
Contributor

toji commented Jan 10, 2022

Unfortunately the underlying APIs that we use to expose those values to the web aren't very clear about exactly what those values represent either, aside from being used for "reflections".

https://developers.google.com/ar/develop/c/lighting-estimation/introduction?hl=cs#hdr_cubemap
https://developers.google.cn/ar/reference/c/group/ar-light-estimate?hl=ur#arlightestimate_acquireenvironmentalhdrcubemap

https://developer.apple.com/documentation/arkit/camera_lighting_and_effects/adding_realistic_reflections_to_an_ar_experience
https://developer.apple.com/documentation/arkit/arenvironmentprobeanchor/2977511-environmenttexture

That said, despite the low resolution the intention of the textures is to give the impression of a plausible, if blurry, reflection of the user's environment in shiny surfaces. I would expect rendering using it to visually resemble the "Phong" screenshot that @optimus007 posted, whereas the "Standard" screenshot feels like it's not making use of it at all.

And yes, as far as I am aware they're not compatible with PMREMGenerator. I talked to @elalish about it at the time I was doing the initial implementation and we determined they wouldn't work as-is, but I forget if there was some sort of fundamental incompatibility there or if it was simply an implementation detail of PMREMGenerator.

@elalish
Copy link
Contributor

elalish commented Jan 10, 2022

I believe this PR is about how PMREM can finally be applied to the reflection map. I think our only concerns were pixel format and performance, which might both be solved now. @WestLangley As for size, agreed that it's not optimal, but PMREMGenerator should work fine with any size input cubemap. This is probably a good excuse to finally make the internal PMREM format adjustable size, but that's just an optimization.

As for radiance vs irradiance; I personally hate those words because I can never remember which one is which. But yes, xrLight.environment is estimated incoming light from the world, supposedly identical to an applied HDR map.

@elalish
Copy link
Contributor

elalish commented Jan 10, 2022

On another note; that reflection map really shouldn't be 16x16; that's part of why ARCore's lighting estimation is so bad. ARKit can return a much higher-res version, and if they ever ship WebXR, this will likely not be so small.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Jan 11, 2022

So according to the guides and the feedback in this thread the xrLight.environment is essentially a cube map generated with a cube camera. I think Unity calls this a reflection probe. In this case, we can indeed use the input for MeshStandardMaterial.envMap. Updated the PR accordingly.

@optimus007
Copy link

optimus007 commented Jan 11, 2022

148922049-dfc85cd3-bb8c-482c-8c95-c9a02f90f88a

The equirectangular defaultHdri seems to be converted into a cube map style now

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Jan 11, 2022

@optimus007 Should be fixed now.

@optimus007
Copy link

optimus007 commented Jan 11, 2022

148931689-7d0bccf3-5388-4743-b451-253810d2346e

👍

Environment from hdri
&
Light from xrLight.lightProbe and xrLight.directionalLight

Correct ?

@WestLangley
Copy link
Collaborator

That is way over-bright. Too much light and/or improper tone mapping.

Also, you should not be applying a light probe and an environment map with MeshStandardMaterial.

The directional light should also be unnecessary.

@optimus007
Copy link

InShot_20220111_211033082.mp4

The environment is not reflecting again. No errors in the console

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Jan 11, 2022

The environment is not reflecting again.

This is not an error but the result you get with MeshStandardMaterial. The environment map is definitely applied.

The directional light should also be unnecessary.

It is part of XREstimatedLight and I'm not going to touch its composition in this PR.

I'm just trying to find a way how to use XREstimatedLight with PBR materials. If we can't agree on a solution, it's better to close this PR and just continue to use the phong material.

The example is now in its original state (#20876), just with PMREM support.

@elalish
Copy link
Contributor

elalish commented Jan 11, 2022

@WestLangley regarding the combination of light sources, generally I would agree you shouldn't use them all together, but it depends on how they are being estimated in XR. This passage makes me think that in fact applying all of them is correct:

This lighting estimation mode provides:

- Main directional light. Represents the main light source. Can be used to cast shadows.
- Ambient spherical harmonics. Represents the remaining ambient light energy in the scene.
- An HDR cubemap. Can be used to render reflections in shiny metallic objects.

You can use these APIs in different combinations, but they're designed to be used together for the most realistic effect.

@optimus007
Copy link

optimus007 commented Jan 11, 2022

InShot_20220111_223258356.mp4

Testing this pr in my webXR page. Looks great 👍, metallic objects don't appear black like before and the webgl warnings are gone. 🙌

The lack of blurry reflection can be solved in another update

Google's scene viewer uses the blurry reflection maybe someone who developed that can help

148990864-02560499-f6fe-419a-bd39-eebcdd92d206

@WestLangley
Copy link
Collaborator

As for radiance vs irradiance; I personally hate those words because I can never remember which one is which.

@elalish This is important. Do you understand that PMREM accounts for both the radiance and the irradiance implied by the environment map?

See #22178 (comment) and #22178 (comment).

That means adding a radiance probe and an irradiance probe is double-counting irradiance.

If you want to accommodate the modeling approach of ARCore, then you have to change how PMREM is implemented.

@mrdoob
Copy link
Owner

mrdoob commented Jan 11, 2022

Regarding the fact that the reflection is (too) blurred...
I suspect the issue is that the PMREM code is scaling down the 16x16 cubemap.

@WestLangley
Copy link
Collaborator

I suspect the issue is that the PMREM code is scaling down the 16x16 cubemap.

Right. PMREM is not designed for a 16px input.

@elalish
Copy link
Contributor

elalish commented Jan 11, 2022

That means adding a radiance probe and an irradiance probe is double-counting irradiance.

Yeah, that may be true; it's a tad unclear from ARCore's documentation. Basically the question is, are their returned spherical harmonics simply an integration of their returned env map (representing its irradiance)? Or are they separate light sources intended to be added together? I should probably ask the ARCore team; I've noticed WebXR scenes often being too dark, so I was thinking that might be because we weren't use all the light sources they gave us, but I may well be wrong.

And yes, I really need to update PMREM to properly handle different input env map sizes.

@mrdoob
Copy link
Owner

mrdoob commented Jan 11, 2022

@elalish Should I merge this in the meantime?

@elalish
Copy link
Contributor

elalish commented Jan 11, 2022

Seems fine to me; it's certainly an improvement, though there's more work to do.

@mrdoob mrdoob merged commit 3bf0ae2 into mrdoob:dev Jan 12, 2022
@mrdoob
Copy link
Owner

mrdoob commented Jan 12, 2022

Thanks!

@elalish
Copy link
Contributor

elalish commented Jan 15, 2022

@Mugen87 @mrdoob I'm looking at making PMREM's internal CubeUV format have variable size, depending on the input texture dimensions. I think I've got it worked out, but the part I'm struggling with is where to communicate that size to the shaders. Would you like it as a #define like ENV_MAP_TYPE, or as a uniform value like toneMappingExposure? Where should that value be set?

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Jan 15, 2022

Is it a valid use case to change the size of a single env map over time? I would expect no. I mean you might have maps of different sizes but once a size is defined it is static.

If so, I think we should use a define.

@elalish elalish mentioned this pull request Jan 24, 2022
@AdaRoseCannon
Copy link
Contributor

I'm working on re-adding lighting estimation to AFrame, unfortunately it seems something odd is happening. I'm using this.lightingEstimationTexture.needsPMREMUpdate = true; after replacing the __webglTexture and that managed to get it partly working.

It seems that the way the texture is being used by the environment is peculiar. It seems to have most of the colour but none of the detail and an artifact that looks like a small thin vertical rectangle. I took a screen shot below, the left shows what the actual texture looks like, the right is how it looks on a material with 0 roughness. I took a look through the lighting estimation example and I don't think I am missing anything.

Screenshot_20220125-104510_Samsung Internet Beta

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Jan 25, 2022

@AdaRoseCannon #23322 should improve the reflections of XREstimatedLight's env map. You might want to download the branch, update the build files and use them temporarily for testing.

@AdaRoseCannon
Copy link
Contributor

Do you think the mismatched size of the env map is what's causing this issue?

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Jan 25, 2022

At least the current PMREMGenerator does not properly handle 16x16 env maps. The PR is going to improve this situation.

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.

7 participants