Cycles: Add Support for IES files as textures for light strength
Needs RevisionPublic

Authored by Lukas Stockner (lukasstockner97) on Oct 7 2015, 4:45 PM.
Tags
None
Tokens
"The World Burns" token, awarded by cardboard."Love" token, awarded by pisuke."Like" token, awarded by disnel."Love" token, awarded by duarteframos."Like" token, awarded by vklidu."Like" token, awarded by bliblubli."Like" token, awarded by sterlingroth."Love" token, awarded by marcog.

Details

Summary

This patch adds support for IES files, a file format that is commonly used to store the directional intensity distribution of light sources.
The new "IES Light" node is supposed to be plugged into the Strength input of the Emission node of the lamp.

Since people generating IES files do not really seem to care about the standard, the parser is flexible enough to accept all test files I have tried.
Some common weirdnesses are distributing values over multiple lines that should go into one line, using commas instead of spaces as delimiters and adding an END marker at the end of the file.

The user interface of the node is similar to the script node, the user can either select an internal Text or load a file.
Internally, IES files are handled similar to Image textures: They are stored by the ImageManager and each unique IES is assigned to one slot.

The local coordinate system of the lamp is used, so that the direction of the light can be changed. For UI reasons, it's usually best to add an area light,
rotate it and then change its type, since especially the point light does not immediately show its local coordinate system in the viewport.

Diff Detail

Lukas Stockner (lukasstockner97) retitled this revision from to Cycles: Add Support for IES files as textures for light strength.Oct 7 2015, 4:45 PM

I forgot to commit some changes. Sorry for the noise...

Sergey Sharybin (sergey) requested changes to this revision.Oct 7 2015, 6:45 PM

Some preliminary feedback for now. More review will happen after recovery from thesis pre-defense.

intern/cycles/blender/blender_shader.cpp
725

Guess we don't really need to try getting both of absolute path and text datablock context.

intern/cycles/blender/blender_util.h
342

get_text_datablock_content? text is a bit vague imo.

intern/cycles/kernel/kernel_shader.h
286

Think we'd rather always consider transform is always a full affine one. Doesn't cost that much more memory

intern/cycles/kernel/kernel_types.h
525

This is worrying, but would need some more time to look into.

intern/cycles/kernel/svm/svm.h
55

Not really sure about this. It's usually done as an explicit check. Either we change it all over the place or just don't bother.

intern/cycles/kernel/svm/svm_ies.h
27

Always use parenthesis.

34

Just split conditions by && instead of adding some weird "alignment" in the middle of the line.

intern/cycles/kernel/svm/svm_types.h
130

Comma

