Added ambient filter in PBRLighting to control the brightness and color

[Ali-RS]

of the ambient light.

[Ali-RS]

@pspeed42 is it ok now ?

[riccardobl]

This doesn't look like a good idea. The env probes are used to get the global illumination from the environment, if you need a different light color, in my opinion, you should change the sky or whatever your probes are baking.

[pspeed42]

So, your recommendation is the rebake the probes every time the ambient color needs to change? Even though if you change the lighting color you don't need to rebake?

There are a lot of cases where PBR looks just fine without baking any probes ever... just using a generic prebuilt probe. You can control all aspects of the lighting color this way EXCEPT the ambient color.

[Ali-RS]

@riccardobl see this in my dynamic sky :

https://youtu.be/T3f3PS9aOgc

As there is noway to change this on LightProbe itself so I need to do this in material to be able to update ambient color for simulating day/night cycle.

[riccardobl]

If the ambient light changes, what is emitting it should change aswell. This looks like an hack for a very specific use case.

[Ali-RS]

Are you saying I need to rebake light probe every frame then ?

[pspeed42]

"very specific use case" = "any use case where you don't generate probes"

Also, does this mean we should ignore DirectionalLight, too?

[riccardobl]

You do whatever works better for you in your game. I just say that it is weird to multiply the ambient light with arbitrary colors given the context of this shader. You can argue that there is an usecase for everything, i'm sure some would also like to add fog here, but shaders included in the engine should have a limited well defined scope

[pspeed42]

Alright, I guess we can also try to strip out the DirectionalLight coloring, too, then?

[pspeed42]

Or actually, maybe just force everyone to always generate a probe and never reuse them. Perhaps keep track of the app runtime and throw an exception if the probe hasn't been generated since the app started? Just depends on how inflexible we want to be, I guess.

Personally, I think this change is fine. From my own perspective, I use it in 100% of my use-cases as none of my games will ever generate light probes.

Generated worlds, flickering lights, etc. all benefit from this change. To me, more use cases than not could benefit from this change and it's small.

But I also believe that until PBR has been converted to shader nodes that, yes, fog color should also be included.

[Ali-RS]

but shaders included in the engine should have a limited well defined scope

but this limitation should not cause everyone to end up with his own clone of PBR shader and no one using the one included in the engine.

Agree with Paul that this change will benefit most of us who are using PBR.
You can see in this post: https://hub.jmonkeyengine.org/t/best-way-to-scale-light-probe-for-day-night/41627

[riccardobl]

This is yet another way to tinker with ambient lights, we have env maps for Image Based Lighting then we have Ambient Lights and then your special Ambient Filter thing that is actually adding a filter to the pbr output, it doesn't even define an AmbientColor (as you named it), unless your env map doesn't have a color (and now we are back to the very specific use case).

Will this even preserve the physical accuracy of pbr? I doubt that. To be honest i've never seen this field in pbr renderers, there is color correction, but not "partial some lights only color filter", but maybe i haven't been looking at the right ones.

Everything else you said, exceptions, forbidding reuse, baking every frame etc, are just straw man arguments, i'm not even sure why you brought them in.

Said that, i don't necessarily oppose the pr, you can squeeze in this shader as many post processing passes as you want, i care only for a very limited amount about this, i can also see how this could work well for you when you have all the dispositions needed for it to make sense. I'm just giving my PERSONAL OPINION.

I use PBR and i've never thought about using this stuff, i do color correction, but not this.

[Ali-RS]

Sorry not meant to argue as I do not have enough knowledge about PBR. For this reason I asked it on the forum before I want to create this PR.

I made a quick search to see if Unity and Unreal are using an ambient filter for controlling day/night light in their PBR but I was not able to find anything useful.

For my usecase I needed a way to work for my day/night cycle with no need to regenerate light probe as I am changing ambient color every frame based on time of day and also adding an AmbienLigh to scene had no effect on PBR objects so I found Paul suggestion to be the simplest way to make it work for day/night lighting.

[pspeed42]

It should still be physically accurate in the sense that it is only modifying the ambient contribution of the environment. It's not modifying the "output" directly as you imply.

Edit: hah... except I guess it is. hm.

Edit 2: looking deeper, I think I'm wrong about being wrong. The fact that the shader keeps adding accumulated info to gl_FragColor threw me. The ambient filter as we've implemented it in this PR should be only adjusting the ambient contribution due to the light probes. It's modifying the colors that were passed back from the renderProbe() call. It's a super convenient way of reusing a grey-scale light probe for a variety of environments... in only three lines of code.

[Nehon]

This is technically OK. But I wouldn't add a new parameter to the material. As ricardo pointed out it's physically wrong to do this. However I can understand the practical motivation behind the change. I'd use the ambient light color instead of a color param per material. It's global.. So easier to control and most of the time you'll want to have the same color on every material.

[Ali-RS]

Thanks @Nehon

I'd use the ambient light color instead of a color param per material.

Does current implementation of PBRLighting take Ambient light into account ? Last time I tried it had no effect on PBR materials.

[Ali-RS]

By looking at SinglePassAndImageBasedLightingLogic code, it seems it already reads the ambientLightColor:

jmonkeyengine/jme3-core/src/main/java/com/jme3/material/logic/SinglePassAndImageBasedLightingLogic.java

