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

[Merged by Bors] - Move texture sample out of branch in prepare_normal #5129

Closed
wants to merge 3 commits into from

Conversation

DGriffin91
Copy link
Contributor

@DGriffin91 DGriffin91 commented Jun 28, 2022

Objective

This fixes #5127

Solution

  • Moved texture sample out of branch in prepare_normal().

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-Rendering Drawing game state to the screen P-High This is particularly urgent, and deserves immediate attention labels Jun 29, 2022
Copy link
Contributor

@Elabajaba Elabajaba left a comment

Choose a reason for hiding this comment

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

Fixes the load_gltf example without breaking the array texture example.

LGTM

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.

Fixed this for me too!

@alice-i-cecile alice-i-cecile 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 Jun 29, 2022
@alice-i-cecile
Copy link
Member

@DGriffin91 can you use either "fixes" or "closes" in the PR description? These are magic words that will cause the issue to get linked properly :)

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.

Works for me too!

@alice-i-cecile
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Jun 29, 2022
# Objective

This fixes #5127

## Solution

- Moved texture sample out of branch in `prepare_normal()`.


Co-authored-by: DGriffin91 <github@dgdigital.net>
@bors bors bot changed the title Move texture sample out of branch in prepare_normal [Merged by Bors] - Move texture sample out of branch in prepare_normal Jun 29, 2022
@bors bors bot closed this Jun 29, 2022
@superdump
Copy link
Contributor

superdump commented Jun 29, 2022

EDIT: Ha! I spoke too soon. Griffin pointed out that both branches sample the texture. Ok. This is fine.

Just noting that this is the ‘wrong’ fix. This will now always sample the texture regardless of whether it needs to or not. The uniform control flow (i.e. branching logic based on values in a uniform) can be evaluated before executing the shader for the 2x2 pixel quad (which is how rasterisation works in order to calculate gradients to identify an appropriate mip level). This means that how things were before, if the uniform flags value meant that the true case didn’t happen, then the texture would not be sampled for any of the fragments being shaded.

The right fix for now is to remove the function parameters for the flags, remove the argument at call sites, and go back to using material.flags, directly referring to the uniform binding variable.

james7132 pushed a commit to james7132/bevy that referenced this pull request Jul 2, 2022
# Objective

This fixes bevyengine#5127

## Solution

- Moved texture sample out of branch in `prepare_normal()`.


Co-authored-by: DGriffin91 <github@dgdigital.net>
inodentry pushed a commit to IyesGames/bevy that referenced this pull request Aug 8, 2022
# Objective

This fixes bevyengine#5127

## Solution

- Moved texture sample out of branch in `prepare_normal()`.


Co-authored-by: DGriffin91 <github@dgdigital.net>
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 28, 2022
# Objective

This fixes bevyengine#5127

## Solution

- Moved texture sample out of branch in `prepare_normal()`.


Co-authored-by: DGriffin91 <github@dgdigital.net>
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective

This fixes bevyengine#5127

## Solution

- Moved texture sample out of branch in `prepare_normal()`.


Co-authored-by: DGriffin91 <github@dgdigital.net>
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 P-High This is particularly urgent, and deserves immediate attention 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.

load_gltf example is broken on main
6 participants