Fix local_invocation_id and local_invocation_index handling#9099
Fix local_invocation_id and local_invocation_index handling#9099Wumpf merged 4 commits intogfx-rs:trunkfrom
Conversation
Wumpf
left a comment
Wumpf
left a comment
There was a problem hiding this comment.
some hygenie/maintainability things that need addressing, otherwise lgtm
| ep_input.arg_name | ||
| "{}.{} / WaveGetLaneCount()", | ||
| ep_input.arg_name, | ||
| ep_input.local_invocation_index_name.as_ref().unwrap() |
There was a problem hiding this comment.
don't like the unwrap here at all. Yes the invariant of (waaaay) above change is right now that if there's a subgroupid, then invocation index name will be Some but it's too easy to violate that via future iterations.
Why not just always set the invocation index name and overwrite only if a user specified one is there? The question of whether we expect it to be in use is a separate one then.
There was a problem hiding this comment.
I'm gonna add a comment. The problem there is that we always get this unused variable, even where it doesn't make sense (like in vertex shaders).
| self.write_workgroup_variables_initialization( | ||
| func_ctx, | ||
| module, | ||
| local_invocation_id_name.unwrap(), |
There was a problem hiding this comment.
as above, it's too hard to reason through these unwraps outside of this PR
There was a problem hiding this comment.
Same comment as above.
|
@Wumpf Can you take another look |
…ex being written incorrectly
ChristopherBiscardi
left a comment
ChristopherBiscardi
left a comment
There was a problem hiding this comment.
Approving for the same reasons as the msl writer PR #8739 (review) which uses this code.
I've read through the code, ported bevy and a series of examples to use it, and it is working as expected.
Wumpf
left a comment
Wumpf
left a comment
There was a problem hiding this comment.
still very much unhappy about the unwrapping :/. Testing looks much nicer
But lgtm overall, so let's ship this.
Fixed changelog entry a bit though.
|
@ChristopherBiscardi thank you so much for giving those changes a spin here and on the other PRs! |
4 participants
Connections
Closes #8820
This is blocking #8752 and #8739, which I would rather not keep having to maintain. So speed would be appreciated here.
Description
Solves issue with builtins possibly being written multiple times in HLSL and MSL, and with naming conflicts when users name variables
__local_invocation_idor__local_invocation_index.Testing
Snapshots
Squash or Rebase?
Squash
Checklist
cargo fmt.taplo format.cargo clippy --tests. If applicable, add:--target wasm32-unknown-unknowncargo xtask testto run tests.CHANGELOG.mdentry.