add pipeline constants plumbing #4683
Conversation
There was a problem hiding this comment.
Only one hard (potential?) issue I see with rustfmt; otherwise, I have a bunch of nitpicks and questions, but nothing that I think is a hard blocker.
| @@ -1465,6 +1466,12 @@ pub struct VertexState<'a> { | |||
| /// The name of the entry point in the compiled shader. There must be a function with this name | |||
| /// in the shader. | |||
| pub entry_point: &'a str, | |||
| /// Specifies the values of pipeline-overridable constants in the shader module module. | |||
| /// | |||
| /// The key represents the numeric ID of the constant, if one is specified, and otherwise the constant's identifier name. | |||
There was a problem hiding this comment.
The key represents the numeric ID of the constant, if one is specified, and otherwise the constant's identifier name.
I don't think I understand this. Does this mean that the key might be one of (1) String containing ASCII numeric digits or (2) a typical C-style identifier like might be written in a shader?
Doing further reading in 7.2.2 `override` Declaration from the spec., it seems that option (1) corresponds to the id(…) attribute:
If the declaration has an id attribute applied, the literal operand is known as the pipeline constant ID.
… The constant is identified by a pipeline-overridable constant identifier string, which is the base-10 representation of the pipeline constant ID if specified, and otherwise the declared name of the constant.
However, that seems like String would be the wrong type for this, since we'd have to parse the ID as a u16 anyway for validation, right? Am I missing something?
This question applies to other places where this doc has been copy-pasted.
There was a problem hiding this comment.
if specified, and otherwise the declared name of the constant.
They key must be the pipeline constant ID (u16) if one was specified via the @id() attribute or the name/identifier of the pipeline-overridable constant.
Since the key is a String in the IDL, we keep it as such until we reach naga where we will validate the necessary invariants.
If wgpu users want more type expressiveness for the constants we can update its API later to accommodate that. It could look like:
enum Key {
Id(u16),
Name(&str)
}
enum Value {
F32(f32),
U32(u32),
I32(i32),
Bool(bool),
}
constants: &HashMap<Key, Value> or &[(Key, Value)]
There was a problem hiding this comment.
I'm not particularly happy with the untyped api. At the very least, I think we should add the Key type. I'm not sure there's actually any benefit from strongly typing the Value here.
There was a problem hiding this comment.
Value could implement From for all the types which would make it easier to use. But yeah, f64 already has From impls for all of those.
There was a problem hiding this comment.
What is your take on?
constants: &'a HashMap<Key, Value> or &'a [(Key<'a>, Value)]Note that Key::Name would most likely have to be a String for the HashMap but could be a &str for the slice of tuples.
There was a problem hiding this comment.
I'm definitely leaning array - hash maps are hard to construct inline and we don't really need deduplication behavior at the edge.
There was a problem hiding this comment.
Thinking a bit more about this, I think we should come up with a solution that works for all users of this API (Firefox, deno and wgpu) since all have to pass this structure through wgpu-core -> wgpu-hal -> naga.
There was a problem hiding this comment.
I'll try to update it to the shape below and report back.
enum Key<'a> {
Id(u16),
Name(&'a str)
}
constants: &'a [(Key<'a>, f64)]There was a problem hiding this comment.
@cwfitzgerald I tried to update the API to use ^ everywhere but I'm running into lifetime issues and would like to get to the implementation. Would you be ok with the current API for now?
There was a problem hiding this comment.
Yeah, we can leave it for now and iterate on it once it's in.
| /// | ||
| /// The key represents the numeric ID of the constant, if one is specified, and otherwise the constant's identifier name. | ||
| /// | ||
| /// The value may represent any of WGSL's concrete scalar types. |
There was a problem hiding this comment.
question: I doubt it, but: is it worth linking to Scalar docs here?
This question applies to other places where this doc has been copy-pasted.
There was a problem hiding this comment.
idk really, maybe we can clarify it once someone runs into this?
There was a problem hiding this comment.
I consider this conversation resolved, but I'll leave it up in case others might be interested in this question. Feel free to Resolve, if you want.
I was thinking it would be best to add a changelog entry once we are done with all the changes (this PR is targeting a feature branch not trunk). |
teoxoy
force-pushed
the
pipeline-constants
branch
from
c3f93e6
to
e3d4779
Compare
|
Pushed a new commit that addresses 2 comments. |
There was a problem hiding this comment.
All concerns I had during my PR LGTM. I can't speak for @jimblandy's concerns, tho.
|
@crowlKats do the deno changes look good? |
|
I managed to do it again... 🤦♂️😅 (#4677 (comment)) I need to get used to this workflow. |
Adds pipeline constants plumbing to the
pipeline-constantsfeature branch.Tracking meta issue: #4484.