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

Meshlet screenspace-derived tangents #15084

Merged
merged 29 commits into from
Sep 29, 2024

Conversation

JMS55
Copy link
Contributor

@JMS55 JMS55 commented Sep 7, 2024

image

For testing add a normal map to the bunnies with StandardMaterial like below, and then test that on both main and this PR (make sure to download the correct bunny for each). Results should be mostly identical.

normal_map_texture: Some(asset_server.load_with_settings(
    "textures/BlueNoise-Normal.png",
    |settings: &mut ImageLoaderSettings| settings.is_srgb = false,
)),

@JMS55 JMS55 requested a review from atlv24 September 7, 2024 22:12
@JMS55 JMS55 added the A-Rendering Drawing game state to the screen label Sep 7, 2024
@JMS55 JMS55 added this to the 0.15 milestone Sep 7, 2024
@DGriffin91
Copy link
Contributor

No need to address my comments. Just wanted to throw out some ideas, possibility for the future.

@JMS55
Copy link
Contributor Author

JMS55 commented Sep 8, 2024

Did some tests visualizing the tangents directly.

Before:
image-1

After:
image

There's a bit of inaccuracy around the head with all the concave curves, but not too bad. Seems good to me.

@JMS55 JMS55 marked this pull request as ready for review September 8, 2024 19:11
@alice-i-cecile alice-i-cecile added C-Performance A change motivated by improving speed, memory usage or compile times S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Sep 8, 2024
@@ -53,6 +53,7 @@ fn fragment(
#ifdef VERTEX_TANGENTS
#ifdef STANDARD_MATERIAL_NORMAL_MAP

// TODO: Transforming UVs mean we need to apply derivative chain rule for meshlet mesh material pass
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for clarification for my understanding, this was also the case before this PR correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I'll need to do it as some point, low priority atm.

@JMS55 JMS55 marked this pull request as ready for review September 13, 2024 03:22
@JMS55
Copy link
Contributor Author

JMS55 commented Sep 13, 2024

Decided just to negate the sign of tangent.w and move on. I read a bunch of papers but idk why it's backwards.

I've confirmed that tangents pretty much match bevy_mikktspace now.

@CptPotato
Copy link
Contributor

Some quick notes:

I've used tangent frames generated from pixel derivatives for a while myself, but reverted back to a full normal+tangent per vertex because of visible artifacts. The problem becomes noticeable when you have a smooth shaded mesh with a "strong" normal map. Calculating the tangent vector via derivatives means that it's not averaged for vertices and interpolated across the surface, causing visible seams between triangles (again, only if a normal map is present).

I haven't closely looked at your implementation, so it's possible that your approach is different from what I used and the issue doesn't apply here. I also think that the error is somewhat negligible - especially with the high resolution geometry that the meshlet rendering targets. So it's fine to keep as is imo., I just thought it was worth pointing out.

Save 16 bytes per vertex by calculating tangents in the shader at runtime, rather than storing them in the vertex data.

If you do go down the route of keeping the tangent frame and packing it, let me know. I think you can easily get the tangent frame down to 48 or 36 bits per vertex, possibly even smaller.

Decided just to negate the sign of tangent.w and move on. I read a bunch of papers but idk why it's backwards.

I've confirmed that tangents pretty much match bevy_mikktspace now.

There's two neat glTF test models you can use to check if your tangents are correct, NormalTangentTest & NormalTangentMirrorTest. They do have some weird seams you can ignore, only the spherical portion is important.

@JMS55
Copy link
Contributor Author

JMS55 commented Sep 17, 2024

Yeah, I'm not personally too worried about the inaccuracy. It's "good enough" for the intended use case. Nanite does have an option for explicit tangents, it wouldn't be too hard to add it back later. Not a priority right now though.

Copy link
Contributor

@atlv24 atlv24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is really cool. well done

@DGriffin91
Copy link
Contributor

Shadows don't seem to match current bevy main (0ebd7fc).

Current main
current main

This PR
current_pr

@JMS55
Copy link
Contributor Author

JMS55 commented Sep 24, 2024

Bevy main is using an outdated bunny model, I had forgot to regenerate the meshlet mesh before merging the last meshlet PR. The model on main is based on one of the earlier commits of the last meshlet PR, and not the last commit when it was ready to merge. I'm fairly certain that if you regenerate the meshlet mesh on main, you'd see the same issue.

As for why it looks broken, the LOD system thinks that the error from a lower LOD is not visible to the shadow map's camera, but when the shadow map is used by the main view it then has too much error. Using higher resolution shadow maps would fix this, but is more expensive. I'll probably look into adding a LOD bias for shadow views in a future PR.

@JMS55 JMS55 requested review from IceSentry and removed request for DGriffin91 September 27, 2024 16:58
@JMS55
Copy link
Contributor Author

JMS55 commented Sep 27, 2024

image

Bunnies on main (regenerated to ensure it's using the exact code from main)

@JMS55
Copy link
Contributor Author

JMS55 commented Sep 27, 2024

image
Bunnies in this PR (also freshly regenerated)

@JMS55 JMS55 added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Sep 28, 2024
Copy link
Contributor

@IceSentry IceSentry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Sep 29, 2024
Merged via the queue into bevyengine:main with commit 9cc7e7c Sep 29, 2024
30 checks passed
robtfm pushed a commit to robtfm/bevy that referenced this pull request Oct 4, 2024
* Save 16 bytes per vertex by calculating tangents in the shader at
runtime, rather than storing them in the vertex data.
* Based on https://jcgt.org/published/0009/03/04,
https://www.jeremyong.com/graphics/2023/12/16/surface-gradient-bump-mapping.
* Fixed visbuffer resolve to use the updated algorithm that flips ddy
correctly
* Added some more docs about meshlet material limitations, and some
TODOs about transforming UV coordinates for the future.


![image](https://github.com/user-attachments/assets/222d8192-8c82-4d77-945d-53670a503761)

For testing add a normal map to the bunnies with StandardMaterial like
below, and then test that on both main and this PR (make sure to
download the correct bunny for each). Results should be mostly
identical.

```rust
normal_map_texture: Some(asset_server.load_with_settings(
    "textures/BlueNoise-Normal.png",
    |settings: &mut ImageLoaderSettings| settings.is_srgb = false,
)),
```
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-Performance A change motivated by improving speed, memory usage or compile times S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants