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 meshlet vertex attribute interpolation #13775

Merged

Conversation

JMS55
Copy link
Contributor

@JMS55 JMS55 commented Jun 9, 2024

Objective

  • Mikktspace requires that we normalize world normals/tangents before interpolation across vertices, and then do not normalize after. I had it backwards.
  • We do not (am not supposed to?) need a second set of barycentrics for motion vectors. If you think about the typical raster pipeline, in the vertex shader we calculate previous_world_position, and then it gets interpolated using the current triangle's barycentrics.

Solution

  • Fix normal/tangent processing
  • Reuse barycentrics for motion vector calculations
  • Not implementing this for 0.14, but long term I aim to remove explicit vertex tangents and calculate them in the shader on the fly.

Testing

  • I tested out some of the normal maps we have in repo. Didn't seem to make a difference, but mikktspace is all about correctness across various baking tools. I probably just didn't have any of the ones that would cause it to break.
  • Didn't test motion vectors as there's a known bug with the depth buffer and meshlets that I'm waiting on the render graph rewrite to fix.

@JMS55 JMS55 added C-Bug An unexpected or incorrect behavior A-Rendering Drawing game state to the screen labels Jun 9, 2024
@JMS55 JMS55 added this to the 0.14 milestone Jun 9, 2024
@alice-i-cecile alice-i-cecile added the S-Needs-Review Needs reviewer attention (from anyone!) to move forward label Jun 9, 2024
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

The code is clear and reasonable, and I trust your testing and motivation here. This gets an LGTM. @atlv24 could you take a look?

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.

yep this looks good to me too :)

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jun 10, 2024
@alice-i-cecile alice-i-cecile 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 Jun 10, 2024
Merged via the queue into bevyengine:main with commit fd30e0c Jun 10, 2024
29 checks passed
mockersf pushed a commit that referenced this pull request Jun 10, 2024
# Objective

- Mikktspace requires that we normalize world normals/tangents _before_
interpolation across vertices, and then do _not_ normalize after. I had
it backwards.
- We do not (am not supposed to?) need a second set of barycentrics for
motion vectors. If you think about the typical raster pipeline, in the
vertex shader we calculate previous_world_position, and then it gets
interpolated using the current triangle's barycentrics.

## Solution

- Fix normal/tangent processing 
- Reuse barycentrics for motion vector calculations
- Not implementing this for 0.14, but long term I aim to remove explicit
vertex tangents and calculate them in the shader on the fly.

## Testing

- I tested out some of the normal maps we have in repo. Didn't seem to
make a difference, but mikktspace is all about correctness across
various baking tools. I probably just didn't have any of the ones that
would cause it to break.
- Didn't test motion vectors as there's a known bug with the depth
buffer and meshlets that I'm waiting on the render graph rewrite to fix.
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 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.

3 participants