Blender Internal: Add "Lamp Data" shader node that allows shaders to acquire information such as light vector from specified Lamp.
ClosedPublic
Actions

Authored by Shinsuke Irie (irie) on Nov 21 2013, 9:20 AM.

Details

Reviewers

Brecht Van Lommel (brecht)

Summary

For now this provides the following outputs:

Color
Light Vector
Distance
Shadow
Visibility Factor

Note that Color output is multiplied by the lamp energy. Multiplication of
color*max(dot(light_vector,normal_vector),0)*shadow*visibility_factor
produces the exact same result as the Lambert shader.

Example: node_shader_lamp_test.blend

Diff Detail

Event Timeline

Can you explain what the rationale is for this node?

I think linking to a specific lamp in a material is not a good idea, you want to be able to easily add and remove lamps in a scene without having to change all the materials too. This seems like a way to allow users to implement their own BSDF, but wouldn't it better to find a more general way to do that so that it can somehow do it in a loop over all influencing lamps?

The other thing is, this is kind of a design decision that I find difficult to take because I'm not really developing Blender Internal anymore. I do bugfixes and some smaller tweaks but don't set big design directions (no one is actually). If I was doing that it would be a definite "no, we need a more general solution than linking to individual lamps". But if other people are going to be working on this then perhaps they need the freedom to take their own decisions.

Brecht Van Lommel (brecht) requested changes to this revision.Nov 21 2013, 6:09 PM

In NPR (especially Japanese anime), many character rigs have one or multiple lamps as their children, to make good specular on the hair, to avoid unwanted shadows on the face, and so on. Of course such "local lamps" shouldn't affect the other characters. However, to do that using exsisting features such as light groups, we have to make a lot of light groups, and are perhaps annoyed with lack of the render layers, and viewport preview goes wrong. If using the "Lamp Data" node, we don't need complicated lamp settings anymore. In addition, we still can use global lamp setting if all the lamp objects link to the same lamp.

In my personal communications, several Blender users indeed want shader nodes to provide the way to obtain light vector. Also, we can easily find the similar users on the internet. For example:

http://wiki.blender.org/index.php/Dev:Source/Node_Editor/Materials#Shader_Nodes
http://www.elysiun.com/forum/showthread.php?290829-Blender-Extended-Expressive-Rendering-%28BEER%29-Extending-Blender-Internal&p=2394687&viewfull=1#post2394687
http://blenderthings.blogspot.jp/2012/11/getting-light-vector-in-osl-for-blender.html

I think the Blender Internal is too poor to do NPR though Freestyle works only with it, so still needs more active developing.

I understand that you want to have some way to do custom shaders/BSDFs, but if there was some more general way to do that for all lights influencing a material, then light groups would work just fine right?

From production experience, I think it is very important to have the shader nodes decoupled from specific lamp objects. Otherwise it becomes difficult to link in a character into various scenes, you'd need to modify the material for each scene to point to the right lamps.

I think, in most cases, the materials linking to "local lamps" via the lamp node are not needed to modify when linking to various scenes. Even if using the lamp node for global scene lamp, we can avoid to modify all materials if the materials share the same node group including the lamp node.

Example 2: node_shader_lamp_test2.blend

In this file, editing "Global Lamp" node group affects all materials, so we can change the link to the scene lamp easily.

There is another reason that this shader node is needed. The standard shaders such as "Lambert" discards the information of shaded area where dot product of normal vector and light vector is negative, so we cannot make shading on such shaded area. If using the lamp node, we can do that.

Example 3: node_shader_lamp_test3.blend

AFAIK, there is no way to do that only with existing features.

I can accept this functionality on the condition that:

