bevyengine / bevy

A refreshingly simple data-driven game engine built in Rust
https://bevyengine.org
Apache License 2.0
36.51k stars 3.6k forks source link

Lighting issue on scaled objects after upgrading to 0.8 #5514

Closed paul-hansen closed 2 years ago

paul-hansen commented 2 years ago

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

superdump commented 2 years ago

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

mockersf commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

superdump commented 2 years ago

I'm asking the question over at https://github.com/KhronosGroup/glTF/issues/2056#issuecomment-1201012199 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 commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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.

kanerogers commented 2 years ago

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 commented 2 years ago

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 .