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

PBR Lighting issue #2330

Open
zzuegg opened this issue Nov 18, 2024 · 7 comments
Open

PBR Lighting issue #2330

zzuegg opened this issue Nov 18, 2024 · 7 comments
Labels
defect Something that is supposed to work, but doesn't. Less severe than a "bug"
Milestone

Comments

@zzuegg
Copy link
Member

zzuegg commented Nov 18, 2024

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
Copy link
Member Author

zzuegg commented Nov 18, 2024

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

@stephengold stephengold added the defect Something that is supposed to work, but doesn't. Less severe than a "bug" label Nov 19, 2024
@stephengold
Copy link
Member

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.

@yaRnMcDonuts
Copy link
Member

yaRnMcDonuts commented Dec 1, 2024

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 added a commit to yaRnMcDonuts/jmonkeyengine_nrs that referenced this issue Dec 1, 2024
addresses this issue: jMonkeyEngine#2330

There were 2 apparent issues with our metallic calculation that were causing our PBR implementation to render different visual results compared to the khronos PBR standards:

1) fZero was being calculated incorrectly, as mentioned in the linked issue
2) the nonMetalSpec float constant was being set incorrectly. According to what I've read, it should always be 0.08

I also checked tested changes in my own app and it appears the rendering difference is more accurate now. But I'm not an expert in lighting math like this, so let me know if anything looks incorrect with the code.
@zzuegg
Copy link
Member Author

zzuegg commented Dec 1, 2024

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?

@yaRnMcDonuts
Copy link
Member

yaRnMcDonuts commented Dec 1, 2024

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.

@zzuegg
Copy link
Member Author

zzuegg commented Dec 1, 2024

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

The models are MetalRoughSperes and the no texture variant

@yaRnMcDonuts
Copy link
Member

yaRnMcDonuts commented Dec 2, 2024

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 yaRnMcDonuts added this to the v3.8.0 milestone Dec 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
defect Something that is supposed to work, but doesn't. Less severe than a "bug"
Projects
None yet
Development

No branches or pull requests

3 participants