Skip to content

add pipeline constants plumbing#4683

Merged
teoxoy merged 1 commit intogfx-rs:pipeline-constantsfrom
teoxoy:pipeline-constants
Nov 20, 2023
Merged

add pipeline constants plumbing#4683
teoxoy merged 1 commit intogfx-rs:pipeline-constantsfrom
teoxoy:pipeline-constants

Conversation

@teoxoy
Copy link
Copy Markdown
Member

@teoxoy teoxoy commented Nov 14, 2023

Adds pipeline constants plumbing to the pipeline-constants feature branch.

Tracking meta issue: #4484.

@teoxoy teoxoy requested review from a team November 14, 2023 14:11
@teoxoy teoxoy requested a review from a team as a code owner November 14, 2023 14:11
Copy link
Copy Markdown
Member

@ErichDonGubler ErichDonGubler left a comment

Choose a reason for hiding this comment

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

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.

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.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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)]

cc @cwfitzgerald

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member Author

@teoxoy teoxoy Nov 16, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member Author

@teoxoy teoxoy Nov 16, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm definitely leaning array - hash maps are hard to construct inline and we don't really need deduplication behavior at the edge.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member Author

@teoxoy teoxoy Nov 16, 2023

Choose a reason for hiding this comment

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

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)]

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@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?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

idk really, maybe we can clarify it once someone runs into this?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

@jimblandy jimblandy left a comment

Choose a reason for hiding this comment

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

This definitely needs a big honkin' CHANGELOG.md entry, since it breaks pretty much everybody.

@teoxoy
Copy link
Copy Markdown
Member Author

teoxoy commented Nov 15, 2023

This definitely needs a big honkin' CHANGELOG.md entry, since it breaks pretty much everybody.

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 teoxoy force-pushed the pipeline-constants branch from c3f93e6 to e3d4779 Compare November 15, 2023 14:01
@teoxoy
Copy link
Copy Markdown
Member Author

teoxoy commented Nov 15, 2023

Copy link
Copy Markdown
Member

@ErichDonGubler ErichDonGubler left a comment

Choose a reason for hiding this comment

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

All concerns I had during my PR LGTM. I can't speak for @jimblandy's concerns, tho.

@teoxoy teoxoy requested a review from jimblandy November 16, 2023 12:37
@teoxoy
Copy link
Copy Markdown
Member Author

teoxoy commented Nov 16, 2023

@crowlKats do the deno changes look good?

Copy link
Copy Markdown
Collaborator

@crowlKats crowlKats left a comment

Choose a reason for hiding this comment

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

Deno part LGTM

@teoxoy teoxoy merged commit e3d4779 into gfx-rs:pipeline-constants Nov 20, 2023
@teoxoy
Copy link
Copy Markdown
Member Author

teoxoy commented Nov 20, 2023

I managed to do it again... 🤦‍♂️😅 (#4677 (comment))

I need to get used to this workflow.

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.

5 participants