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

Lighting issue on scaled objects after upgrading to 0.8 #5514

Closed
paul-hansen opened this issue Jul 31, 2022 · 11 comments
Closed

Lighting issue on scaled objects after upgrading to 0.8 #5514

paul-hansen opened this issue Jul 31, 2022 · 11 comments
Labels
A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior P-Regression Functionality that used to work but no longer does. Add a test for this!

Comments

@paul-hansen
Copy link
Contributor

Bevy version

0.8

Relevant system information

Windows 10

`AdapterInfo { name: "NVIDIA GeForce RTX 3080", vendor: 4318, device: 8726, device_type: DiscreteGpu, backend: Vulkan }`

What you did

Upgraded my project to 0.8 following the migration guide.

What went wrong

I have an asteroid field with asteroids that are randomly positioned and scaled. The asteroid models appear to have incorrect lighting when they are scaled in 0.8 but they looked correct in 0.7. They also look correct when their scale is left at 1.0

Additional information

It seems like this may only be happening with directional lights? I haven't 100% confirmed that though.

I've created a minimal reproduction of this here: https://github.com/paul-hansen/bevy-0.8-migration-lighting/blob/main/src/main.rs

Here are images showing the problem from the minimal repo:

Bevy 0.8

image

Bevy 0.7

image

@paul-hansen paul-hansen added C-Bug An unexpected or incorrect behavior S-Needs-Triage This issue needs to be labelled labels Jul 31, 2022
@superdump superdump added A-Rendering Drawing game state to the screen P-Regression Functionality that used to work but no longer does. Add a test for this! and removed S-Needs-Triage This issue needs to be labelled labels Jul 31, 2022
@superdump
Copy link
Contributor

superdump commented Jul 31, 2022

I’ll take a look at this when I can. I’d guess it’s a problem with normals / tangents / normal mapping.

@mockersf
Copy link
Member

mockersf commented Jul 31, 2022

This started with #3872.

The object with shadow issues is at scale 30, maybe the tangents generated are not precise enough at that scale

@superdump
Copy link
Contributor

superdump commented Aug 1, 2022

Thanks for bisecting! I suspected it was due to that code one way or another. I thought the tangents should be transformed by the forward model transform, but it makes sense that the scale needs to be unaffected somehow. Scaling and renormalising is probably not a good idea due to precision loss. I'll look into it. :)

@superdump
Copy link
Contributor

I just tested normalising the xyz components of the vertex tangent and that works. Now to figure out how to fix it properly.

@superdump
Copy link
Contributor

I'm asking the question over at KhronosGroup/glTF#2056 (comment) where there is a discussion about where to state that renormalisation should be done, and I think it also applies to this case of uniform scaling in bevy, as well as uniform/non-uniform scaling contained in a glTF hierarchy. I know that the scaling is being applied manually in bevy in this issue, but it could just as well have been part of the transform hierarchy in a glTF scene.

@kanerogers
Copy link

@superdump Have you looked at using the "Normal Mapping without Precomputed Tangents" method? I must admit that it's completely over my head, but this has helped Hotham with models that have tricky normals / tangents (eg. models with mirror UV maps):

http://www.thetenthplanet.de/archives/1180

@superdump
Copy link
Contributor

I’ve taken a quick look at it and it sounds to me like there are still some drawbacks and cases it doesn’t handle compared with mikktspace.

@kanerogers
Copy link

I’ve taken a quick look at it and it sounds to me like there are still some drawbacks and cases it doesn’t handle compared with mikktspace.

Oh, interesting! Keen to hear if there's something we're doing wrong, and honestly I don't know enough about tangents/normals to have a strong opinion. Do you have any references that identify shortcomings with the computed tangents method?

@superdump
Copy link
Contributor

Here's a reference to Unreal Engine 5 using the method right now for nanite geometry: https://docs.unrealengine.com/5.0/en-US/nanite-virtualized-geometry-in-unreal-engine/#supportedfeaturesofnanite and there's a paragraph that states:

Per vertex tangents are not stored from the Static Mesh when it is enabled for Nanite. Instead, tangent space is implicitly derived in the pixel shader. Tangent data is not stored in order to reduce data size. This difference in tangent space using this approach could cause discontinuities at edges. However, this particular issue has not shown to be significant, and there are plans to support vertex tangents in a future release.

But also in the comments of the article you linked it seems there are notes about the T and B 'pseudovectors' being 'faceted' and having discontinuities across triangle edges, if I understood correctly.

From my perspective, much of real-time rendering seems to be about trying to use approximations that are fast and work well enough. So if the in-shader derived tangents work for you, great! If you observe issues, then you can look into using mikktspace instead.