intern/cycles/render/image.h
78 ↗(On Diff #5125)

I'm really skeptical about this. To me it seems more logical if ies-specific stuff happens in util_iesor so and image manager is only managing actual bitmap textures. Meaning, if any sampling of the ies file is needed it's done outside of the image manager.

Or maybe just have IESManager.

In any case, don't make unmanageable monster out of image manager.

112 ↗(On Diff #5125)

This is really outside of the scope of image manager's job contract.

source/blender/blenkernel/intern/text.c
622

ELEM(node->type, SH_NODE_SCRIPT, NODE_FRAME, SH_NODE_IESLIGHT)

This revision now requires changes to proceed.Oct 7 2015, 6:45 PM

patch works really well. Best implementation I have seen until now in an open source renderer. UI-wise, I would add a new lamp type. I mean, actually point, hemi, spot and area lights are particular cases of what IES lights can be. For new users (and also old ones like me :D ), it's really not intuitive to make a point-light (some lights are very long or are big squares so far away from a point) and then add a texture to it that is plugged not in color like usual but strength. For an artists, it would be much better to have a IES light type.

I would like to test this new IES lamp
so wondering if there is a build available for win 64 ?

I have a lot of IES files and can test it

thanks for any feedback
happy bl

any minimum doc for this new patch
giving general data's for these IES specs ?

IES Std

there are 2 IES - one for US and one for EU

note: In the USA many large manufacturers of fixtures still have old IES std from 1990
so just hope it can be read too!

so just wondering which IES files can be read here!

Thanks
happy bl

Would be good to have it early in master to have a good testing before 2.77 :)

I finally got to updating this patch, the IES handling code is now split into an IESLight class (in util/) that handles parsing and some code in the ShaderManager that handles the IESLight slots (like the ImageManager did before).
Most of the first review points should be covered, the main issue that remains is the Light transform: The "Lamp primitive" seems strange, and the question remains how to store the Transform: IIRC, @Thomas Dinges (dingto) wanted to save more storage and @Sergey Sharybin (sergey) wanted to store both full transforms. Personally, I'd go with the full transforms: That's 128 additional bytes per lamp, compared to meshes, images and the BVH it's probably negligible...

I think you are right and this per light overhead is worth the price.

Thomas Dinges (dingto) requested changes to this revision.Nov 15 2015, 3:43 PM

Looks quite good already. I am fine with the additional memory usage per light. Some comments inline.

intern/cycles/kernel/kernel_textures.h
76

This will take away one more texture from sm_2x GPUs. Not so much of an issue, but we need to reduce the amount of max textures for these cards then. (see changes in svm_image.h and image.h in rB0ce3a755f83b)

intern/cycles/kernel/kernel_types.h
524

Not so happy with this approach either, other solutions?

This revision now requires changes to proceed.Nov 15 2015, 3:43 PM
Brecht Van Lommel (brecht) added inline comments.
intern/cycles/kernel/kernel_textures.h
76

The lookup_table texture can probably be used, check how other code uses scene->lookup_tables->add_table(...).

intern/cycles/kernel/kernel_types.h
524

For faster lamp MIS it would make sense to eventually include lamps in the BVH, so then they would be traceable. Still somewhat confusing now, but a lamp primitive is not a bad concept to have I think.

Some general questions:

  • Why IES was decided to be in the ShaderManager?
  • Don't see why can't we move to LUT for both SVM and OSL?
  • Perhaps it makes sense to add parsing facilities to the RNA API, similar to the PontDensity? So we don't need to re-implement this code for all the render engines (upcoming OpenGL PBR?). This is something where @Campbell Barton (campbellbarton) could have storng opinion as well.
intern/cycles/app/cycles_xml.cpp
519

Don't you miss readinf path to the IES file here or so?

intern/cycles/blender/blender_shader.cpp
728

Use mustage brackets here, plus consider adding assert(b_ies_node.mode() == BL::ShaderNodeIESLight::mode_INTERNAL) to the else branch.

intern/cycles/kernel/kernel_shader.h
291

Consider for the future: move it to the utility function like lamp_fetch_transform().

intern/cycles/kernel/kernel_textures.h
76

Yes, using lookup table will be the proper way to deal with this. Think i've mentioned it in the comment from a while ago..

intern/cycles/kernel/kernel_types.h
957

If IES table is stored in lookup_table we can remove this?

intern/cycles/kernel/osl/osl_services.cpp
915

This is something really weak, can't we interpolate LUT from a dedicated OSL node, similar to SVM?

intern/cycles/kernel/svm/svm_ies.h
24

This is more like a generig 2D LUT interpolation which will actually belong to table routines?

intern/cycles/render/light.h
47

We should start being more verbose on documentation. Mention of which axis is aligned with the light direction and which is an "up" direction wouldn't hurt here at all.

intern/cycles/render/nodes.cpp
1010

Split into two lines.

intern/cycles/render/shader.cpp
568

Why it's part of ShaderManager?

source/blender/makesdna/DNA_node_types.h
879

char filepath[1024]; /* 1024 = FILE_MAX */ so it's simplier to nail all the filepaths down when/if we'll be ding extra filepath length bump.

I'm working on improving this patch right now, but I've got no idea how to improve the OSL handling. I could duplicate the interpolation into OSL, sure, but in the end it will always need a way to access the LUT data and therefore need a way to call code "outside" of OSL.
However, I don't see a way to to implement this without using the texture name hack. Also, considering that it's used already for image slots, there probably is no better way to implement this.

So, I'd be open to suggestions here, but I don't really expect that there's a better way to do it...

Ok, will have a look of how to make it all nice for OSL.

could someone please revise it, i have no problems using it in experimental build & would be very welcomed if it was included in the 2.79 release

Hi everyone.
In my opinion this should be a standard feature. Not only ies but also ldt.
And should go, if possible, in a nightly build.
Is there some way to test it?
can't complie it succesfully on debian9.