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

Fix incorrect metallic calculations in PBRLighting.frag #2334

Merged
merged 4 commits into from
Dec 31, 2024

Conversation

yaRnMcDonuts
Copy link
Member

@yaRnMcDonuts yaRnMcDonuts commented Dec 1, 2024

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:

  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 anyone sees anything else that looks incorrect with the PBR shader's code.

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

@MeFisto94 MeFisto94 left a 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"

@yaRnMcDonuts
Copy link
Member Author

yaRnMcDonuts commented Dec 30, 2024

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.:

vec4 specularColor = (0.08 - 0.08 * Metallic) + albedo * Metallic; .

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.

@yaRnMcDonuts yaRnMcDonuts merged commit 36aac25 into jMonkeyEngine:master Dec 31, 2024
14 checks passed
@yaRnMcDonuts
Copy link
Member Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants