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

Add attributes to gfx_defines macros #1080

Merged
merged 2 commits into from
Nov 25, 2016
Merged

Add attributes to gfx_defines macros #1080

merged 2 commits into from
Nov 25, 2016

Conversation

djmcgill
Copy link

For this commit, it only works on the vertex and constant structs, and NOT any of their fields.

For this commit, it only works on the `vertex` and `constant` structs, and NOT any of their fields.
@djmcgill
Copy link
Author

djmcgill commented Nov 20, 2016

This is a workaround for the issue rust-lang/rust#24189 which causes the character '#' to be considered a valid identifier character. Thus, the macro pattern

$(#[$attr:meta])* $name:ident

(which is greedy with no lookahead) fails because the parse is ambiguous. So whenever we want a pattern like this, there needs to be something separating the two tokens.

Lazy static uses commas: https://github.com/rust-lang-nursery/lazy-static.rs/blob/v0.2.1/src/lib.rs
Bitflags uses access modifiers: https://github.com/rust-lang-nursery/bitflags/blob/master/src/lib.rs#L183

This commit uses the "keywords" vertex and/or constant.

}

#[macro_export]
macro_rules! gfx_impl_struct_meta {
Copy link
Member

Choose a reason for hiding this comment

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

any reason you decided to not just put the extra stuff into gfx_impl_struct instead of routing through the new gfx_impl_struct_meta?

Copy link
Author

Choose a reason for hiding this comment

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

Backwards compatibility in case anybody called gfx_impl_struct directly.

Copy link
Author

Choose a reason for hiding this comment

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

I could probably have two patterns inside gfx_impl_struct, one with $(#[$attr:meta])* impl_struct_meta and one without, but then I believe I'd have to duplicate the whole body, which is worse to me than having an extra name.

@@ -52,6 +52,29 @@ macro_rules! gfx_defines {
gfx_pipeline!($name {$($field:$ty = $e,)+});
};

// The recursive case for vertex structs
($(#[$attr:meta])* vertex $name:ident {
Copy link
Member

Choose a reason for hiding this comment

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

is this ever going to hit, given that the same pattern is parsed above?

Copy link
Author

Choose a reason for hiding this comment

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

This pattern contains $($tail:tt)+, it's the recursive case. The above pattern only matches for a single struct.

@kvark
Copy link
Member

kvark commented Nov 25, 2016

Thanks @djmcgill ! I hope you'll be around if/when somebody faces the macros bugs ;)
@homu r+

@homu
Copy link
Contributor

homu commented Nov 25, 2016

📌 Commit 6f79425 has been approved by kvark

@homu homu merged commit 6f79425 into gfx-rs:master Nov 25, 2016
homu added a commit that referenced this pull request Nov 25, 2016
Add attributes to gfx_defines macros

For this commit, it only works on the `vertex` and `constant` structs, and NOT any of their fields.
@homu
Copy link
Contributor

homu commented Nov 25, 2016

⚡ Test exempted - status

adamnemecek pushed a commit to adamnemecek/gfx that referenced this pull request Apr 1, 2021
1080: Fix the validation of vertex buffer sizes r=kvark a=JCapucho

**Connections**
None that i know of

**Description**
~~The vertex buffer size (in vertices) was being divided by stride causing the limit to be lower than it was supposed to be.~~
This bug wasn't triggered earlier because if the stride was 0 it wouldn't perform any calculation and the stride was only set when a set pipeline command was received and the `VertexState` `inputs` were already created so the following commands would work:
```
SetPipeline with 1 vertex buffer
SetVertexBuffer with only 4 vertices
Draw 6 vertices
```
This would have passed validation while this wouldn't
```
SetPipeline with 1 vertex buffer of stride 8
SetVertexBuffer with 4 vertices
SetPipeline with 1 vertex buffer of stride 8
SetVertexBuffer with 4 vertices
Draw 3 vertices
```

Now all draw calls have proper vertex validation and not only after the `inputs` are populated

**Testing**
This change was tested after debugging an issue with the draw calls failing in a specific order in [veloren](https://gitlab.com/veloren/veloren/-/tree/imbris/wgpu-master-rebased)

Co-authored-by: Capucho <jcapucho7@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants