-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Fix incorrect metallic calculations in PBRLighting.frag #2334
Conversation
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again not really an expert too, but I guess it's a case of "try and see"
jme3-core/src/main/resources/Common/MatDefs/Light/PBRLighting.frag
Outdated
Show resolved
Hide resolved
jme3-core/src/main/resources/Common/MatDefs/Light/PBRLighting.frag
Outdated
Show resolved
Hide resolved
I updated my comment about the constant value in the metallic equation to be more understandable. I also am finding conflicting information about whether or not that constant value for non-metals should be 0.04 or 0.08 in this line.:
From my research, it sounds like either could work for PBR and it can be tweaked slightly for different implementation depending on how shiney they want their non-metallic substance to be by default. So that would explain why I previously was finding conflicting values. But now I believe 0.04 is the value that would match Khronos standards. Either way, a small change to this value ends up having very miniscule bearing on the rendering results, and changing this doesn't cause nearly the same issue as was being caused by the fZero calculation being incorrect. The fZero fix is definitely the more important aspect of this PR. |
I've merged this PR so it is ready for 3.8.0-alpha1. The only real change ended up being the straightforward fix for the broken fZero calculation, and I left nonMetalSpec as is, since it was already correct. And there will also be some other major changes (refactoring and feature-wise) happening to the PRB shader in another soon to be merged PR anyways, so if any further refactoring or code condensing changes are to be discussed, it is better off here: #2191 |
addresses this issue: #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:
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 anyone sees anything else that looks incorrect with the PBR shader's code.