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

Split StandardMaterialFlags in two for compatibility with low end GPUs #11525

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

oscrim
Copy link

@oscrim oscrim commented Jan 25, 2024

Objective

Fixes #10066

Solution

Added StandardMaterialAlphaModeFlags containing the alpha mode bitflags previously in StandardMaterialFlags. This removes all the bits previously in the last 16-bits of the StandardMaterialFlags u32 which makes transparency function as intended again.


Changelog

This section is optional. If this was a trivial fix, or has no externally-visible impact, you can delete this section.

  • What changed as a result of this PR?
  • If applicable, organize changes under "Added", "Changed", or "Fixed" sub-headings
  • Stick to one or two sentences. If more detail is needed for a particular change, consider adding it to the "Solution" section
    • If you can't summarize the work, your change may be unreasonably large / unrelated. Consider splitting your PR to make it easier to review and merge!

Migration Guide

This section is optional. If there are no breaking changes, you can delete this section.

  • If this PR is a breaking change (relative to the last release of Bevy), describe how a user might need to migrate their code to support these changes
  • Simply adding new functionality is not a breaking change.
  • Fixing behavior that was definitely a bug, rather than a questionable design choice is not a breaking change.

Copy link
Contributor

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

@oscrim
Copy link
Author

oscrim commented Jan 25, 2024

Ive tested this fix on my phone which uses an Adreno 660 which previously couldnt render transparency but with this fix it can.

@mockersf
Copy link
Member

could you add comments in the code explaining why it's needed and we can't have everything in a single u32?

@oscrim
Copy link
Author

oscrim commented Jan 25, 2024

could you add comments in the code explaining why it's needed and we can't have everything in a single u32?

Absolutely, would something like this suffice?

// NOTE: These bitflags are separate from StandardMaterialFlags due to a bug on some older GPUs
// that are unable to use the last 16 bits of a u32
bitflags::bitflags! {
    /// Bitflags info about the alpha mode of the material a shader is currently rendering.
    /// This is accessible in the shader in the [`StandardMaterialUniform`]
    #[repr(transparent)]
    pub struct StandardMaterialAlphaModeFlags: u32 {
        const ALPHA_MODE_OPAQUE          = 0;
        const ALPHA_MODE_MASK            = 1;
        const ALPHA_MODE_BLEND           = 2;
        const ALPHA_MODE_PREMULTIPLIED   = 3;
        const ALPHA_MODE_ADD             = 4;
        const ALPHA_MODE_MULTIPLY        = 5;
    }
}

Also this in pbr_types.wgsl

    // 'flags' is a bit field indicating various options. 
    // u32 is 32 bits so we would have up to 32 options. Some lower-end mobile GPUs only support 16-bits
    // so we can only use half of the u32 
    flags: u32,
    alpha_mode_flags: u32,

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-Rendering Drawing game state to the screen X-Controversial There is active debate or serious implications around merging this PR labels Jan 25, 2024
@oscrim
Copy link
Author

oscrim commented Jan 26, 2024

@alice-i-cecile Can I ask why this is considered controversial? And is there anything I can do to where its not controversial?

@alice-i-cecile
Copy link
Member

Twiddling with bitflags for hardware-specific support is always a bit scary. It looks like this is a good approach, but it requires more testing and expert opinion than the average rendering PR as a result.

Hmm, actually I think Complex is a better tag here. Going to wait for rendering expertise and testing before merging :)

@alice-i-cecile alice-i-cecile added D-Complex Quite challenging from either a design or technical perspective. Ask for help! and removed X-Controversial There is active debate or serious implications around merging this PR labels Jan 26, 2024
@valaphee
Copy link
Contributor

valaphee commented Jan 28, 2024

Might be worth creating an issue in wgpu (as it could be poly-filled/worked around there)

@oscrim
Copy link
Author

oscrim commented Feb 28, 2024

Might be worth creating an issue in wgpu (as it could be poly-filled/worked around there)

I created an issue gfx-rs/wgpu#5318

Going to wait for rendering expertise and testing before merging

Is there anything I can do to help out with the testing?

@BenjaminBrienen BenjaminBrienen added X-Contentious There are nontrivial implications that should be thought through D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed D-Complex Quite challenging from either a design or technical perspective. Ask for help! labels Jan 23, 2025
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 D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged X-Contentious There are nontrivial implications that should be thought through
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

Transparency doesnt work when on webgl adreno devices
5 participants