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

Make the prepass shader compile when lightmaps are present. #13402

Merged
merged 5 commits into from
May 18, 2024

Conversation

pcwalton
Copy link
Contributor

Commit 3f5a090 added a reference to STANDARD_MATERIAL_FLAGS_BASE_COLOR_UV_BIT, a nonexistent identifier, in the alpha discard portion of the prepass shader. Moreover, the logic didn't make sense to me. I think the code was trying to choose between the two UV sets depending on which is present, so I made it do that.

I noticed this when trying Bistro with #13277. I'm not sure why this issue didn't manifest itself before, but it's clearly a bug, so here's a fix. We should probably merge this before 0.14.

@pcwalton pcwalton added C-Bug An unexpected or incorrect behavior A-Rendering Drawing game state to the screen labels May 16, 2024
Commit 3f5a090 added a reference to
`STANDARD_MATERIAL_FLAGS_BASE_COLOR_UV_BIT`, a nonexistent identifier,
in the alpha discard portion of the prepass shader. Moreover, the logic
didn't make sense to me. I think the code was trying to choose between
the two UV sets depending on which is present, so I made it do that.
@alice-i-cecile
Copy link
Member

FYI @geckoxx <3 If you'd like to review this fix it would be much appreciated :)

@robtfm
Copy link
Contributor

robtfm commented May 17, 2024

it should be using shader def STANDARD_MATERIAL_BASE_COLOR_UV_B (which also needs to be defined for the prepass rust-side), similar to the main pass.

@pcwalton
Copy link
Contributor Author

@robtfm OK, I made that change. I believe that the Rust side is already defining that.

@pcwalton pcwalton requested a review from robtfm May 17, 2024 00:50
pcwalton added a commit to pcwalton/bevy that referenced this pull request May 17, 2024
This fixes the problem that led to bevyengine#13402.

I'm not sure if this had any negative consequences, but it didn't
replicate the prior behavior with the `Or` query filter, so I changed it
to match.
@geckoxx
Copy link
Contributor

geckoxx commented May 17, 2024

This seems correct. Sorry, I missed this when moving from using material flags to shader defs.

@pcwalton pcwalton added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label May 17, 2024
Copy link
Contributor

@robtfm robtfm left a comment

Choose a reason for hiding this comment

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

actually doesn't seem to work - error: Too many '# endif' lines. Each endif should be preceded by an if statement.

@pcwalton pcwalton marked this pull request as draft May 18, 2024 01:57
@pcwalton
Copy link
Contributor Author

Marking as draft while I investigate.

@geckoxx
Copy link
Contributor

geckoxx commented May 18, 2024

The endif in line 27 should be removed.

@alice-i-cecile alice-i-cecile added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels May 18, 2024
@alice-i-cecile alice-i-cecile added this to the 0.14 milestone May 18, 2024
@pcwalton pcwalton removed the S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged label May 18, 2024
@pcwalton pcwalton added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label May 18, 2024
@pcwalton pcwalton marked this pull request as ready for review May 18, 2024 21:40
@alice-i-cecile alice-i-cecile added this pull request to the merge queue May 18, 2024
Merged via the queue into bevyengine:main with commit 846757c May 18, 2024
28 checks passed
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.

None yet

5 participants