three.js icon indicating copy to clipboard operation
three.js copied to clipboard

Adding IES profile support to spot lights.

Open richardassar opened this issue 3 years ago • 6 comments

Implements the feature requested in https://github.com/mrdoob/three.js/issues/9624

  • For demonstration/testing purposes I've just modified examples/webgl_lights_spotlight, I'll pull this out into its own example before we merge this PR.
  • There are other uses of getSpotLightInfo that have still to be brought in line with these changes.
  • Currently using the IESLoader that I updated, tidied and integrated into gkjohnson/three-gpu-pathtracer as part of the spot light IES support there.
  • Using gkjohnson's gist for the profile data itself, feel free to create some unique to three.js
  • The profiles can (rarely) be two dimensional but this is ignored as it's a very rare case, I can add support for this if necessary.

Here's how it looks:

Screenshot from 2022-07-30 16-34-04

Screenshot from 2022-07-30 16-33-09

Please review and I'll respond with changes accordingly.

richardassar avatar Jul 30 '22 16:07 richardassar

I don't think this approach ("integrating" IES profiles into core) is good... ~~I think what was meant by @bhouston is rather loading IES profiles by parsing them and then creating lights using three.js' lights' own constructor parameters (i.e. intensity/distance/so on).~~ An alternative approach could be creating a IESLight class which lights will IESLoader return.

LeviPesin avatar Jul 30 '22 17:07 LeviPesin

I think what was meant by @bhouston is rather loading IES profiles by parsing them and then creating lights using three.js' lights' own constructor parameters (i.e. intensity/distance/so on).

IES profiles cannot be replicated by just adjusting the existing constructor parameters alone for spot lights

gkjohnson avatar Jul 30 '22 17:07 gkjohnson

Then I think the second suggestion is better. And I think IESLight and IESLoader would better go in examples rather than core...

LeviPesin avatar Jul 30 '22 18:07 LeviPesin

And I think IESLight and IESLoader would better go in examples rather than core...

If you look at the code it requires core shader changes...

gkjohnson avatar Jul 30 '22 18:07 gkjohnson

I don't think this approach ("integrating" IES profiles into core) is good... ~I think what was meant by @bhouston is rather loading IES profiles by parsing them and then creating lights using three.js' lights' own constructor parameters (i.e. intensity/distance/so on).~ An alternative approach could be creating a IESLight class which lights will IESLoader return.

I agree that if this is also required on point lights (although the distinction is artificial between point and spot lights with IES profiles) then a common base class IESLight which IESPointLight and IESSpotLight derive from seems like an acceptable solution.

UnrealEngine does allow one to set profiles on most light types, but again there's little difference except in the case of their "Rect Light" which just masks the profile leaving only one hemisphere. (Edit: and they opt to apply this masking to point lights also)

The optional extra parameter to SpotLight seems unintrusive, but if there is consensus to refactor this as described above I'm happy to do so.

richardassar avatar Jul 30 '22 19:07 richardassar

I've added IESTexture which accepts a single or array of IES profile URLs as an input to the constructor or the add method. Profiles can be added at creation time or later and needsUpdate will be flagged when appropriate.

As before the example remains in webgl_lights_spotlight.html (lights / spotlight). I will move this to it's own example and restore the original before we merge.

Screenshot from 2022-09-13 17-46-49

Example screenshot showing four spotlights. One has no IES profile assigned and the others are assigned IES profiles, two of which are loaded when IESTexture is created and one of which is loaded with iesProfilesTexture.add.

richardassar avatar Sep 13 '22 16:09 richardassar

@sunag pulled some of these changes into https://github.com/mrdoob/three.js/pull/25238

Ping @gkjohnson

Would it be possible to get a review on this?

I'd like to get #9624 closed, thanks.

richardassar avatar Jan 06 '23 16:01 richardassar

I just think it would be easier to merge if we could do it based on Nodes for WebGL as well, that seems closer to where things are going, in which case we should start supporting LightNode in WebGL as well. Things like atlas/caches can be done using a Node system in more intuitive ways.

sunag avatar Jan 09 '23 12:01 sunag

I wasn't aware of any merge difficulties, unless dev has diverged a lot since I created this PR.

Does it make sense to block this feature pending a node-based implementation?

The changes to existing code are relatively minimal for a feature that some might find useful, unless all new features are to be node-based this could be refactored as a separate task.

richardassar avatar Jan 09 '23 15:01 richardassar

@sunag

I just think it would be easier to merge if we could do it based on Nodes for WebGL as well, that seems closer to where things are going, in which case we should start supporting LightNode in WebGL as well.

Agreed. Sounds like it'll be better to add this to the Nodes code instead.

mrdoob avatar Jan 19 '23 03:01 mrdoob

@mrdoob @sunag Thanks, I'll adapt this into a Nodes implementation soon when I have some time.

richardassar avatar Jan 21 '23 17:01 richardassar

Hello there, I have noticed a few issues with how the IES lights are implemented right now:

  • in the IESLoader, the intensity is accounted for twice
  • in the IESLoader, the symmetries are not accounted for correctly
  • in the IESLoader, only type C is supported
  • one of the default IES files in the webgl_lights_spotlight.html example does not follow the specifications of the IES format
  • only the first column is considered, making all the spot lights symmetric
  • why not use the map attributes to store the complete data and adress it properly using the inclination and azimuth of the ray direction ?

I could not figure out how to make the proper modifications to the three.js project so I have implemented a ray marcher with a better IESLoader : https://asliceofcuriosity.fr/assets/ray-marching-ies/webgl_raymarching_ies.html I have dealt with the symmetries but not with types A and B yet.

I have made a github project with the source code: https://github.com/Wagyx/ray-marching-ies

Wagyx avatar Oct 11 '23 19:10 Wagyx

As mentioned in previous comments, it's best to reimplement this feature with the nodes system for WebGPURenderer and with a fresh PR. The new PR can also honor the feedback from @Wagyx.

Mugen87 avatar Oct 19 '23 12:10 Mugen87

it's best to reimplement this feature with the nodes system for WebGPURenderer and with a fresh PR

I believe it's already been addd in #25238. @Wagyx it would be worth checking out that implementation and making a PR if you see issues.

gkjohnson avatar Oct 22 '23 11:10 gkjohnson