URP Forward+ Additional lights only rendering in specific camera angles

[Ainqui]

I’m trying to write a shader on URP, usind the Forward+ rendering path. I’ve been following the manual page on adding additional lights for a custom node, but the lights only render either when the camera is inside their area, or when the light is near the bottom left of the screen. The scene only has three lights: a directional light, which seems to be functioning without issue, a a spotlight and a point light. Unity version is 6000.0.34f1, but I’ve tested and the issue still happens in both 6000.0.59f1 and 6000.2.8f1.

This is the custom node:

#ifndef MY_SHADER_INCLUDED
#define MY_SHADER_INCLUDED

#ifndef SHADERGRAPH_PREVIEW


#include "Packages/com.unity.render-pipelines.core/ShaderLibrary/CommonMaterial.hlsl"
#include "Packages/com.unity.render-pipelines.universal/ShaderLibrary/Core.hlsl"
#include "Packages/com.unity.render-pipelines.universal/ShaderLibrary/RealtimeLights.hlsl"

float3 MyLightingFunction(float3 normalWS, Light light)
{
    float NdotL = dot(normalWS, normalize(light.direction));
    NdotL = (NdotL + 1) * 0.5;
    return saturate(NdotL) * light.color * light.distanceAttenuation * light.shadowAttenuation;
}
#endif // SHADERGRAPH_PREVIEW

void LightingCelShaded_float(
    in float3 Position,
    in float3 Normal,
    out float3 Color)
{
#if defined(SHADERGRAPH_PREVIEW)
    Color = float3(0, 0, 0); // Value for preview
#else
    InputData inputData = (InputData)0;
    inputData.positionWS = Position;
    inputData.normalWS = Normal;
    inputData.viewDirectionWS = GetWorldSpaceNormalizeViewDir(Position);

    float4 clipPos = TransformWorldToHClip(Position);
    inputData.positionCS = clipPos;
    inputData.normalizedScreenSpaceUV = GetNormalizedScreenSpaceUV(clipPos.xy);

    Light mainLight = GetMainLight();

    // Main light
    Color = MyLightingFunction(Normal, mainLight);

    // Additional lights
    #if defined(_ADDITIONAL_LIGHTS)

    #if USE_FORWARD_PLUS
        UNITY_LOOP for (uint lightIndex = 0; lightIndex < min(URP_FP_DIRECTIONAL_LIGHTS_COUNT, MAX_VISIBLE_LIGHTS); lightIndex++)
        {
            Light additionalLight = GetAdditionalLight(lightIndex, inputData.positionWS, half4(1,1,1,1));
            Color += MyLightingFunction(Normal, additionalLight);
        }
    #endif

    uint pixelLightCount = GetAdditionalLightsCount();
    LIGHT_LOOP_BEGIN(pixelLightCount)
        Light additionalLight = GetAdditionalLight(lightIndex, inputData.positionWS, half4(1,1,1,1));
        Color += MyLightingFunction(Normal, additionalLight);
        LIGHT_LOOP_END
    #endif // _ADDITIONAL_LIGHTS

#endif // SHADERGRAPH_PREVIEW
}

#endif // MY_SHADER_INCLUDED

This is the complete shader:

And finally this is the rendering path settings asset, in case it helps:

The closes post I found to my issue was this one, but changing the clipping planes does nothing for me.
The farthest I got in troubleshooting was hardcoding the inputData.positionCS = clipPos; to a static value, which did change the angles at which the light renders. That said, I can’t see what’s wrong with the way I’m setting up that value.

Any help is appreciated!

[Canijo]

So, as far as i understand, there are a few things wrong.

First, the keyword for Forward+ is no longer USE_FORWARD_PLUS, but rather _CLUSTER_LIGHT_LOOP, however i believe they implemented something to also declare it for backward compatibility, but just as a heads up.

Also, you ¿should? be missing lots of keywords, as you are on an Unlit ShaderGraph, so i dont even know how its showing lighting :P, i guess you can still access some light info without those keywords so you might have just lucked out, but you probably should make sure you declare all they keywords you need (you can declare them in your CustomFunctionNode by checking the option “Use Pragmas” at the end of the node inspector)

Since Forward+ was implemented, the light loop has changed, since these changes are barely documented, your best source of truth is always the standard URP Lit.shader (com.unity.render-pipelines.universal/Shaders/Lit.shader), there you can see the keywords you need to declare and the way they calculate lighting.

Lighting in particular typically resides in the library “Lighting.hlsl” (com.unity.render-pipelines.universal/ShaderLibrary/Lighting.hlsl)

Now is something like this, for additional lights (from Lighting.hlsl), where you would replace BlinnPhong with your own

