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

Custom Shader: Lighting artifacts are produced just outside of the area of effect of Spotlights and Omnilights #73176

Open
hidemat opened this issue Feb 12, 2023 · 18 comments

Comments

@hidemat
Copy link

hidemat commented Feb 12, 2023

Godot version

Godot 4 rc1

System information

Windows 10, Vulkan, NVIDIA GTX 1660

Issue description

It's likely that I configured something incorrectly, but I'm using a custom lighting shader and objects that are outside of the area of effect of spotlights and omnilights get lighting artifacts:
image

This is the shader code:

shader_type spatial;
render_mode diffuse_toon, specular_toon;

group_uniforms Textures;

	uniform vec4 base_color : source_color = vec4(1.0, 1.0, 1.0, 1.0);

group_uniforms;


group_uniforms LightingAndShadows;

	uniform float attenuation_strength : hint_range(0.0, 1.0, 0.01) = 0.8;
	uniform float shadow_edge : hint_range(0, 2.0, 0.01) = 0;
	uniform float light_edge : hint_range(0.01, 2.0, 0.01) = 0.3;
	
group_uniforms;


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

void light() {
	// Attenuation
	float attenuation = mix(clamp(ATTENUATION, 0.0, 1.0), 1.0, 1.0 - attenuation_strength);
	
	// Diffuse
	float NdotL = dot(NORMAL, LIGHT);
	float diffuse = clamp(NdotL, 0.0, 1.0);
	diffuse = smoothstep(shadow_edge, light_edge, diffuse);
	
	// Light color
	vec3 light_color = normalize(LIGHT_COLOR);
	
	DIFFUSE_LIGHT += diffuse * attenuation * ALBEDO * light_color;
}

Steps to reproduce

  1. Create an object that uses a material with the toon.gdshader
  2. Add either a spot light or an omni light, place the object just outside of the area of effect of the light; where it shouldn't be light, but is close to the border.
  3. This produces lighting artifacts

Minimal reproduction project

CustomShaderLightingBug.zip

@mrjustaguy
Copy link
Contributor

This is due to Forward Clustering.

@hidemat hidemat changed the title Custom Shader: Lighting artefacts are produced just outside of the area of effect of Spotlights and Omnilights Custom Shader: Lighting artifacts are produced just outside of the area of effect of Spotlights and Omnilights Feb 12, 2023
@Calinou
Copy link
Member

Calinou commented Feb 12, 2023

@clayjohn
Copy link
Member

I think the documentation can be improved about light shaders. The issue here is that the shader is trying to get lights to affect objects well outside their respective ranges. You can see in the image in the OP that there is no overlap between the lights and the objects.

The ATTENTUATION variable includes the range attenuation which would naturally make the light's impact fall off as it reached the edge of the range. In the shader in the OP, ATTENTUATION is override and interpolated towards 1.0 making the light not fall off smoothly as it reaches the outer rim of its range. At a certain point the light just falls off as it exceeds the boundary of the cluster it is in and since ATTENUATION was ignored, there is no smooth falloff before that happens.

To be clear, the offending line is float attenuation = mix(clamp(ATTENUATION, 0.0, 1.0), 1.0, 1.0 - attenuation_strength);

@clayjohn clayjohn added this to the 4.x milestone Feb 14, 2023
@hidemat
Copy link
Author

hidemat commented Feb 15, 2023

Can confirm. It was my mistake.
image

At the end I just multiply by the default built-in ATTENUATION, and that fixed the issue. This is what I changed.
DIFFUSE_LIGHT += diffuse * ATTENUATION * ALBEDO * light_color;

Should I close this issue?

@clayjohn
Copy link
Member

I would like to leave this open until a note is added to the documentation. We currently don't have enough information about light shaders so it is too easy to make this sort of mistake

@hidemat
Copy link
Author

hidemat commented Feb 15, 2023

Ok.

I was doing some more testing to correct this issue in the shader that I'm working on and found that it still occurs even if I'm not using attenuation. Am I doing something wrong? Is this expected behavior?

image

This is my new light() function:

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

@lyuma
Copy link
Contributor

lyuma commented Feb 16, 2023