Line 265 in dae85e1

if (l instanceof AmbientLight) {

then set it to g_AmbientLightColor uniform :

jmonkeyengine/jme3-core/src/main/java/com/jme3/material/logic/SinglePassAndImageBasedLightingLogic.java

Line 132 in dae85e1

ambientColor.setValue(VarType.Vector4, ambientLightColor);

but it is not used in vertex or frag shader as you can see here:

jmonkeyengine/jme3-core/src/main/resources/Common/MatDefs/Light/PBRLighting.vert

Line 7 in d57c362

uniform vec4 g_AmbientLightColor;

I took a look at phong material and in there this g_AmbientLightColor is sent from vertex shader to frag shader as varying vec3 AmbientSum;

jmonkeyengine/jme3-core/src/main/resources/Common/MatDefs/Light/Lighting.vert

Line 148 in d57c362

AmbientSum = (m_Ambient * g_AmbientLightColor).rgb;

and finally get applied to gl_FragColor :

jmonkeyengine/jme3-core/src/main/resources/Common/MatDefs/Light/Lighting.frag

Line 206 in d57c362

gl_FragColor.rgb = AmbientSum * diffuseColor.rgb +

Probably we need to grab g_AmbientLightColor in PBRLighting.frag shader and apply it to gl_FragColor ?

[Ali-RS]

I just tried it by adding this into fragment shader:

uniform vec4 g_AmbientLightColor;

and changing the code to :

color1.rgb *= g_AmbientLightColor.rgb;
color2.rgb *= g_AmbientLightColor.rgb;
color3.rgb *= g_AmbientLightColor.rgb;

then added an AmbientLight to scene and it worked. So we wont need to add a new parameter to material at all if we use g_AmbientLightColor. Then we can add an AmbientLight to scene and modify color on it however we want.

@Nehon is this alright ?

[Nehon]

Yep this is precisely what I meant.
Good work.
IMO it's a lot more practical to be able to change the ambient color globally than material by material.

However as Paul hinted it'd be less incorrect to multiply the color fetched from the prefiltered env map directly instead of multiplying it post lighting computation.
Even if it's still not completely right since the env map already contains prebaked lighting information.

[Ali-RS]

Thank you @Nehon
However there is one minor issue in :

jmonkeyengine/jme3-core/src/main/java/com/jme3/material/logic/SinglePassAndImageBasedLightingLogic.java

Line 53 in dae85e1

private final ColorRGBA ambientLightColor = new ColorRGBA(0, 0, 0, 1);

as you see it is initialized with (0,0,0,1), so if one not add an AmbientLight to scene every thing will be black. I want to know your idea for this.

In other branch in my repo I added a commit to use a boolean parameter UseAmbientLightColor so that i will apply g_AmbientLightColor only if UseAmbientLightColor is true.

Please see here:
Ali-RS@0718b86

Can you please tell me your idea about this ?

[Ali-RS]

Also another way to fix it without adding UseAmbientLightColor parameter in matdef.

I modified

jmonkeyengine/jme3-core/src/main/java/com/jme3/material/logic/SinglePassAndImageBasedLightingLogic.java

Line 261 in dae85e1

protected void extractIndirectLights(LightList lightList, boolean removeLights) {

to:

protected void extractIndirectLights(LightList lightList, boolean removeLights) {
        ambientLightColor.set(0, 0, 0, 1);
        boolean hasAmbientLight = false;
        for (int j = 0; j < lightList.size(); j++) {
            Light l = lightList.get(j);
            if (l instanceof AmbientLight) {
                hasAmbientLight = true;
                ambientLightColor.addLocal(l.getColor());
                if(removeLights){
                    lightList.remove(l);
                    j--;
                }
            }
            if (l instanceof LightProbe) {
                lightProbes.add((LightProbe) l);
                if(removeLights){
                    lightList.remove(l);
                    j--;
                }
            }
        }
        
        if(!hasAmbientLight) {
            ambientLightColor.r = 1;
            ambientLightColor.g = 1;
            ambientLightColor.b = 1;
        }
        
        ambientLightColor.a = 1.0f;
    }

Notice the use of boolean hasAmbientLight.

Which fix do you think is more suitable ?

[Nehon]

You can init the color to (1,1,1,1). Here it's remnant of the multi pass PhongLighting that computed ambient light in the first non ambient light pass.For the boolean, you can just inject the define whenever you find an ambient light in the Lighting logic.

[Nehon]

You can init the color to (1,1,1,1). Here it's remnant of the multi pass PhongLighting that computed ambient light in the first non ambient light pass.For the boolean, you can just inject the define whenever you find an ambient light in the Lighting logic.

[Ali-RS]

Okay, updated it to inject define value when ambient light found

@Nehon is this alright now?

Ali-RS/jmonkeyengine@master...Ali-RS:pbr-ambient-light

[pspeed42]

Neat. I like how this solution is turning out.

Note: today there is no real difference between ambient light and a material parameter override... but AmbientLight is waaay clearer in this case (and the first thing I tried when trying to make this work before). This is also maybe why other engines don't have the ambient filter parameter.

[Nehon]

looks good @Ali-RS ;)

[Ali-RS]

Thank you guys, I will make a new PR soon.

Thanks to Nehon today I learned something new with shaders.

[Ali-RS]

New PR is created: #1077