#if defined(_ADDITIONAL_LIGHTS)
    uint pixelLightCount = GetAdditionalLightsCount();

    #if USE_CLUSTER_LIGHT_LOOP
    [loop] for (uint lightIndex = 0; lightIndex < min(URP_FP_DIRECTIONAL_LIGHTS_COUNT, MAX_VISIBLE_LIGHTS); lightIndex ++)
    {
        CLUSTER_LIGHT_LOOP_SUBTRACTIVE_LIGHT_CHECK

        Light light = GetAdditionalLight(lightIndex, inputData, shadowMask, aoFactor);
        #ifdef _LIGHT_LAYERS
        if (IsMatchingLightLayer(light.layerMask, meshRenderingLayers))
        #endif
        {
            lightingData.additionalLightsColor += CalculateBlinnPhong(light, inputData, surfaceData);
        }
    }
    #endif

    LIGHT_LOOP_BEGIN(pixelLightCount)
    Light light = GetAdditionalLight(lightIndex, inputData, shadowMask, aoFactor);
    #ifdef _LIGHT_LAYERS
    if (IsMatchingLightLayer(light.layerMask, meshRenderingLayers))
    #endif
    {
        lightingData.additionalLightsColor += CalculateBlinnPhong(light, inputData, surfaceData);
    }
    LIGHT_LOOP_END
#endif

As an extra note, as per this post announcement, since 6.3, you have a few samples to implement custom lighting which i believe in your use case are going to come incredibly handy. There is also finally the option “Keep Lighting Variants” when you use an Unlit ShaderGraph, so that will let you forget about all the required lighting keywords declarations so ShaderGraph takes care of it for you.

Hope this helps

PD: I initially thought your light loop might be the old one, now i can see is following the new approach, so my best guess the problem might be just your keywords? else i dont really know. A fast way to check for it is changing your ShaderGraph to Lit, hardcode 0 in all the outputs except for emissive, where your output will be connected

[Ainqui]

Switched the keyword to _CLUSTER_LIGHT_LOOP, no changes there.
For the keywords, I added the following on line 6, from the Lit shader:

#pragma multi_compile _ _MAIN_LIGHT_SHADOWS _MAIN_LIGHT_SHADOWS_CASCADE _MAIN_LIGHT_SHADOWS_SCREEN
#pragma multi_compile _ _ADDITIONAL_LIGHTS_VERTEX _ADDITIONAL_LIGHTS
#pragma multi_compile _ _ADDITIONAL_LIGHT_SHADOWS

It made no difference. I also tried adding all other keywords on there, but it likewise made no difference. Am I declaring them wrong?

Changing it Lit does indeed correctly render lighting, with the emission turning off in the same place as before.

I’ll keep digging into the Lit shader.

[Canijo]

Ok i see you have been following this example from the manual.

I can see what error you are falling to, and how to fix it, sadly dont know enough to really understand why exactly it is like that, but anyways:

The LIGHTLOOP macro expects the struct InputData to be within its scope, specifically, it needs the variables positionWS and normalizedScreenSpaceUV to be propperly set. You are getting the normalizedScreenSpaceUV by first calculating the clipPos, then using its X and Y coords.

I dont know exactly why? but apparently you cannot properly calculate ClipPos from the fragment stage. In the Unity Manual example you can see it calculates it on the Vertex stage, and then it gets interpolated to the frag, so im guessing if you really needed ClipPos, you would need a custom interpolator on the Vertex shader, and then use that on the frag.

However, since you really only need “NormalizedScreenSpaceUV”, you can get that info from the “ScreenPosition” node. So, get that value, feed it into you CustomFunction, and then assign it to the variable on InputData

void LightingCelShaded_float(
    in float3 Position,
    in float3 Normal,
    in float2 ScreenPos,
    out float3 Color)
{
#if defined(SHADERGRAPH_PREVIEW)
    Color = float3(0, 0, 0); // Value for preview
#else
    InputData inputData = (InputData)0;
    inputData.positionWS = Position;
    inputData.normalWS = Normal;

    // all these are no longer needed currently for your use case
    // -------
    // inputData.viewDirectionWS = GetWorldSpaceNormalizeViewDir(Position);
    // float4 clipPos = TransformWorldToHClip(Position);
    // inputData.positionCS = clipPos;
    // inputData.normalizedScreenSpaceUV = GetNormalizedScreenSpaceUV(clipPos.xy);
    // -------
    inputData.normalizedScreenSpaceUV = ScreenPos;

This way, i believe everything should work propperly (i still think if you dont add the propper keywords eventually something will break, but the light angle flickering should stop)

[Ainqui]

That seems to have fixed it.
Thank you so much!