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] - Derive AsBindGroup Improvements: Better errors, more options, update examples #5364

Closed

Conversation

wrapperup
Copy link
Contributor

@wrapperup wrapperup commented Jul 18, 2022

Objective

Builds on #5053

  • Provide better compile-time errors and diagnostics.
  • Add more options to allow more textures types and sampler types.
  • Update array_texture example to use upgraded AsBindGroup derive macro.

Solution

Split out the parsing of the inner struct/field attributes (the inside part of a #[foo(...)] attribute) for better clarity

Parse the binding index for all inner attributes, as it is part of all attributes (#[foo(0, ...)), then allow each attribute implementer to parse the rest of the attribute metadata as needed. This should make it very trivial to extend/change if needed in the future.

Replaced invocations of panic! with the syn::Error type, providing fine-grained errors that retains span information. This provides much nicer compile-time errors, and even better IDE errors.

image

Updated the array_texture example to demonstrate the new changes.

New AsBindGroup attribute options

#[texture(u32, ...)]

Where ... is an optional list of arguments.

Arguments Values Default
dimension = "..." "1d", "2d", "2d_array", "3d", "cube", "cube_array" "2d"
sample_type = "..." "float", "depth", "s_int" or "u_int" "float"
filterable = ... true, false true
multisampled = ... true, false false
visibility(...) all, none, or a list-combination of vertex, fragment, compute vertex, fragment

Example: #[texture(0, dimension = "2d_array", visibility(vertex, fragment))]

#[sampler(u32, ...)]

Where ... is an optional list of arguments.

Arguments Values Default
sampler_type = "..." "filtering", "non_filtering", "comparison". "filtering"
visibility(...) all, none, or a list-combination of vertex, fragment, compute vertex, fragment

Example: #[sampler(0, sampler_type = "filtering", visibility(vertex, fragment)]

Changelog

  • Added more options to #[texture(...)] and #[sampler(...)] attributes, supporting more kinds of materials. See above for details.
  • Upgraded IDE and compile-time error messages.
  • Updated array_texture example using the new options.

@IceSentry IceSentry added A-Rendering Drawing game state to the screen C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Jul 18, 2022
@IceSentry
Copy link
Contributor

I know it's still a draft, but I would suggest splitting this in 2 PRs. One for the better parsing and one that adds the new options. This will make it easier to review and increases how fast something can get merged.

@wrapperup
Copy link
Contributor Author

I know it's still a draft, but I would suggest splitting this in 2 PRs. One for the better parsing and one that adds the new options. This will make it easier to review and increases how fast something can get merged.

Yep, I may actually split it into 3, one for the syn errors, one for the reorganized parsing, and one for the new options.

@superdump
Copy link
Contributor

Looks like you need to run cargo fmt.

@superdump superdump added this to the Bevy 0.8 milestone Jul 18, 2022
@superdump
Copy link
Contributor

I'm adding this to the 0.8 milestone as it makes for a big improvement to the new AsBindGroup feature.

Copy link
Contributor

@superdump superdump left a comment

Choose a reason for hiding this comment

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

I think the mostly looks OK. I'm not strong with macros in Rust though so I'd suggest someone else take a look, like @cart as he wrote this. I haven't closely double-checked all the rendering bits are covered but it looks correct to me.

crates/bevy_render/macros/src/as_bind_group.rs Outdated Show resolved Hide resolved
crates/bevy_render/macros/src/as_bind_group.rs Outdated Show resolved Hide resolved
crates/bevy_render/macros/src/lib.rs Outdated Show resolved Hide resolved
examples/shader/array_texture.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@superdump superdump left a comment

Choose a reason for hiding this comment

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

And now I have double-checked the rendering bits. What has been added is correct. There's still some more to go, but that is fine for this PR. :)

@superdump
Copy link
Contributor

I know it's still a draft, but I would suggest splitting this in 2 PRs. One for the better parsing and one that adds the new options. This will make it easier to review and increases how fast something can get merged.

Yep, I may actually split it into 3, one for the syn errors, one for the reorganized parsing, and one for the new options.

Normally I would definitely agree with @IceSentry . In this case, as it's a key feature for the release, in my opinion, I suspect @cart may want to review it as-is.

@superdump
Copy link
Contributor

I just had a thought for a future PR: it would be nice to automatically label the bindings.

@superdump
Copy link
Contributor

If you want to iterate more quickly on getting this to pass CI checks, you can run cargo run -p ci locally.

@wrapperup
Copy link
Contributor Author

wrapperup commented Jul 18, 2022

Did a bit of clean-up, pretty happy with this PR now. Also now supports visibility flags

#[derive(AsBindGroup, Debug, Clone, TypeUuid)]
#[uuid = "9c5a0ddf-1eaf-41b4-9832-ed736fd26af3"]
struct ArrayTextureMaterial {
    #[texture(0, dimension = "2d_array", visibility(fragment))]
    #[sampler(1)]
    array_texture: Handle<Image>,
}

impl Material for ArrayTextureMaterial {
    fn fragment_shader() -> ShaderRef {
        "shaders/array_texture.wgsl".into()
    }
}

The syntax I think is pretty solid, but feel free to suggest any changes.

@wrapperup wrapperup marked this pull request as ready for review July 18, 2022 22:49
#[derive(Default)]
enum ShaderStageVisibility {
#[default]
All,
Copy link
Contributor

@superdump superdump Jul 19, 2022

Choose a reason for hiding this comment

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

I think the default should probably be vertex|fragment. If someone wants compute then they can set it, and if someone wants to restrict to vertex or fragment they can set it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The default previously was to have all visibility, but no material uses compute so this sounds reasonable.

Copy link
Contributor

Choose a reason for hiding this comment

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

@cart any opinions on what should be the default here?

Copy link
Member

Choose a reason for hiding this comment

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

It would mean that "by default" AsBindGroup won't work with compute shaders, which is a scenario we probably want to support (and increasingly so as the renderer progresses).

Its hard to have this conversation without knowing the performance tradeoffs. When I ran benchmarks of All vs Fragment (for the pbr shader), I couldn't identify any differences. But that doesn't necessarily mean that this is true for all platforms / scenarios.

Copy link
Member

Choose a reason for hiding this comment

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

Best I could find was kvark responding with "could be, yes" to this question:

When choosing bind group visibility, is there any performance implications of choosing something more specific like ShaderStages::VERTEX over just all?

Copy link
Member

Choose a reason for hiding this comment

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

Short term, I'm cool with limiting this to vertex|fragment given that compute is still pretty niche. But we should revisit this decision when it becomes more relevant.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was also thinking that aside from performance, enabling a resource for compute usage could change something about some validation/usage constraints as well. I don't know.

crates/bevy_render/macros/src/as_bind_group.rs Outdated Show resolved Hide resolved
@cart
Copy link
Member

cart commented Jul 19, 2022

Yup lets leave this as-is. Splitting out would definitely help with reviews, but I'm comfortable reviewing this as a unit in the interest of time / including this in 0.8.

Copy link
Member

@cart cart left a comment

Choose a reason for hiding this comment

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

Looks great to me. Love the improvements to the derive internals!

crates/bevy_render/macros/src/as_bind_group.rs Outdated Show resolved Hide resolved
Copy link
Member

@cart cart left a comment

Choose a reason for hiding this comment

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

I think this is ready to go. We can discuss "bind group visibility defaults" post-merge.

@cart
Copy link
Member

cart commented Jul 19, 2022

bors r+

bors bot pushed a commit that referenced this pull request Jul 19, 2022
…examples (#5364)

# Objective

- Provide better compile-time errors and diagnostics.
- Add more options to allow more textures types and sampler types.
- Update array_texture example to use upgraded AsBindGroup derive macro.

## Solution

Split out the parsing of the inner struct/field attributes (the inside part of a `#[foo(...)]` attribute) for better clarity

Parse the binding index for all inner attributes, as it is part of all attributes (`#[foo(0, ...)`), then allow each attribute implementer to parse the rest of the attribute metadata as needed. This should make it very trivial to extend/change if needed in the future.

Replaced invocations of `panic!` with the `syn::Error` type, providing fine-grained errors that retains span information. This provides much nicer compile-time errors, and even better IDE errors.

![image](https://user-images.githubusercontent.com/7478134/179452241-6d85d440-4b67-44da-80a7-9d47e8c88b8a.png)

Updated the array_texture example to demonstrate the new changes.

## New AsBindGroup attribute options


### `#[texture(u32, ...)]`
Where `...` is an optional list of arguments.
| Arguments    	| Values                                                         	| Default |
|--------------	|----------------------------------------------------------------	| ----------- |
| dimension = "..."    	| `"1d"`, `"2d"`, `"2d_array"`, `"3d"`, `"cube"`, `"cube_array"` 	|    `"2d"`    |
| sample_type = "..."  	| `"float"`, `"depth"`, `"s_int"` or `"u_int"`                   	|    `"float"`    |
| filterable = ...   	| `true`, `false`                                                	|    `true`     |
| multisampled = ... 	| `true`, `false`                                                	|    `false` |
| visibility(...) 	| `all`, `none`, or a list-combination of `vertex`, `fragment`, `compute` |   `vertex`, `fragment`   |

Example: `#[texture(0, dimension = "2d_array", visibility(vertex, fragment))]`


### `#[sampler(u32, ...)]`
Where `...` is an optional list of arguments.
| Arguments 	| Values                                            	| Default |
|-----------	|---------------------------------------------------	| ----------- |
| sampler_type = "..."   	| `"filtering"`, `"non_filtering"`, `"comparison"`. 	|  `"filtering"`  |
| visibility(...) 	| `all`, `none`, or a list-combination of `vertex`, `fragment`, `compute` |   `vertex`, `fragment`   |

Example: `#[sampler(0, sampler_type = "filtering", visibility(vertex, fragment)]`

## Changelog

- Added more options to `#[texture(...)]` and `#[sampler(...)]` attributes, supporting more kinds of materials. See above for details.
- Upgraded IDE and compile-time error messages.
- Updated array_texture example using the new options.
@bors bors bot changed the title Derive AsBindGroup Improvements: Better errors, more options, update examples [Merged by Bors] - Derive AsBindGroup Improvements: Better errors, more options, update examples Jul 19, 2022
@bors bors bot closed this Jul 19, 2022
@wrapperup wrapperup deleted the bindgroup-macro-additions branch July 22, 2022 15:36
@wrapperup wrapperup restored the bindgroup-macro-additions branch July 22, 2022 15:36
@alice-i-cecile alice-i-cecile added the A-Diagnostics Logging, crash handling, error reporting and performance analysis label Jul 27, 2022
inodentry pushed a commit to IyesGames/bevy that referenced this pull request Aug 8, 2022
…examples (bevyengine#5364)

# Objective

- Provide better compile-time errors and diagnostics.
- Add more options to allow more textures types and sampler types.
- Update array_texture example to use upgraded AsBindGroup derive macro.

## Solution

Split out the parsing of the inner struct/field attributes (the inside part of a `#[foo(...)]` attribute) for better clarity

Parse the binding index for all inner attributes, as it is part of all attributes (`#[foo(0, ...)`), then allow each attribute implementer to parse the rest of the attribute metadata as needed. This should make it very trivial to extend/change if needed in the future.

Replaced invocations of `panic!` with the `syn::Error` type, providing fine-grained errors that retains span information. This provides much nicer compile-time errors, and even better IDE errors.

![image](https://user-images.githubusercontent.com/7478134/179452241-6d85d440-4b67-44da-80a7-9d47e8c88b8a.png)

Updated the array_texture example to demonstrate the new changes.

## New AsBindGroup attribute options


### `#[texture(u32, ...)]`
Where `...` is an optional list of arguments.
| Arguments    	| Values                                                         	| Default |
|--------------	|----------------------------------------------------------------	| ----------- |
| dimension = "..."    	| `"1d"`, `"2d"`, `"2d_array"`, `"3d"`, `"cube"`, `"cube_array"` 	|    `"2d"`    |
| sample_type = "..."  	| `"float"`, `"depth"`, `"s_int"` or `"u_int"`                   	|    `"float"`    |
| filterable = ...   	| `true`, `false`                                                	|    `true`     |
| multisampled = ... 	| `true`, `false`                                                	|    `false` |
| visibility(...) 	| `all`, `none`, or a list-combination of `vertex`, `fragment`, `compute` |   `vertex`, `fragment`   |

Example: `#[texture(0, dimension = "2d_array", visibility(vertex, fragment))]`


### `#[sampler(u32, ...)]`
Where `...` is an optional list of arguments.
| Arguments 	| Values                                            	| Default |
|-----------	|---------------------------------------------------	| ----------- |
| sampler_type = "..."   	| `"filtering"`, `"non_filtering"`, `"comparison"`. 	|  `"filtering"`  |
| visibility(...) 	| `all`, `none`, or a list-combination of `vertex`, `fragment`, `compute` |   `vertex`, `fragment`   |

Example: `#[sampler(0, sampler_type = "filtering", visibility(vertex, fragment)]`

## Changelog

- Added more options to `#[texture(...)]` and `#[sampler(...)]` attributes, supporting more kinds of materials. See above for details.
- Upgraded IDE and compile-time error messages.
- Updated array_texture example using the new options.
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 28, 2022
…examples (bevyengine#5364)

# Objective

- Provide better compile-time errors and diagnostics.
- Add more options to allow more textures types and sampler types.
- Update array_texture example to use upgraded AsBindGroup derive macro.

## Solution

Split out the parsing of the inner struct/field attributes (the inside part of a `#[foo(...)]` attribute) for better clarity

Parse the binding index for all inner attributes, as it is part of all attributes (`#[foo(0, ...)`), then allow each attribute implementer to parse the rest of the attribute metadata as needed. This should make it very trivial to extend/change if needed in the future.

Replaced invocations of `panic!` with the `syn::Error` type, providing fine-grained errors that retains span information. This provides much nicer compile-time errors, and even better IDE errors.

![image](https://user-images.githubusercontent.com/7478134/179452241-6d85d440-4b67-44da-80a7-9d47e8c88b8a.png)

Updated the array_texture example to demonstrate the new changes.

## New AsBindGroup attribute options


### `#[texture(u32, ...)]`
Where `...` is an optional list of arguments.
| Arguments    	| Values                                                         	| Default |
|--------------	|----------------------------------------------------------------	| ----------- |
| dimension = "..."    	| `"1d"`, `"2d"`, `"2d_array"`, `"3d"`, `"cube"`, `"cube_array"` 	|    `"2d"`    |
| sample_type = "..."  	| `"float"`, `"depth"`, `"s_int"` or `"u_int"`                   	|    `"float"`    |
| filterable = ...   	| `true`, `false`                                                	|    `true`     |
| multisampled = ... 	| `true`, `false`                                                	|    `false` |
| visibility(...) 	| `all`, `none`, or a list-combination of `vertex`, `fragment`, `compute` |   `vertex`, `fragment`   |

Example: `#[texture(0, dimension = "2d_array", visibility(vertex, fragment))]`


### `#[sampler(u32, ...)]`
Where `...` is an optional list of arguments.
| Arguments 	| Values                                            	| Default |
|-----------	|---------------------------------------------------	| ----------- |
| sampler_type = "..."   	| `"filtering"`, `"non_filtering"`, `"comparison"`. 	|  `"filtering"`  |
| visibility(...) 	| `all`, `none`, or a list-combination of `vertex`, `fragment`, `compute` |   `vertex`, `fragment`   |

Example: `#[sampler(0, sampler_type = "filtering", visibility(vertex, fragment)]`

## Changelog

- Added more options to `#[texture(...)]` and `#[sampler(...)]` attributes, supporting more kinds of materials. See above for details.
- Upgraded IDE and compile-time error messages.
- Updated array_texture example using the new options.
bors bot pushed a commit that referenced this pull request Dec 11, 2022
# Objective

- #5364 Added a few features to the AsBindGroup derive, but if you don't know they exist they aren't documented anywhere.


## Solution

- Document the new arguments in the doc block for the derive.
alradish pushed a commit to alradish/bevy that referenced this pull request Jan 22, 2023
# Objective

- bevyengine#5364 Added a few features to the AsBindGroup derive, but if you don't know they exist they aren't documented anywhere.


## Solution

- Document the new arguments in the doc block for the derive.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
…examples (bevyengine#5364)

# Objective

- Provide better compile-time errors and diagnostics.
- Add more options to allow more textures types and sampler types.
- Update array_texture example to use upgraded AsBindGroup derive macro.

## Solution

Split out the parsing of the inner struct/field attributes (the inside part of a `#[foo(...)]` attribute) for better clarity

Parse the binding index for all inner attributes, as it is part of all attributes (`#[foo(0, ...)`), then allow each attribute implementer to parse the rest of the attribute metadata as needed. This should make it very trivial to extend/change if needed in the future.

Replaced invocations of `panic!` with the `syn::Error` type, providing fine-grained errors that retains span information. This provides much nicer compile-time errors, and even better IDE errors.

![image](https://user-images.githubusercontent.com/7478134/179452241-6d85d440-4b67-44da-80a7-9d47e8c88b8a.png)

Updated the array_texture example to demonstrate the new changes.

## New AsBindGroup attribute options


### `#[texture(u32, ...)]`
Where `...` is an optional list of arguments.
| Arguments    	| Values                                                         	| Default |
|--------------	|----------------------------------------------------------------	| ----------- |
| dimension = "..."    	| `"1d"`, `"2d"`, `"2d_array"`, `"3d"`, `"cube"`, `"cube_array"` 	|    `"2d"`    |
| sample_type = "..."  	| `"float"`, `"depth"`, `"s_int"` or `"u_int"`                   	|    `"float"`    |
| filterable = ...   	| `true`, `false`                                                	|    `true`     |
| multisampled = ... 	| `true`, `false`                                                	|    `false` |
| visibility(...) 	| `all`, `none`, or a list-combination of `vertex`, `fragment`, `compute` |   `vertex`, `fragment`   |

Example: `#[texture(0, dimension = "2d_array", visibility(vertex, fragment))]`


### `#[sampler(u32, ...)]`
Where `...` is an optional list of arguments.
| Arguments 	| Values                                            	| Default |
|-----------	|---------------------------------------------------	| ----------- |
| sampler_type = "..."   	| `"filtering"`, `"non_filtering"`, `"comparison"`. 	|  `"filtering"`  |
| visibility(...) 	| `all`, `none`, or a list-combination of `vertex`, `fragment`, `compute` |   `vertex`, `fragment`   |

Example: `#[sampler(0, sampler_type = "filtering", visibility(vertex, fragment)]`

## Changelog

- Added more options to `#[texture(...)]` and `#[sampler(...)]` attributes, supporting more kinds of materials. See above for details.
- Upgraded IDE and compile-time error messages.
- Updated array_texture example using the new options.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective

- bevyengine#5364 Added a few features to the AsBindGroup derive, but if you don't know they exist they aren't documented anywhere.


## Solution

- Document the new arguments in the doc block for the derive.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Diagnostics Logging, crash handling, error reporting and performance analysis A-Rendering Drawing game state to the screen C-Usability A targeted quality-of-life change that makes Bevy easier to use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants