bevy icon indicating copy to clipboard operation
bevy copied to clipboard

Split `StandardMaterialFlags` in two for compatibility with low end GPUs

Open oscrim opened this issue 2 years ago • 7 comments

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.

oscrim avatar Jan 25 '24 11:01 oscrim

Welcome, new contributor!

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

github-actions[bot] avatar Jan 25 '24 11:01 github-actions[bot]

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

oscrim avatar Jan 25 '24 11:01 oscrim

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

mockersf avatar Jan 25 '24 12:01 mockersf

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,

oscrim avatar Jan 25 '24 12:01 oscrim

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

oscrim avatar Jan 26 '24 10:01 oscrim

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 avatar Jan 26 '24 13:01 alice-i-cecile

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

valaphee avatar Jan 28 '24 12:01 valaphee

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

I created an issue https://github.com/gfx-rs/wgpu/issues/5318

Going to wait for rendering expertise and testing before merging

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

oscrim avatar Feb 28 '24 09:02 oscrim