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

bevy_pbr: Normalize world_normal #6540

Closed
wants to merge 1 commit into from

Conversation

greytdepression
Copy link
Contributor

Objective

Fixes #6528

Solution

Commit 681c9c6 introduced a bug for certain GPUs where the foxes in the many_foxes stress test example were not shaded correctly. The cause of this bug (at least on my machine) is apply_normal_mapping() no longer normalizing the world_normal vector.

Note that this solution is not ideal as the comment specifically says:

NOTE: The mikktspace method of normal mapping explicitly requires that the world normal NOT
be re-normalized in the fragment shader. This is primarily to match the way mikktspace
bakes vertex tangents and normal maps so that this is the exact inverse. Blender, Unity,
Unreal Engine, Godot, and more all use the mikktspace method. Do not change this code
unless you really know what you are doing.
http://www.mikktspace.com/

We may want to revert this in the future, but for now it seems to fix this bug.


Changelog

Reverted part of 681c9c6

@greytdepression
Copy link
Contributor Author

@hymm @DasLixou @SecretPocketCat can you test this out on your machines?
@superdump do you know if this change might cause any issues in other places?

@alice-i-cecile alice-i-cecile added this to the Bevy 0.9 milestone Nov 10, 2022
@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-Rendering Drawing game state to the screen labels Nov 10, 2022
@hymm
Copy link
Contributor

hymm commented Nov 10, 2022

The foxes seem to be colored correctly now.

Note: On discord @mockersf mentioned that animated_fox example is colored correctly on main. The difference might be that the animated fox is not scaled, but the ones in many foxes are scaled. So the issue might be that we're not dealing with scaling correctly somewhere and normalizing at this stage will just hide the probelm.

@Elabajaba
Copy link
Contributor

I tested animated_fox with a sub 1.0 scale and it slowly gets more glitched out as you decrease the scale from 1.0.

@superdump
Copy link
Contributor

I think #6543 is perhaps the proper fix... perhaps.

@superdump
Copy link
Contributor

The root cause that I fixed in #6543 was that skinned normals were not being normalised in the vertex stage, which was missed in #5666. Also, it turns out that even normalising all normals in the vertex stage and then having them interpolated to each fragment, there is a discrepancy in the length of the normals so I added re-normalisation at the end of apply_normal_mapping().

I'm grateful for @greytdepression looking into this. At the same time, I think this PR incorrectly re-normalises the normal in the fragment shader before normal mapping is applied, which Mikkelsen expressly said should not be done. Nor does it address the scale problem that some were observing. #6543 addresses these problems so I'd prefer to close this one and go with that one. I hadn't seen that this PR had been opened until late in my debugging/fixing process but I want it to be clear that I do appreciate the effort!

@SecretPocketCat
Copy link
Contributor

@hymm @DasLixou @SecretPocketCat can you test this out on your machines? @superdump do you know if this change might cause any issues in other places?

Looks good on my setup.

@hymm
Copy link
Contributor

hymm commented Nov 11, 2022

Works on mine

@superdump
Copy link
Contributor

Closing as #6543 was merged. Thanks again for the effort!

@superdump superdump closed this Nov 11, 2022
@greytdepression
Copy link
Contributor Author

Closing as #6543 was merged. Thanks again for the effort!

No problem, it was fun digging around in the source code a bit! :D
I'm happy you found the actual underlying problem! My PR was more of a quick hack to put a bandaid on it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

many_foxes are all black
6 participants