@hidemat The issue will occur if you do not multiply by ATTENUATION, or at the very least ensure that your lighting approaches 0 as ATTENUATION approaches 0 (doesn't need to be linear necessarily).

If ATTENUATION is 0, Godot can choose to include or not include your light arbitrarily, and that arbitrary engine decision is what causes those clustering artifacts. That's why you need to respect ATTENUATION.

@hidemat
Copy link
Author

hidemat commented Feb 16, 2023

@hidemat The issue will occur if you do not multiply by ATTENUATION, or at the very least ensure that your lighting approaches 0 as ATTENUATION approaches 0 (doesn't need to be linear necessarily).

If ATTENUATION is 0, Godot can choose to include or not include your light arbitrarily, and that arbitrary engine decision is what causes those clustering artifacts. That's why you need to respect ATTENUATION.

Interesting. Ok I rewrote it to account for ATTENUATION when it is approaching 0, and that fixes the issue:

image

@hidemat
Copy link
Author

hidemat commented Feb 16, 2023

Please let me know if I shouldn't be posting here, but I want to explain the reason why I'm trying to do this. The main purpose of me messing around with the attenuation value is because I want invert the shadow values to create a mask that will allow me to add color to the shadows. While attempting to invert the diffuse values, I discovered the culprit of my original shader (the one I posted initially in this issue.):

void light() {
	  float diffuse = clamp(dot(NORMAL, LIGHT), 0.0, 1.0) * ATTENUATION;
	  float inverted_diffuse = mix(1.0, 0.0, diffuse);
	  DIFFUSE_LIGHT += inverted_diffuse * ALBEDO;
}

image

I also tried doing a one-minus with the same result:
float inverted_diffuse = 1.0 - diffuse;

Is there a correct way to invert the diffuse values? Or is just not possible?

[EDIT]
Here is another approah:

image

I think I could achieve the desired result if I could add color to the completely black (unlit) parts of the shadow. Is there a way to do that?

@lyuma
Copy link
Contributor

lyuma commented Feb 16, 2023

@hidemat That's just not going to work the way you describe it. As long as you are using Forward+ (Clustered), you'll need to ensure that the light function has no effect once ATTENUATION reaches 0. If not, you'll need to come up with a different approach, something like...

  1. Perhaps clustered rendering just isn't for you. If you use the Mobile renderer, every light will run the light() function and you can make whatever arbitrary shading logic you want without artifacts.
  2. Keep the clustered renedering, but increase the range for each light far enough that you can ensure that it is not relevant when attenuation hits 0 (or make the range so high that it's globally visible, and calculate the actual light attenuation yourself. This is similar to losing the clustering optimization since every object now sees every light, similar to the Mobile renderer.
  3. Another option perhaps would be to use a spotlight with large angle like 180 so it's sort of a negative spotlight.
  4. Perhaps negate the diffuse light, so you're subtracting the area it hits instead of adding, and then position your light sources so you can subtract them safely. Not sure about this exactly.
  5. Keep using 3.x for now since it has a renderer that may be better suited to your specific needs of colored shadows. As 4.1 and 4.2 are developed, more options should open up to allow you to better customize the rendering pipeline to suit your needs.

@Calinou
Copy link
Member

Calinou commented Feb 16, 2023

You can emulate colored shadows in 4.0 by using two lights at the same position: one with shadows and with the inverted color of the shadow color you desire, and one without shadows and with the shadow color you desire.

For instance, to get a white light with green shadows, place a magenta light with shadows and a green light without shadows at the exact same position.

@hidemat
Copy link
Author

hidemat commented Feb 17, 2023

Although it seems that simply enabling SDFGI produces some pretty results:
image

This is with multiplying by the default ATTENUATION value and not attempting to colorize the shadow. It also requires that ambient_light_disabled is not included in the render_mode parameters.
image

I'm actually quite pleased with how this looks. So, props to Godot 4 and all the hard work you guys put in. I really appreciate it.

@warcanin
Copy link

@hidemat 5. Keep using 3.x for now since it has a renderer that may be better suited to your specific needs of colored shadows. As 4.1 and 4.2 are developed, more options should open up to allow you to better customize the rendering pipeline to suit your needs.

Is there something the godot team is already working on that should fix this issue or is it low priority ? Thanks.

@Calinou
Copy link
Member

Calinou commented Apr 18, 2023

Is there something the godot team is already working on that should fix this issue or is it low priority ? Thanks.

Colored per-light shadows were removed on purpose in #45023 for optimization reasons. They're unlikely to be readded, unless a way is found to do so without impacting performance when you don't use the feature. Note that per-light shadow opacity is still supported, which is useful for LOD fading of distant light shadows.

Note that Godot 3.x only supported per-light tweaks of shadow color, not per-pixel colors in the shadow map itself (which is what you'd need for stained glass to cast shadows of the correct color, for instance). Per-pixel shadow colors are more flexible, but are also even more demanding than per-light shadow colors. See also godotengine/godot-proposals#3276.

@warcanin
Copy link

warcanin commented Apr 20, 2023

Custom texture shadow is really a deal breaker for me using godot.
There is one last thing i want to try but there is nothing in the documentation that help me understand how the light function work.
What kind of caluclation is done because i don't think it's a standard multiply because when i grab my texture and divide it by the albedo and plug it into a mix3Scalar with an dot(light,normal)*attenuation the result is not the texture that i pluged in, there is always the effect of the fragment albedo in there.
Edit, i know that i can use the "forward mobile" but i want sdfgi, my goal is not much pure cel shading as it is having control of the shadows.
image

@Calinou
Copy link
Member

Calinou commented Apr 20, 2023

Custom texture shadow is really a deal breaker for me using godot.

Are you referring to light projector textures (aka. light cookies)? This is already supported in Godot 4 using the Projector property in OmniLight3D and SpotLight3D. You must enable shadows on the light for it to work though.

@warcanin
Copy link

warcanin commented Apr 20, 2023

Custom texture shadow is really a deal breaker for me using godot.

Are you referring to light projector textures (aka. light cookies)? This is already supported in Godot 4 using the Projector property in OmniLight3D and SpotLight3D. You must enable shadows on the light for it to work though.

No i updated my post with a picture so you can better understand what is was getting to.
Normally this should work if the light fonction is multiplying with the albedo color then if i take the texture(a) that i want to be put on the light and divide it by the albedo then it would give me what the result necessary to get the texture(a) because it will just be multiplyed at the end with the albedo.
But seing the result it must no be a simple multiplication so that's why i wanted to know what is the light function doing at the end.

@warcanin
Copy link

Here is a more flagrant exemple.
image
Here is the fragment shader with a pure blue as a color constant, on of the quirk of this technique is that you need to add a value to all 3 channel or the division will not work.
And here is the shader graph with the resulting color.
image
Keep in mind the the color inputed is pure greenand there are blue in it now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants