Skip to content

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

Merged
alice-i-cecile merged 5 commits intobevyengine:mainfrom
pcwalton:prepass-lightmaps-fix
May 18, 2024
Merged

Make the prepass shader compile when lightmaps are present.#13402
alice-i-cecile merged 5 commits intobevyengine:mainfrom
pcwalton:prepass-lightmaps-fix

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.
@pcwalton pcwalton force-pushed the prepass-lightmaps-fix branch from cae2b30 to d2e8dbb Compare May 16, 2024 23:59
@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
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.

5 participants