nicopap added a commit to nicopap/bevy_mod_fbx that referenced this issue Aug 4, 2022
Read the FBX transform tree and assign entities for each node in the hierarchy,
computing and setting the bevy `Transform` according to FBX's completely
insane transform propagation algorithm.

Also a few unrelated things (I'm sorry)

This is also a first step toward loading skeleton rigs.

- Correct the way the scene's `UnitScaleFactor` is applied: instead of dividing
  the vertices position by it, we just add the scale to the root node of the scene,
  this also requires dividing by default by 100.0 the scale of all FBX files.
- Add a tool to print the FBX file content in the `dev_docs` directory
- Add a "Contributing" section to the README
- Fixes #26
- The Bistro scene's normals are broken, but this is a bevy bug: bevyengine/bevy#5514
@kanerogers
Copy link

Here's a reference to Unreal Engine 5 using the method right now for nanite geometry: https://docs.unrealengine.com/5.0/en-US/nanite-virtualized-geometry-in-unreal-engine/#supportedfeaturesofnanite and there's a paragraph that states:

Per vertex tangents are not stored from the Static Mesh when it is enabled for Nanite. Instead, tangent space is implicitly derived in the pixel shader. Tangent data is not stored in order to reduce data size. This difference in tangent space using this approach could cause discontinuities at edges. However, this particular issue has not shown to be significant, and there are plans to support vertex tangents in a future release.

But also in the comments of the article you linked it seems there are notes about the T and B 'pseudovectors' being 'faceted' and having discontinuities across triangle edges, if I understood correctly.

From my perspective, much of real-time rendering seems to be about trying to use approximations that are fast and work well enough. So if the in-shader derived tangents work for you, great! If you observe issues, then you can look into using mikktspace instead.

That's really helpful, thank you! Appreciate the thoughtful response here! 🙏🏻

@superdump
Copy link
Contributor

Fixed in the above PR. I've proposed that it go into a 0.8.1 if we make a patch release. But that's up to @cart .

@bors bors bot closed this as completed in 681c9c6 Aug 18, 2022
cart pushed a commit that referenced this issue Aug 19, 2022
# Objective

- Morten Mikkelsen clarified that the world normal and tangent must be normalized in the vertex stage and the interpolated values must not be normalized in the fragment stage. This is in order to match the mikktspace approach exactly.
- Fixes #5514 by ensuring the tangent basis matrix (TBN) is orthonormal

## Solution

- Normalize the world normal in the vertex stage and not the fragment stage
- Normalize the world tangent xyz in the vertex stage
- Take into account the sign of the determinant of the local to world matrix when calculating the bitangent

---

## Changelog

- Fixed - scaling a model that uses normal mapping now has correct lighting again
maccesch pushed a commit to Synphonyte/bevy that referenced this issue Sep 28, 2022
# Objective

- Morten Mikkelsen clarified that the world normal and tangent must be normalized in the vertex stage and the interpolated values must not be normalized in the fragment stage. This is in order to match the mikktspace approach exactly.
- Fixes bevyengine#5514 by ensuring the tangent basis matrix (TBN) is orthonormal

## Solution

- Normalize the world normal in the vertex stage and not the fragment stage
- Normalize the world tangent xyz in the vertex stage
- Take into account the sign of the determinant of the local to world matrix when calculating the bitangent

---

## Changelog

- Fixed - scaling a model that uses normal mapping now has correct lighting again
james7132 pushed a commit to james7132/bevy that referenced this issue Oct 28, 2022
# Objective

- Morten Mikkelsen clarified that the world normal and tangent must be normalized in the vertex stage and the interpolated values must not be normalized in the fragment stage. This is in order to match the mikktspace approach exactly.
- Fixes bevyengine#5514 by ensuring the tangent basis matrix (TBN) is orthonormal

## Solution

- Normalize the world normal in the vertex stage and not the fragment stage
- Normalize the world tangent xyz in the vertex stage
- Take into account the sign of the determinant of the local to world matrix when calculating the bitangent

---

## Changelog

- Fixed - scaling a model that uses normal mapping now has correct lighting again
ItsDoot pushed a commit to ItsDoot/bevy that referenced this issue Feb 1, 2023
# Objective

- Morten Mikkelsen clarified that the world normal and tangent must be normalized in the vertex stage and the interpolated values must not be normalized in the fragment stage. This is in order to match the mikktspace approach exactly.
- Fixes bevyengine#5514 by ensuring the tangent basis matrix (TBN) is orthonormal

## Solution

- Normalize the world normal in the vertex stage and not the fragment stage
- Normalize the world tangent xyz in the vertex stage
- Take into account the sign of the determinant of the local to world matrix when calculating the bitangent

---

## Changelog

- Fixed - scaling a model that uses normal mapping now has correct lighting again
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 P-Regression Functionality that used to work but no longer does. Add a test for this!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants