mrdoob / three.js

JavaScript 3D Library.
https://threejs.org/
MIT License
102k stars 35.33k forks source link

Scene: Lighting does not work if environment map has dimensions < 64 pixels #27716

Open gkjohnson opened 7 months ago

gkjohnson commented 7 months ago

Description

Lighting does not work at all when setting an environment map smaller than 64x64 to Scene.environment. Creating a small map on the fly is useful when generating basic, soft gradient lighting. I suspect this may be related to PMREMGenerator. The texture still works if it's set as a background.

64x64 63x63
image image

Reproduction steps

  1. Create a DataTexture with dimensions of 63x63 with white data
  2. Set mapping to "EquirectangularReflectionMapping"
  3. Lighting does not work

Code

const dim = 64; // NOTE: Lighting does not work if this value is changed to < 64
const tex = new THREE.DataTexture( new Uint8Array( dim * dim * 4 ).fill( 255 ), dim, dim );
tex.mapping = THREE.EquirectangularReflectionMapping;
tex.needsUpdate = true;
scene.environment = tex;

Live example

https://jsfiddle.net/y48Lqt1m/

Screenshots

No response

Version

r161

Device

Desktop

Browser

Chrome

OS

MacOS

elalish commented 7 months ago

Yeah, I don't think I bothered to make that work. Certainly possible, but it seemed like the added code complexity wasn't worth it - is it so bad to make a 64x64 pixel white image? I suppose that should at least be documented somewhere at the least though...

Mugen87 commented 7 months ago

We can documented a minimum texture size of 64x64 in the documentation page: https://threejs.org/docs/index.html#api/en/extras/PMREMGenerator

I agree if the code complexity isn't worth it, let's consider this as a known limitation. The potential memory savings of using a size < 64 are negligible anyway, imo.

gkjohnson commented 7 months ago

is it so bad to make a 64x64 pixel white image?

It's not bad it's just confusing.

Yeah, I don't think I bothered to make that work. Certainly possible, but it seemed like the added code complexity wasn't worth it

Can you elaborate on which part of the code base needs to be adjusted for add support? Is it the shader sampling or the generation step?

In addition to documentation it would be nice if it logged a waning, as well, since this wouldn't be a hot path.

elalish commented 7 months ago

It's the generation step - you'd need to upsample in addition to down-sample. At least I think that would be the simplest way.

RemusMar commented 7 months ago

We can documented a minimum texture size of 64x64 in the documentation page: https://threejs.org/docs/index.html#api/en/extras/PMREMGenerator

I agree if the code complexity isn't worth it, let's consider this as a known limitation. The potential memory savings of using a size < 64 are negligible anyway, imo.

+1 Honestly, an environment map smaller than 128x128 does not make much sense for me.

Mugen87 commented 7 months ago

Side note: It's right that for classic environment/reflection mapping you usually go for larger texture sizes. However, there is at least on valid exception: Irradiance environment maps (or diffuse environment maps) are a different representation of diffuse light probes (like our THREE.LightProbe class) and they usually have a small size. They only represent the diffuse, low-frequency part of the environmental light so there is no need for large texture dimensions.

That said, they are not a valid input of PMREMGenerator anyway so I think we can ignore them in this context.

Mugen87 commented 7 months ago

Creating a small map on the fly is useful when generating basic, soft gradient lighting.

@gkjohnson Given this use case, could you use a light probe instead? I have updated your fiddle so it generates a light probe based on your data texture.

https://jsfiddle.net/3gkevjLh/2/

WestLangley commented 7 months ago

The smallest equirectangular image supported by PMREMGenerator.fromEquirectangular() is 64 x 32 px.

The smallest cube map supported by PMREMGenerator.fromCubemap() is 16 x 16 px.

three.js r161

gkjohnson commented 7 months ago

@gkjohnson Given this use case, could you use a light probe instead? I have updated your fiddle so it generates a light probe based on your data texture.

This is primarily for use with three-gpu-pathtracer so a light probe doesn't work. I'll often generate a low-res procedural texture with a gradient or a couple hotspots, for example, to provide simple, soft ambient lighting. When using the same texture on scene.environment to render the rasterized fallback render, though, it just renders as black due to this issue. It's not a huge problem, just confusing and unexpected and not at all obvious. It would be nice if it worked as expected but I didn't think it would be complex to fix.

Documentation would be good in addition to a logged warning. Maybe I'll take a look at the issue when I have a bit of time.

Mugen87 commented 7 months ago

@WestLangley Would you like to make a PR that adds your text to the descriptions of fromEquirectangular() and fromCubemap()?

I'm undecided about the warning though since we normally do not sanitize inputs. If @WestLangley thinks it's worth in this particular instance, I'm okay with adding a check to the respective methods.

WestLangley commented 7 months ago

The PMREMGenerator.html document appears to be a bit out-of-date and should probably be updated for clarity. Perhaps the size limitations can be added there, too.