Improve bindgroup layout entry docs to document PARTIALLY_BOUND_BINDING_ARRAY #8795
Conversation
…LayoutEntry. Specifically documenting that this field becomes an upper bound when `Features::PARTIALLY_BOUND_BINDING_ARRAY` is active.
| /// | ||
| /// When this is `Some` the following validation applies: | ||
| /// - Size must be of value 1 or greater. | ||
| /// - Count must be of value 1 or greater, this corresponds to the length of the array of resources that will be bound. |
There was a problem hiding this comment.
I changed this to Count to match the name of the field and made it explicit that this conveys the length of the array, which helps clarify how PARTIALLY_BOUND_BINDING_ARRAY affects the validation rules.
inner-daemons
left a comment
inner-daemons
left a comment
There was a problem hiding this comment.
I have a love-hate relationship with these tiny docs changes, but in general they are helpful and positive. Welcome!
.gitignore
Outdated
| # Generated by Cargo | ||
| # will have compiled files and executables | ||
| target/ | ||
| target |
There was a problem hiding this comment.
Ending with a slash has no effect on how this file is interpreted but makes it clear to the reader that target is a directory and not a file. This change won't be accepted.
Edit:
I see your explanation about the symlinks. I'm unqualified to speak on this, but I still highly doubt this will be accepted. Nearly every rust project uses target/, and it is cargo's default.
There was a problem hiding this comment.
It is interpreted differently, target/ matches only directories, while target (or /target like the examples I linked) also matches symlinks.
Either way, maintainer's call, I've force pushed to the branch and dropped the commit 👍
There was a problem hiding this comment.
Oh, I see this was edited as I was wording a reply and addressing the feedback. On my system cargo init . generated a .gitignore with /target, just like the examples I linked in the description. Either way, I think it's fine to keep the changes limited to the documentation, that's the main goal of this PR.
There was a problem hiding this comment.
Ultimately I'm completely out of my depth here. I'd give it a 50/50 chance that the next guy to see this (hello Connor!) accepts this, not because its controversial, but because I have no idea about it. But yeah keeping it to its own PR seems like a good habit.
There was a problem hiding this comment.
From docs for gitignore:
If there is a separator at the end of the pattern then the pattern will only match directories, otherwise the pattern can match both files and directories.
This matches the reason stated in the Cargo commit that changed from /target/ to /target 8 (!?) years ago: rust-lang/cargo@f4e0eef
wgpu is actually an old enough project to have missed the train with that change for gitignore, and the change is well-motivated. So, I think we'd accept a (separate) PR!
There was a problem hiding this comment.
The only other thing I'd say is that we should then use "/target/" to avoid ignoring all files named target anywhere, even if we don't expect to have any such files.
There was a problem hiding this comment.
Ah, interesting find @ErichDonGubler, thanks for looking that up. The starting slash is interesting, I had never given that much thought, but agree it would be best to have it there @inner-daemons .
Good discussion & feedback, thanks to you both. I've filed a separate PR to move the slash from the end of the line to the beginning of the line in #8796.
| /// - When `ty == BindingType::StorageTexture`, [`Features::STORAGE_RESOURCE_BINDING_ARRAY`] must be supported. | ||
| /// - When any binding in the group is an array, no `BindingType::Buffer` in the group may have `has_dynamic_offset == true` | ||
| /// - When any binding in the group is an array, no `BindingType::Buffer` in the group may have `ty.ty == BufferBindingType::Uniform`. | ||
| /// - If [`Features::PARTIALLY_BOUND_BINDING_ARRAY`] is enabled, the specified count becomes the upper bound instead. |
There was a problem hiding this comment.
This is very helpful!
There was a problem hiding this comment.
Yeah... I hear you, it's a small change and a lot of overhead, but this extra bit of information would've saved me much quite a bit of time trying to understand & use a variable length array :)
iwanders
force-pushed
the
improve-bindgroup-layout-entry-docs
branch
from
2dd4eb9 to
7781696
Compare
With the trailing slash it will not ignore 'target' if it is a symlink. By removing that symlinked 'target' directories are correctly ignored. The slash at the start ensures that it will not ignore directories called 'target' that may be created in the future that aren't at the root of the repository. This change aligns with the default .gitignore from `cargo init`. Originally discussed in gfx-rs#8795 (comment)
3 participants
Connections
None.
Description
This improves the documentation for the count field of the
BindGroupLayoutEntryto document the behaviour of thecountfield when thePARTIALLY_BOUND_BINDING_ARRAYfeature is enabled. I expect this was just an accidental omission when that feature was introduced.Second commit removes the
/fromtarget/in the gitignore. Iftarget/is used, this does not match symlinks but only directories. Usingtargetapplies to both symlinks and directories as far as I know. I usually symlink to/tmp/targetto keep the volatile assets in tmpfs and off my actual disk, so when I made the first commit I almost comitted my target symlink. Other projects in this namespace, like wgpu-native and rspirv also lack the trailing/.Testing
I determined the existing validation rules by assigning texture arrays of varying sizes with and without
PARTIALLY_BOUND_BINDING_ARRAYactive. Then I modified the documentation, built the documentation and verified that the link to the feature works.Squash or Rebase?
I would recommend a rebase, given that the two commits are completely independent.
Checklist
cargo fmt.taplo format. -> don't have this installed, but I didn't touch any toml files.cargo clippy --tests. If applicable, add:--target wasm32-unknown-unknownN/Acargo xtask testto run tests. -> Don't have this installed, first thing passed.CHANGELOG.mdentry. -> No changes the user needs to be aware of.