jmonkeyengine icon indicating copy to clipboard operation
jmonkeyengine copied to clipboard

PBR Lighting issue

Open zzuegg opened this issue 1 year ago • 9 comments

To continue the discussion from: https://hub.jmonkeyengine.org/t/zombiegirl-gltfloader-vs-monkeywrench/48007/15

Summary:

According to gltf specs, and on the reference viewers, the default value for metallic factor is 1. Jme's importers are setting the metallic factor to 0 to pass the test cases. The problematic line in my opinion is:

//223 of PBRLighting
vec4 diffuseColor = albedo - albedo * Metallic;

I have checked, a few other repositories, and i have not yet found that piece of math again. Additionally, the whole block:

float specular = 0.5;
float nonMetalSpec = 0.08 * specular;
vec4 specularColor = (nonMetalSpec - nonMetalSpec * Metallic) + albedo * Metallic;
vec4 diffuseColor = albedo - albedo * Metallic;
vec3 fZero = vec3(specular);

is quite unique. F0 is in all other shaders defined as:

vec3 F0 = vec3(0.04);
F0 = mix(F0, albedo, metallic);

zzuegg avatar Nov 18 '24 19:11 zzuegg

When testing the MetalRoughSpheres jme shows also different results than the khronos sample viewer. Screenshot 2024-11-18 211130

The reflexivity is lost completely in the second column in jme while in the khronos renderer it slowly fades

zzuegg avatar Nov 18 '24 20:11 zzuegg

Thanks for investigating.

When it comes time to solve this, instead of modifying the PBRLighting shaders (and potentially breaking many apps) I suggest defining a new PBR material with a new name.

stephengold avatar Nov 19 '24 01:11 stephengold

This looks like it is quite a big bug in our PBR implementation.

I've always speculated that something looks slightly off with some of my PBR textures in jme compared to the original source, especially under certain lighting conditions... and I think this might have been it.

I just tested the results with the old (broken) fZero calculation compared to the new code you suggested, and it appears to look much better with your code.

The difference is even more noticeable first-hand, but you can still see a difference in these 2 screenshots (both taken with identical lighting in same place and the same time of day in-game) :

Here is with broken fZero calculation (you can notice a very slight annoying glare to parts of the scene) image

And here is the same time/place with the fixed fZero calculation. It looks much better in my opinion, especially the grass: image

yaRnMcDonuts avatar Dec 01 '24 08:12 yaRnMcDonuts

Unfortunately the sphere tests renders wrong when i replace the fZero calculation. There is more to it. My guess is that jme counteracts the wrongness at some point. I am not yet at that point. Currently i am convertig the khronos reference shader to jme so it can be used as a direct replacement.

Are the samples rendering correct on your end?

zzuegg avatar Dec 01 '24 09:12 zzuegg

Are the samples rendering correct on your end?

Do you mean the varying shiny spheres example you posted the screenshot from?

I tried finding that test to run and check, but I can't seem to find it.

yaRnMcDonuts avatar Dec 01 '24 09:12 yaRnMcDonuts

https://github.com/KhronosGroup/glTF-Sample-Models/tree/main/2.0

The models are MetalRoughSperes and the no texture variant

zzuegg avatar Dec 01 '24 09:12 zzuegg

I applied the fixed PBRLighting shader from my PR to those MetalRoughSpheres and loaded them up in my editor with defaultProbe.j3o, and it looks okay as far as I can tell:

image

What did it look like when you tried rendering them?

yaRnMcDonuts avatar Dec 02 '24 13:12 yaRnMcDonuts

Hi @zzuegg I'm curious if you managed to test this in the latest alpha release to see if you think the fixed fZero calculation is okay?

Or do you think there still may be a chance that the strange fZero calcualtion was necessary to account for something else that is unique in jme's pbr implementation?

I also just realized I didn't apply the new fZero calculation to the specGloss pipeilne, and it still uses the old calculation here: https://github.com/jMonkeyEngine/jmonkeyengine/blob/b846c6a3863cbf2bd193151fbb03c0cd9e375d50/jme3-core/src/main/resources/Common/ShaderLib/module/pbrlighting/PBRLightingUtils.glsllib#L477

Should I also apply these changes there, or do you think specGloss needs a different fZero calculation? I could try to test and see the difference, but unfortunately I only have 1 specGloss model in my game so its not as easy for me to spot the rendering differences with specGloss like I was able to when I changed things for the metallicRoughness pipeline.

Thanks!

yaRnMcDonuts avatar Mar 06 '25 02:03 yaRnMcDonuts

I am super unlucky currently and both of my pc's have a hardware failure, i hope to be able to salvage one combinded and get a new one once the 9950x3d is out. Not sure when i'll be able to do some more serious programming

zzuegg avatar Mar 07 '25 08:03 zzuegg