godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
91.18k stars 21.2k forks source link

3D Light shader doesn't work as it should #37668

Closed 2plus2makes5 closed 4 years ago

2plus2makes5 commented 4 years ago

Godot version: 3.2.1

OS/device including version: Windows 10

Issue description: I wanted to make a custom toon shader and it works with a single light, but if there are more lights their interaction is wrong. This problem isn't limited to a toon effect, it's a bug of the light shader itself.

Steps to reproduce: Put a colored ball between two lights and use this simple shader with the light function copied from the documentation here: https://docs.godotengine.org/en/latest/tutorials/shading/shading_reference/spatial_shader.html

shader_type spatial;
render_mode blend_mix,depth_draw_opaque,cull_back,diffuse_burley,specular_schlick_ggx;
uniform vec4 albedo : hint_color=vec4(1.0);

void fragment() 
{
    ALBEDO = albedo.rgb;
}

void light()
{
    DIFFUSE_LIGHT += dot(NORMAL, LIGHT) * ATTENUATION * ALBEDO;
}

Then move the lights and you will see that the lights don't add but it seems more like that one "prevails" over the other when they affect the same areas.

errore

The image on the left seems right(but it isn't), the one on the right shows what happens if you make one light closer, if you make it even closer the ball is completely black.

Minimal reproduction project: Just make a new scene and follow the instrunctions in "steps to reproduce".

guilhermefelipecgs commented 4 years ago

If you are trying to create a toon shader, this DIFFUSE_LIGHT += dot(NORMAL, LIGHT) * ATTENUATION * ALBEDO; is not the right way.

I suggest to you look at this example https://github.com/EXPWorlds/Godot-Cel-Shader/blob/master/Shaders/cel.shader.

To enable multiple lights on the example shader, just change this line DIFFUSE_LIGHT = diffuse; to this DIFFUSE_LIGHT += diffuse;

guilhermefelipecgs commented 4 years ago

If you want to correct your example to create a standard light behavior, do the following: DIFFUSE_LIGHT += max(dot(NORMAL, LIGHT), 0.0) * ATTENUATION * ALBEDO;

2plus2makes5 commented 4 years ago

@guilhermefelipecgs 1) about the toon shader, you are right, with that line it seems to work right! :D 2) about the standard light, i copied that light function from the documentation, so if that's wrong then the documentation is wrong. Your function gives better results but not 100% correct as you can see here: error2

This issue was initially mentioned in this proposal for the light function improvement: https://github.com/godotengine/godot-proposals/issues/675

clayjohn commented 4 years ago

After looking @guilhermefelipecgs's comments, it appears this is not a bug, but rather a misunderstanding. Light shaders allow you to override the standard light calculations and substitute your own custom light calculations. It is up to you to decide what those functions look like.

In this case, the interaction of light was not "wrong". The interaction of light was exactly as you coded it. The problem is that what you coded and what you expected were two different things. That is not a bug with Godot.

As for the documentation, the documentation is not saying "here is a line of code that will make perfect lights for your game" the line says "here is what the syntax looks like for making your own custom light"

2plus2makes5 commented 4 years ago

@clayjohn The documentation says:

"Below is an example of a custom light function using a Lambertian lighting model"

now i understand i was wrong expecting a similar result as the diffuse_lambert render_mode, but don't you think that line is at least a little misleading?

That light function is the only example that i'm aware of in the entire documentation so i think it should have been more clear and useful, especially considering that the light function works differently from the vertex and fragment ones.

Anyway i'm sorry for the opening of a non-issue.

clayjohn commented 4 years ago

now i understand i was wrong expecting a similar result as the diffuse_lambert render_mode, but don't you think that line is at least a little misleading?

Not really, it is an example of a lambertian lighting model. It's just a really ugly one! The benefit is that it is as simple as possible.

I have been planning on writing a full lighting shader tutorial for awhile but haven't found the time. Having more info on lighting shaders will really help clear up confusion.