godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.14k stars 93 forks source link

Add an option to provide custom light occlusion functions #8671

Open reduz opened 10 months ago

reduz commented 10 months ago

Describe the project you are working on

Godot

Describe the problem or limitation you are having in your project

There are some cases where users may want to provide custom occlusion for lights depending on the material.

This is currently not possible to do. Would only work on forward renderer, of course, but still would be a good to have.

An example here of how displacement mapping looks with and without occlusion (shadows):

image (source)

Describe the feature / enhancement and how it helps to overcome the problem or limitation

The idea is to give users the option to write a custom occlusion function (without the need to write an entire light shader or custom shader template).

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

The function would look like this (in Godot shader language).

float light_occlusion(vec3 light_dir, uint light_mask) {

}

This allows the user to utilize global variables such as UV, position, etc. and compute a proper occlusion value for the light.

With this feature, displacement maps would automatically have light occlusion, too (or with a flag), and would also work on the compatibility renderer.

If this enhancement will not be used often, can it be worked around with a few lines of script?

n/a

Is there a reason why this should be core and not an add-on in the asset library?

Needs core shader work.

Calinou commented 10 months ago
jitspoe commented 8 months ago

In case this is useful, here's how I implemented this in my custom light() function for KOOK:

void light() {
    // LIGHT is the direction to the light in view space
#ifdef TRANSMISSION
    float d = max(-dot(LIGHT, NORMAL) * TRANSMISSION, dot(LIGHT, NORMAL));
#else
    float d = max(0.0, dot(LIGHT, NORMAL));
#endif
    float a = ATTENUATION;
    // Cast self-shadows based on heightmap
#ifndef DISABLE_HEIGHT_MAP
    {
        vec2 texture_size = vec2(textureSize(texture_albedo, 0));
        vec3 light_dir_relative_to_uv = normalize(normalize(LIGHT) * mat3(tangent, -binormal, normal_no_normalmap));
        float h = texture(texture_heightmap, uv_snapped).r;
        float shadow_scale = 1.0;
        for (int i = 1; i <= 4; ++i) {
            vec3 light_dir_scaled = light_dir_relative_to_uv * float(i) * 0.8;
            vec2 ofs = uv_snapped + (light_dir_scaled.xy / texture_size);
            float offset_h = texture(texture_heightmap, ofs).r;
            float heightmap_scale = 32.0;
            if (offset_h > (h + light_dir_scaled.z / heightmap_scale)) {
                shadow_scale = max(shadow_scale - 0.25, 0.0);
            }
        }
        a *= shadow_scale;
    }
#endif
    // PI? https://seblagarde.wordpress.com/2012/01/08/pi-or-not-to-pi-in-game-lighting-equation/
    float light_amount = d / PI * a;
    DIFFUSE_LIGHT += LIGHT_COLOR * light_amount;

With: image

Without: image

Note that I'm doing texel snapping and such, and have this configured for low-res chunky textures, so it might not be a simple copy/paste, but hopefully useful for reference.

lemilonkh commented 8 months ago

It seems a bit inconsistent to me that the other shader functions that users can define don't take arguments but this one does.

Everything else is passed in as global constants, why not the light direction and mask in this case?

On the other hand, I can see that you would pretty much always use at least the light direction if you define the occlusion function.

If there is a technical reason why these should be arguments instead of constants this can be ignored of course.

jitspoe commented 8 months ago

Hmm, that's a fair point. Also, constants make it easier to change the API without breaking existing shaders if you add more parameters. Could convert to parameters under the hood, potentially, if there's a performance concern (I have no idea what the pros/cons of parameters vs. constants are).

How does this work with baked lights? Would it be possible to get self-shadowing with baked lights that have the direction info? That's one thing I'm not able to do with my light() function since it seems the baked lights are handled differently.

Calinou commented 8 months ago

How does this work with baked lights? Would it be possible to get self-shadowing with baked lights that have the direction info?

Even if Directional is enabled in LightmapGI, it's not possible to know which light is lighting up a pixel when using fully baked lights (Static GI mode). This can only be done with the Dynamic GI mode, which only bakes indirect lighting (in which case you can determine direct light contribution).

Directional lightmaps can infer a general light direction at a given luxel, but it can't determine which lights contributed (and to what extent).

jitspoe commented 8 months ago

I mean, it won't be perfect, but it would be nice if the function were still called to do custom stuff with it. Possibly with a flag/parameter indicating it's a static lightmap, or maybe even a completely separate function. I'm running into perf issues with too many dynamic lights, and static lights don't use my custom light code for shadow casting, which makes the textures look very flat. 😟

Calinou commented 8 months ago

and static lights don't use my custom light code for shadow casting, which makes the textures look very flat. 😟

I'd suggest baking some of the bumpiness directly into the texture's base color (i.e. make the texture bumpy even if it has no normal map). While this is not PBR-compliant, this will likely lead to better visuals overall, especially for a game with a pixel art style.

Light generally comes from above in most scenes, so having bumps display as if there was always a subtle light on top is likely the way to go.

jitspoe commented 8 months ago

The fundamental core of my project was kind of to have self shadows done in as a shader to perfectly match the environment. Also, the cases where I need to use baked light are often for area lights like lava pits and such where point lights don't really work, so being able to apply custom shader logic to the lightmap rendering would be really useful here, as lava, slime, etc are lighting from below.

Calinou commented 6 months ago

Out of curiosity, how would passing varyings to the light occlusion function work? In 4.0, this was added for passing varyings from the fragment to light function: https://github.com/godotengine/godot/pull/44698

Would this need to be duplicated to allow passing varyings from the fragment to the light occlusion function? What about using a varying in both a light and light occlusion function (assuming both can be defined at the same time in a given shader)?

Also, am I correct in saying that this would look something like this? https://github.com/Calinou/godot/tree/shader-add-light-occlusion-function (I don't specifically intend to continue this, I was just exploring this for curiosity's sake.)

Edit: This shader can be ported to BaseMaterial3D for testing: https://godotshaders.com/shader/parallax-occlusion-mapping-with-self-shadowing/

jitspoe commented 5 months ago

Out of curiosity, is it really necessary to have a separate function for this as opposed to just setting some value in the light() function? That would reduce the complexity of having to add varyings support to the new function and whatnot.

Calinou commented 1 month ago

is it really necessary to have a separate function for this as opposed to just setting some value in the light() function?

The point of having a separate function is that overriding light() requires you to reimplement lighting behavior, while the separate function wouldn't require you to reimplement light computations. If you leave light() empty, the material won't react to lighting like it would if there was no light() function defined. This is why light() is commented out in the default shader template, but vertex() and fragment() are not.

I agree setting a built-in in light_occlusion() makes more sense than returning a value still.