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

SpotLightMap2 #20290

Closed
wants to merge 6 commits into from
Closed

SpotLightMap2 #20290

wants to merge 6 commits into from

Conversation

mbredif
Copy link
Contributor

@mbredif mbredif commented Sep 8, 2020

rebased version of the original PR #14621

@mbredif
Copy link
Contributor Author

mbredif commented Sep 8, 2020

Should I run npm run make-screenshot webgl_shadowmap_viewer and push the new screenshot ?

@Mugen87
Copy link
Collaborator

Mugen87 commented Sep 8, 2020

Yes that would be great!

@mbredif
Copy link
Contributor Author

mbredif commented Sep 9, 2020

all clear, ready for review !

@mbredif
Copy link
Contributor Author

mbredif commented Sep 17, 2020

@mrdoob, should it be milestoned to r121 ? (#14621 was)

@WestLangley WestLangley added this to the r121 milestone Sep 18, 2020

var spotLight = new THREE.SpotLight( 0xffffff );
spotLight.position.set( 100, 1000, 100 );
spotLight.map = new THREE.TextureLoader().load(textureUrl);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would not include the usage of map in the basic spot light example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's an editorial call, I'll remove it if needed

@mrdoob mrdoob modified the milestones: r121, r122 Sep 28, 2020
@mbredif
Copy link
Contributor Author

mbredif commented Oct 1, 2020

Ok now the spot light maps shoud work with or without shadow mapping !

@mbredif mbredif closed this Oct 2, 2020
@mbredif mbredif reopened this Oct 2, 2020
@mbredif
Copy link
Contributor Author

mbredif commented Oct 2, 2020

That is weird, I cannot reproduce the test errors on my computer, neither visually nor with npm run test-e2e, which gives me yet another set of failing examples (!).

@Mugen87
Copy link
Collaborator

Mugen87 commented Oct 2, 2020

I've downloaded your latest code and tested one of the failing examples (webgl_clipping). It produces the following shader compilation error.

WebGLProgram.js:749 THREE.WebGLProgram: shader error: 0 35715 false gl.getProgramInfoLog invalid shaders THREE.WebGLShader: gl.getShaderInfoLog() fragment
ERROR: 0:1058: '#' : unexpected token after conditional expression
ERROR: 0:1059: '#' : unexpected token after conditional expression
ERROR: 0:1060: '#' : unexpected token after conditional expression
ERROR: 0:1062: 'LIGHT_MAP_INDEX' : unexpected token after conditional expression
ERROR: 0:1063: 'LIGHT_MAP_INDEX' : undeclared identifier
ERROR: 0:1063: '[]' : integer expression required
ERROR: 0:1063: 'LIGHT_MAP_INDEX' : undeclared identifier
ERROR: 0:1063: '[]' : integer expression required
ERROR: 0:1065: 'spotLightMap' : undeclared identifier
ERROR: 0:1065: 'LIGHT_MAP_INDEX' : undeclared identifier
ERROR: 0:1065: '[]' : integer expression required
ERROR: 0:1065: 'expression' : left of '[' is not of type array, matrix, or vector
ERROR: 0:1065: 'texture' : no matching overloaded function found
ERROR: 0:1065: '=' : dimension mismatch
ERROR: 0:1065: 'assign' : cannot convert from 'const mediump float' to 'highp 4-component vector of float'

@mbredif
Copy link
Contributor Author

mbredif commented Oct 2, 2020

Ok so, the culprit is here

On my computer (windows/chrome), the preprocessor is ok with it. I could #undef LIGHT_MAP_INDEX at each iteration, but that will not solve the issue. The issue is that I need to compute the light map index at compile time so that I can point to the right sampler in the light map sampler array.

@mbredif
Copy link
Contributor Author

mbredif commented Oct 2, 2020

Ok, tests are fixed.
for the record, the issue was some glsl preprocessing that was badly translating // comments (/* */ comments are fine)

#if (1 > 0) // comment
#define D
#endif

turned into

#if (1 > 0) #define D
#endif

Thus, it chocked on #define which was not at the beginning of a line.

@DagerD

This comment has been minimized.

@mrdoob mrdoob removed this from the r122 milestone Oct 28, 2020
@mbredif
Copy link
Contributor Author

mbredif commented Nov 16, 2020

@carstenschwede I've reviewed this PR multiple times and I think it should be mostly fine. The only thing I'm not sure is whether the resulting complexity in WebGLLights and especially the shader code is acceptable. However, this might be something which can be refactored in the future.

Thanks @Mugen87 for your reviews! I agree this is a bit more complicated than I expected, but that was the simplest solution I found to support the 4 cases (with/without shadowmap and with/without texturemap) with no big refactoring and with minimal impact on the shadowmap code path.

@mrdoob
Copy link
Owner

mrdoob commented Nov 21, 2020

Spent some time reviewing this and I'm not sure adding a map to SpotLight is the right approach. I think it makes the internal code that handles spotlights too complicated, and it's also limited by it (square shape).

How about doing a TextureLight?

const light = new THREE.TextureLight( texture );
light.setSize( 2, 1 ); // We can add a method to make sure the frustum matches the texture size.
scene.add( light );

I think having a different type of light will make the implementation more readable.

@mbredif What do you think?

@mbredif
Copy link
Contributor Author

mbredif commented Nov 23, 2020

it's also limited by it (square shape).

Not exactly, the aspect ratio of the projected texture is not fixed to 1.
Instead, it matches the aspect ratio of the shadow map, which you can tune using spotLight.shadow.mapSize.width and spotLight.shadow.mapSize.height . But I agree that this could be improved.

The complication comes from supporting both an optional texture and an optional shadowmap (4 cases).
I agree a light that would support only a texture (required) and no shadowmap support would be simpler, but could not be used as a light projector as light would shine on occluded surfaces as well...

A tradeoff could be to introduce a TextureLight with optional shadowmap support. I expect some redundance (a lot?) with SpotLight but the code would be simpler (mostly in the shader : one extra for loop for TextureLights instead of a larger for loop with index checks to distinguish the 4 spotlight cases).

Also the TextureLight could be slightly simpler by not supporting the circular spot lighting defined by (angle, penumbra...)

On a related note, have you ever thought about a ShaderLight, that could be tuned by advanced users, similar to (Raw)ShaderMaterial is offered for advanced Material users ?

@mrdoob
Copy link
Owner

mrdoob commented Nov 24, 2020

A tradeoff could be to introduce a TextureLight with optional shadowmap support. I expect some redundance (a lot?) with SpotLight but the code would be simpler (mostly in the shader : one extra for loop for TextureLights instead of a larger for loop with index checks to distinguish the 4 spotlight cases).

Also the TextureLight could be slightly simpler by not supporting the circular spot lighting defined by (angle, penumbra...)

That's what I'm proposing yep!

On a related note, have you ever thought about a ShaderLight, that could be tuned by advanced users, similar to (Raw)ShaderMaterial is offered for advanced Material users ?

Interesting!

@mrdoob mrdoob modified the milestones: r123, r124 Nov 25, 2020
@gkjohnson
Copy link
Collaborator

new THREE.TextureLight( texture );

This is called a "Projector" in Unity, I think, if you're interested in a more consistent / intuitive name. In Unity it can be used to multiple the surface color by the texture (create custom shadows from textures) or additively for lighting. It also supports orthographic projection as well as attenuated projection (though those features can come later if needed).

On a related note, have you ever thought about a ShaderLight, that could be tuned by advanced users, similar to (Raw)ShaderMaterial is offered for advanced Material users?

You should be able to achieve this by updating a RenderTarget passed into a TextureLight, right?

This looks great! Looking forward to this feature.

@Mugen87
Copy link
Collaborator

Mugen87 commented Nov 26, 2020

This is called a "Projector" in Unity, I think, if you're interested in a more consistent / intuitive name.

We can't use Projector as a name, see #13057 (comment).

@gkjohnson
Copy link
Collaborator

We can't use Projector as a name, see #13057 (comment).

Oh I see -- just took a look at Projector.js and as far as I can tell it's just used in SVGRenderer and is pretty undocumented, right? It seems like it could be pretty safely renamed to something like SVGRendererProjector or similar if we felt that Projector was a better name for this kind of light. Otherwise ProjectorLight might be an option, too. It just seemed more intuitive to me.

@mrdoob mrdoob modified the milestones: r124, r125 Dec 24, 2020
@vade
Copy link

vade commented Jan 10, 2021

Hi, this functionality is super awesome. Thank you. Apologies for being 'that guy' - as im unfamiliar with the release cycles of three.js - is this in good enough shape to start using off of this branch? if not, is this expected to be rolled into dev / master for a release?

Thank you!

@mrdoob mrdoob modified the milestones: r125, r126 Jan 27, 2021
@RobertoAlda
Copy link

RobertoAlda commented Jan 28, 2021

Hello everybody,
I'm usinng this cool extension to project textures in threejs, but i think i've found a issue.
I need an accurate color management so i followed the Don McCurdy guide (https://www.donmccurdy.com/2020/06/17/color-management-in-threejs/):

  • textures on materials use THREE.sRGBEncoding
  • textures on spotlightmap use THREE.LinearEncoding
  • WebGLRenderer outputEncoding is THREE.sRGBEncoding

The texture on objects are ok, but the textures projected by spotlight have some artifacts around the white pixel points and colors are brighter.
I set the camera in order to view object fullscreen to see in full details mode.

Original file texture:
https://drive.google.com/file/d/195ZJx456IoGzICUhv6Gr0wNL7ZdMLjet/view?usp=sharing

Texture loaded on material of mesh:
https://drive.google.com/file/d/1aEZEUCzUPIdGqG5zgjKyd1VIuJFY9QHE/view?usp=sharing

Texture projected on mesh:
https://drive.google.com/file/d/1gVq1ugVmB44_8BB0j1EL5v5AhcHQmB20/view

Any advise to solve these issue?
Thank you very much.

@mrdoob mrdoob modified the milestones: r126, r127 Feb 23, 2021
@mrdoob mrdoob modified the milestones: r127, r128 Mar 30, 2021
@mrdoob mrdoob modified the milestones: r128, r129 Apr 23, 2021
@mrdoob mrdoob modified the milestones: r129, r130 May 27, 2021
@mbredif
Copy link
Contributor Author

mbredif commented Jun 2, 2021

@RobertoAlda, there might be other issues, but the spotlight effect sure does not help by modulating the mapped color.

directLight.color *= spotEffect * punctualLightIntensityToIrradianceFactor( lightDistance, spotLight.distance, spotLight.decay );
.

That goes towards the hint from @mrdoob to remove the spot light falloff from this and rework this PR as a TextureLight...
@mrdoob, what do you think ? is that what is blocking this PR ?

@mrdoob
Copy link
Owner

mrdoob commented Jun 2, 2021

@mrdoob, what do you think ? is that what is blocking this PR ?

At this point, the main blocker are the conflicting files. Any chance you could clean these up?

@mbredif
Copy link
Contributor Author

mbredif commented Jun 2, 2021

At this point, the main blocker are the conflicting files. Any chance you could clean these up?

I gave it a try :
dev...mbredif:spotLightMap3

@mbredif mbredif mentioned this pull request Jun 2, 2021
@Mugen87
Copy link
Collaborator

Mugen87 commented Jun 2, 2021

Closing in favor of #21944.

@Mugen87 Mugen87 closed this Jun 2, 2021
@Mugen87 Mugen87 removed this from the r130 milestone Jun 2, 2021
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.