Skip to content

Commit

Permalink
Fix AsBindGroup derive, texture attribute, visibility flag parsing (#…
Browse files Browse the repository at this point in the history
…8868)

# Objective

- Fix the AsBindGroup texture attribute visibility flag parsing
- This appears to have been caused by a syn crate update which then the
visibility code got updated
- Also I noticed that by default the vertex and fragment flags were on,
so visibility(compute) would actually make the texture visible to
vertex, fragment and compute shaders, I fixed this too

## Solution

- Update flag parsing to use MetaList.parse_nested_meta function, which
loads the flags into a Vec then loop through those flags
- Change initial visibility flags to use VisibilityFlags::default()
rather than VisibilityFlags::vertex_fragment()
  • Loading branch information
loganbenjamin committed Jun 21, 2023
1 parent c39e02c commit ee1368a
Show file tree
Hide file tree
Showing 2 changed files with 70 additions and 31 deletions.
69 changes: 38 additions & 31 deletions crates/bevy_render/macros/src/as_bind_group.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use syn::{
parse::{Parse, ParseStream},
punctuated::Punctuated,
token::Comma,
Data, DataStruct, Error, Fields, LitInt, LitStr, Meta, Result,
Data, DataStruct, Error, Fields, LitInt, LitStr, Meta, MetaList, Result,
};

const UNIFORM_ATTRIBUTE_NAME: Symbol = Symbol("uniform");
Expand Down Expand Up @@ -636,39 +636,46 @@ const VISIBILITY_COMPUTE: Symbol = Symbol("compute");
const VISIBILITY_ALL: Symbol = Symbol("all");
const VISIBILITY_NONE: Symbol = Symbol("none");

fn get_visibility_flag_value(meta: Meta) -> Result<ShaderStageVisibility> {
let mut visibility = VisibilityFlags::vertex_fragment();
fn get_visibility_flag_value(meta_list: &MetaList) -> Result<ShaderStageVisibility> {
let mut flags = Vec::new();

use syn::Meta::Path;
match meta {
// Parse `#[visibility(all)]`.
Path(path) if path == VISIBILITY_ALL => {
return Ok(ShaderStageVisibility::All)
}
// Parse `#[visibility(none)]`.
Path(path) if path == VISIBILITY_NONE => {
return Ok(ShaderStageVisibility::None)
meta_list.parse_nested_meta(|meta| {
flags.push(meta.path);
Ok(())
})?;

if flags.is_empty() {
return Err(Error::new_spanned(
meta_list,
"Invalid visibility format. Must be `visibility(flags)`, flags can be `all`, `none`, or a list-combination of `vertex`, `fragment` and/or `compute`."
));
}

if flags.len() == 1 {
if let Some(flag) = flags.get(0) {
if flag == VISIBILITY_ALL {
return Ok(ShaderStageVisibility::All);
} else if flag == VISIBILITY_NONE {
return Ok(ShaderStageVisibility::None);
}
}
// Parse `#[visibility(vertex, ...)]`.
Path(path) if path == VISIBILITY_VERTEX => {
}

let mut visibility = VisibilityFlags::default();

for flag in flags {
if flag == VISIBILITY_VERTEX {
visibility.vertex = true;
}
// Parse `#[visibility(fragment, ...)]`.
Path(path) if path == VISIBILITY_FRAGMENT => {
} else if flag == VISIBILITY_FRAGMENT {
visibility.fragment = true;
}
// Parse `#[visibility(compute, ...)]`.
Path(path) if path == VISIBILITY_COMPUTE => {
} else if flag == VISIBILITY_COMPUTE {
visibility.compute = true;
} else {
return Err(Error::new_spanned(
flag,
"Not a valid visibility flag. Must be `all`, `none`, or a list-combination of `vertex`, `fragment` and/or `compute`."
));
}
Path(path) => return Err(Error::new_spanned(
path,
"Not a valid visibility flag. Must be `all`, `none`, or a list-combination of `vertex`, `fragment` and/or `compute`."
)),
_ => return Err(Error::new_spanned(
meta,
"Invalid visibility format: `visibility(...)`.",
)),
}

Ok(ShaderStageVisibility::Flags(visibility))
Expand Down Expand Up @@ -794,7 +801,7 @@ fn get_texture_attrs(metas: Vec<Meta>) -> Result<TextureAttrs> {
}
// Parse #[texture(0, visibility(...))].
List(m) if m.path == VISIBILITY => {
visibility = get_visibility_flag_value(Meta::Path(m.path))?;
visibility = get_visibility_flag_value(&m)?;
}
NameValue(m) => {
return Err(Error::new_spanned(
Expand Down Expand Up @@ -910,7 +917,7 @@ fn get_sampler_attrs(metas: Vec<Meta>) -> Result<SamplerAttrs> {
}
// Parse #[sampler(0, visibility(...))].
List(m) if m.path == VISIBILITY => {
visibility = get_visibility_flag_value(Meta::Path(m.path))?;
visibility = get_visibility_flag_value(&m)?;
}
NameValue(m) => {
return Err(Error::new_spanned(
Expand Down Expand Up @@ -966,7 +973,7 @@ fn get_storage_binding_attr(metas: Vec<Meta>) -> Result<StorageAttrs> {
match meta {
// Parse #[storage(0, visibility(...))].
List(m) if m.path == VISIBILITY => {
visibility = get_visibility_flag_value(Meta::Path(m.path))?;
visibility = get_visibility_flag_value(&m)?;
}
Path(path) if path == READ_ONLY => {
read_only = true;
Expand Down
32 changes: 32 additions & 0 deletions crates/bevy_render/src/render_resource/bind_group.rs
Original file line number Diff line number Diff line change
Expand Up @@ -334,3 +334,35 @@ where
self.into()
}
}

#[cfg(test)]
mod test {
use super::*;
use crate as bevy_render;
use bevy_asset::Handle;

#[test]
fn texture_visibility() {
#[derive(AsBindGroup)]
pub struct TextureVisibilityTest {
#[texture(0, visibility(all))]
pub all: Handle<Image>,
#[texture(1, visibility(none))]
pub none: Handle<Image>,
#[texture(2, visibility(fragment))]
pub fragment: Handle<Image>,
#[texture(3, visibility(vertex))]
pub vertex: Handle<Image>,
#[texture(4, visibility(compute))]
pub compute: Handle<Image>,
#[texture(5, visibility(vertex, fragment))]
pub vertex_fragment: Handle<Image>,
#[texture(6, visibility(vertex, compute))]
pub vertex_compute: Handle<Image>,
#[texture(7, visibility(fragment, compute))]
pub fragment_compute: Handle<Image>,
#[texture(8, visibility(vertex, fragment, compute))]
pub vertex_fragment_compute: Handle<Image>,
}
}
}

0 comments on commit ee1368a

Please sign in to comment.