You become part of the render module team, and do maintenance and bugfixing for these shading system features.
If you add features or big fixes, do code review with other render module members (actually mostly me at the moment because ton doesn't have time for coding much)

The linking system as it exists does not allow you to create a local lamp group per scene file as far as I know, though with python trickery some things are possible. I do think a more general solution should be available eventually where you can loop over lamps in the nodes, but I think we can have the node roughly as is and perhaps generalize it later.

Still need to review the actual code, will try to do that soon.

Initial code review, might find more stuff later.

source/blender/gpu/GPU_material.h
248	Consistency: if you are going to call this the Lamp Data node you could also use the name "lamp_get_data" here and in other places, and name the arguments similar to the outputs of the nodes
source/blender/gpu/intern/gpu_material.c
1425–1433	I don't fully understand what is going on here, but it seems like this code is in the wrong place. I'm guessing it is because there is some case where lamps are not getting initialized for the Lamp Data node, but then this code does not actually solve the problem, but only when you have a shader node setup with a material node with a shadeless material in it.
1975	Probably you want to allow texturing the light color here with shade_light_textures, sample as in shade_one_light. Also missing here seems to be lamp energy?
source/blender/render/intern/source/shadeoutput.c
1986	Inline is not a valid keyword in C. I wouldn't really use that here either, optimizing compilers should be smart enough to decide this kind of thing.
2009–2012	The shade 4 value vector is a bit strange, you probably do not want to expose like this, as if the 4th channel is alpha. I can't quite remember what it is but I think you could multiply the 4th channel into the other channels or something like this, and leave the alpha component to 1.0 always. The results that you want is probably that `color * shadow * visibility_factor * dot(N, light_vector)` gives you the same exact shading results as a simple diffuse shader with a regular blender material?
2020	I think this function should have a RE_ prefix. There's a few others that don't but they should actually have it as well, just to avoid naming conflicts and make it clear which module this function is in.
2022	If this is not true there will be uninitialized values. It's a rare case as you already check for the right object type when assigning the object to the node, but with objects linked from other files the types can change so this check is needed, just need to ensure the values are zeroed then.
2035	This seems like some kind of debug code left in? Not sure why there is a hardcoded name here.

OK, I'd like to become a module member and fix bugs in this area if any.

Many thanks for the review. I'll update the patch as soon as possible.

source/blender/gpu/GPU_material.h
248	Initially I planned to use a node name "Lamp Info", and the old name remained as function names. The argument "shadfac" is not same as node output "shadow". The former is a scalar, while the latter is a 4 dimensional vector corresponding to the shadow color, so I think the different names are reasonable here.
source/blender/gpu/intern/gpu_material.c
1425–1433	Probably you are right. For shadeless materials this node often fails to output the correct values. Will look into this more carefully.
1975	This is for variance shadow maps in the gameengine. So far it seems to work correctly. The lamp color is already multiplied by the lamp energy in GPU_lamp_update_colors().
source/blender/render/intern/source/shadeoutput.c
1986	Ah sorry, I forgot to remove this "inline" keyword. Anyway, this function is too large to be inline.
2009–2012	The calculations in this part are exactly same as "Shadows Only" materials. lamp_get_shadow() returns the shadow color as RGB channels and the light absorption as alpha channel. The RGB channels were already multiplied by the 4th channel here, so the alpha channel simply can be set 1.0.
2020	I see, will add the RE_ prefix.
2022	Will change it to set values anyway, though the uninitialized values shouldn't cause serious problems.
2035	Checking lamp name is same as preview_prepare_scene() in render_preview.c:365. "Lamp.002" is used for searching main key light of the material preview. Is there better way to do that?

Brecht Van Lommel (brecht) added inline comments.Nov 22 2013, 10:11 PM

source/blender/render/intern/source/shadeoutput.c
2009–2012	Ok then. In case you haven't it's probably a good idea still to verify that multiplying all those things together gives you the same result as a perfect white diffuse shader, to verify if it's all correct.
2031	Don't forget to set this one to 1 as well.
2035	Ah ok. Maybe add a comment to indicate that that's what this does.

Shinsuke Irie (irie) added inline comments.Nov 23 2013, 5:42 AM

source/blender/gpu/intern/gpu_material.c
1425–1433	Ah sorry, the failures happened only when I removed this code for testing. So far this works as I expect. Anyway I'll investigate if there is better way.
source/blender/render/intern/source/shadeoutput.c
2009–2012	I confirmed the result is exactly same as Lambert shader when the diffuse color is 100% white and the ray bias is disabled.

I believe that all issues in the previous patch have been resolved.
Also, shadow color support and light/shadow textures support were added.

Code changes look good generally but need to test using it still to verify if it all works ok.

source/blender/render/intern/source/shadeoutput.c
70	I prefer initialized to zero at the start and return instead of goto.

Looks ok to commit now when you change the goto.

We are trying to be pretty strict about not allowing any new feature commits without documentation. So before the commit, the node should be documented here:
http://wiki.blender.org/index.php/Doc:2.6/Manual/Materials/Nodes/Types/Input

And added to the release notes here:
http://wiki.blender.org/index.php/Dev:Ref/Release_Notes/2.70

Thanks!

Committed in rBab9822eff8865846d3c7ef81ff30cc35cb48ae0c.