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

Textured lighting as a spotLight.map property #14621

Closed
wants to merge 3 commits into from

Conversation

mbredif
Copy link
Contributor

@mbredif mbredif commented Aug 2, 2018

This PR is a second attempt at #13057, by adding a colorMap map property to the SpotLight class :

image

http://localhost:8000/examples/webgl_shadowmap_viewer.html

This PR generalizes the single-channel cookie concept to a 4-channel texture : the directLight.color of the spot light is mixed with the rgb value of the colorMap lookup with a ratio corresponding to its alpha value : mix( directLight.color, spotColor.rgb, spotColor.a ). The cookie effect is reproduced using pixel values (0,0,0,1-cookie_value).

Comments :

  • with this PR, each spotLight takes an extra texture slot (similar to Implement Projectors to support textured lighting #13057 (comment)). Is it an issue ? any idea ? (the second commit 8d683b8 addresses this issue.)
  • it is not currently possible to control the geometry of the spotlight to match a known PerspectiveCamera (the spotShadowMatrix is used for texture lookup). This will be tackled in a follow-up PR if this one gets merged.

@looeee
Copy link
Collaborator

looeee commented Aug 2, 2018

Live example

Copy link
Contributor

@haroldiedema haroldiedema left a comment

Choose a reason for hiding this comment

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

Build files should not be part of the PR.

@mbredif
Copy link
Contributor Author

mbredif commented Aug 3, 2018

good to know !
Rebased on three.js/dev without the build files.

@mbredif mbredif force-pushed the spotColorMap branch 3 times, most recently from 7dfc774 to 8d683b8 Compare August 3, 2018 16:23
@mbredif
Copy link
Contributor Author

mbredif commented Aug 3, 2018

(Rebased on dev after shadowmap/fog was fixed by #14617)

The second commit ( 5ccf289 ) adds :

  • documentation, including an extended doc example (including a video texture) with switchable textures/video. When no texture is selected, no extra texture slot is used.
  • examples/webgl_shadowmap_viewer is still working, but takes one less texture slot (as one of the two spotlights has no map)
  • SpotLight.colorMap is renamed to SpotLight.map for simplicity, but I can change it back if requested.

This PR is now ready for review.

@mbredif mbredif force-pushed the spotColorMap branch 4 times, most recently from fd5282c to 5ccf289 Compare August 3, 2018 21:11
@mbredif mbredif changed the title Textured lighting as a spotLight.colorMap property Textured lighting as a spotLight.map property Aug 3, 2018
@mbredif
Copy link
Contributor Author

mbredif commented Oct 4, 2018

@mrdoob @WestLangley , any feedback on this long awaited feature ?

@WestLangley
Copy link
Collaborator

I definitely support this feature.

I am sure working, live links would be helpful to @mrdoob.

@mbredif
Copy link
Contributor Author

mbredif commented Oct 5, 2018

I am sure working, live links would be helpful to @mrdoob.

sure !

@danielgranat
Copy link

@mbredif I think this can be a great addition to the Threejs. At my company, we are trying to use this branch, and being a noob with shaders, how would you handle rotation of the texture (maybe use Texture.rotation?
Is it possible to change the alpha so the image isn't transparent?

@mbredif
Copy link
Contributor Author

mbredif commented May 2, 2019

Texture.matrix and its defining parameters (such as Texture.rotation) is not yet supported in this PR.
I see 2 solutions :

Please file a PR against mine if you go for the second option ! :)

@danielgranat
Copy link

Thank you! I will try the second option.

@WestLangley
Copy link
Collaborator

This was an excellent contribution by @mbredif, IMO.

I am disappointed it has not received the attention it deserves.

@marcofugaro
Copy link
Contributor

So sad this was never implemented, would have been awesome

@mbredif
Copy link
Contributor Author

mbredif commented Oct 14, 2019

Let me correct : textured spotlights ARE implemented in this PR. However it has not been merged yet...
I do not know what s blocking it though.

@Mugen87
Copy link
Collaborator

Mugen87 commented Oct 14, 2019

Sorry, this PR has lacked attention in the last month. I would also like to see this feature merged!

However, I would only put effort in resolving the merge conflicts if @mrdoob is fine with this. Maybe he wants to head in another direction in context of textured lighting.

BTW: I think one spotlight example is sufficient to demonstrate this new feature. Hence, I would undo the changes to webgl_shadowmap_viewer. There is also one issue in webgl_lights_spotlight example. If I switch to the video texture and then to a different one, the video is still playing in the background.

@dlawrence2060
Copy link

Any idea if this will get merged soon?

@mbredif
Copy link
Contributor Author

mbredif commented Jan 23, 2020

Not my call unfortunately... I can update the PR if I get a green light that this is very likely to be merged by a maintainer @mrdoob @WestLangley @Mugen87

@WestLangley
Copy link
Collaborator

@mbredif At this point, you should get buy-in from @mrdoob before investing your time.

@mrdoob Will you please render a decision?

@WestLangley WestLangley added this to the r117 milestone May 4, 2020
@mrdoob mrdoob modified the milestones: r117, r118 May 27, 2020
@mrdoob mrdoob modified the milestones: r118, r119 Jun 24, 2020
@carstenschwede
Copy link
Contributor

@mbredif Not sure if having a milestone assigned can be considered "likely to be merged", but I would love to see this PR updated to current dev. Really nice work!

@mbredif
Copy link
Contributor Author

mbredif commented Jul 21, 2020

@mrdoob I did not see you attached a milestone. Is it a green light that says that a rebase is likely to be merged ?

@mrdoob
Copy link
Owner

mrdoob commented Jul 27, 2020

Yes! Sorry for the... 2 years late review... 🙁

I think the (This PR is the geometric counterpart of #14621) comment in #14658 made me think that the PRs were entangled and I never knew which one to start reading first 🙃

Looks good to me! Any chance you can rebase/resolve the conflicts?

@mrdoob mrdoob modified the milestones: r119, r120 Jul 29, 2020
@mrdoob mrdoob modified the milestones: r120, r121 Aug 23, 2020
@trbll
Copy link

trbll commented Aug 28, 2020

Did this not get added in r120 as planned? Looks great and am really looking forward to having this support built in. Thanks!

@carstenschwede
Copy link
Contributor

Friendly pinging @mbredif cause I am not sure he receives notifications.

@mrdoob
Copy link
Owner

mrdoob commented Sep 1, 2020

@trbll The PR has conflicts...

@trbll
Copy link

trbll commented Sep 1, 2020

@trbll The PR has conflicts...

@mrdoob Yeah, I see that now. Thank you and sorry about that. Just got going too fast in anticipation after I saw r120 had dropped. Appreciate all that you do!

@mbredif
Copy link
Contributor Author

mbredif commented Sep 2, 2020

Sorry for the delay, I started rebasing this very old PR but it had a lot of conflicts due to the reformating of the glsl code. I was too busy to polish it but I am quite there.

@mbredif
Copy link
Contributor Author

mbredif commented Sep 4, 2020

I got it working again : dev...mbredif:spotLightMap2
Should I file a new PR or just push force on this one ?

@mrdoob
Copy link
Owner

mrdoob commented Sep 4, 2020

Whatever is easier for you 👍

@mbredif mbredif mentioned this pull request Sep 8, 2020
@mbredif
Copy link
Contributor Author

mbredif commented Sep 8, 2020

rebased version of this PR is available at #20290

@mbredif mbredif closed this Sep 8, 2020
@mrdoob mrdoob removed this from the r121 milestone Sep 8, 2020
@mrdoob mrdoob mentioned this pull request Aug 24, 2022
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.