Skip to content

Add inherited Bind Groups to Render Bundles. - #5185

Open
toji wants to merge 4 commits into
gpuweb:mainfrom
toji:inherited-bind-groups
Open

Add inherited Bind Groups to Render Bundles.#5185
toji wants to merge 4 commits into
gpuweb:mainfrom
toji:inherited-bind-groups

Conversation

@toji

@toji toji commented May 1, 2025 •
edited by kainino0x
Loading

Copy link
Copy Markdown
Member

WIP change to add inherited bind groups. I'd appreciate some feedback!

The bit I feel the most uncertain about at this point is the kinda hand-wavy "placeholder bind group" concept. I'm not sure it holds up very well under rigorous examination of all uses of bind groups, but I'm also not sure how to address it better without creating a new branch in effectively every encoder algorithm that touches bind groups saying "But IF it's an inherited group do this separate thing instead with the layout..." ☹️

#5171

@toji
toji requested review from jimblandy and kainino0x May 1, 2025 22:32
@github-actions

github-actions Bot commented May 1, 2025 •
edited by mehmetoguzderin
Loading

Copy link
Copy Markdown
Contributor

Previews, as seen when this build job started (2cb07ab):
WebGPU webgpu.idl | Explainer | Correspondence Reference
WGSL grammar.js | wgsl.lalr.txt

@toji toji mentioned this pull request May 2, 2025
Open
@kainino0x kainino0x added the api WebGPU API label May 5, 2025
@kainino0x kainino0x mentioned this pull request May 5, 2025
Open
@toji
toji force-pushed the inherited-bind-groups branch from dff7316 to 0bc869f Compare May 6, 2025 20:39
@toji
toji marked this pull request as ready for review May 6, 2025 21:59
@toji

toji commented May 6, 2025

Copy link
Copy Markdown
Member Author

Alright, I think this is ready for initial review. Still a few bits that are more handwavy than I'd like, but I want to get some feedback before trying to upend anything.

@kainino0x kainino0x added this to the Milestone 1 milestone May 7, 2025
Kangz
Kangz reviewed May 12, 2025
View reviewed changes
Comment thread spec/index.bs
Comment thread spec/index.bs
: <dfn>\[[dynamic_offsets]]</dfn>, of type [=ordered map=]&lt;{{GPUIndex32}}, [=list=]&lt;{{GPUBufferDynamicOffset}}&gt;&gt;, initally empty
::
The current dynamic offsets for each {{GPUBindingCommandsMixin/[[bind_groups]]}} entry.

@Kangz Kangz May 12, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe we could replace [[bind_groups]] with [[bind_groups_layouts]] or track both? This could avoid the need for placeholder bind groups as a concept. WDYT?

@toji toji May 12, 2025

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.

I wondered about that myself. I'll take a pass at it and see how complicated it would be.

Comment thread spec/index.bs
Comment thread spec/index.bs
boolean stencilReadOnly = false;

// Requires "interited-bundle-bind-groups" feature.
sequence<GPURenderBundleInheritedBindGroupEntry> inheritedBindGroups = [];

@Kangz Kangz May 12, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Alternatively we could match GPUPipelineLayoutDescriptor with sequece<GPUBindGroupLayout?>. This would force that the inherited bindgroups are a prefix of all the groups, that is you can inherit groups 0 , 1, 2 but not just 0, 2. This would be a useful way to encode in the API that the groups should be ordered from less frequently changed to more frequently changed.

@mwyrzykowski
mwyrzykowski self-requested a review May 14, 2025 15:57
@jimblandy

jimblandy commented May 14, 2025

Copy link
Copy Markdown
Contributor

@toji In JavaScript code using this feature, Is the only visible change the inclusion of an inheritedBindGroups property in the GPURenderBundleEncoderDescriptor?

@toji

toji commented May 14, 2025

Copy link
Copy Markdown
Member Author

@toji In JavaScript code using this feature, Is the only visible change the inclusion of an inheritedBindGroups property in the GPURenderBundleEncoderDescriptor?

Yes, and a new feature string. If this were core like @Kangz suggested then in would just be the inheritedBindGroups property.

@Kangz

Kangz commented May 27, 2025

Copy link
Copy Markdown
Contributor
GPU Web WG 2025-05-14 Atlantic-time
  • BJ: Given Mike's not here probably won't dive too deep, but some things worth socializing. I've written up a spec draft. The big thing is I have it as a feature, Corentin wondered if it could be core. Also, currently I have individual bind group slots inheritable, Corentin suggested it could be a contiguous sequence instead of sparse. Probably bit better for performance, about same for ergonomics.
  • CW: In WebGPU and Vulkan there's this non-normative suggestion that lower bind groups should be changed less frequently. Doesn't matter for bindless hardware, but bindful hardware would need to repack all the bindings. But this is a backward looking concept… so happy to remove that suggestion.
  • JB: Similar to secondary command buffers, not a performance win, so take the flexibility 🙂. Placeholders, having the layout certainly seems necessary for validation. Concern [with the placeholder bind group objects]: we have very few bind groups (4), constraining the flexibility by tying it to this limit?
  • BJ: For placeholder BGL, thinking about removing that with some restructuring of the validation.
  • BJ: For the limitation of the number of bind groups themselves, think it is useful for a bunch of use cases. Not so useful for Graphite with 100 textures. But for example it is useful to change a matrix or a few things per view of an object. These are the use cases that inheritance will help with. Won't help fill parametrization but at least some
  • CW: Also solves the previously described use case with external texture.
  • CW: Babylon.js has some models that are extremely static (e.g. chunk of the map) and could be in a render bundle. They can do this by updating the model-view-projection matrix in a bound bundle for example. Even this case benefits from inheritance, as each bundle doesn't have to bind the MVP matrix separately, they can just inherit it.
  • BJ: Can also modify the contents of an indirect buffer, e.g. I do this in my frustum culling sample.
  • BJ: Temperature check, is anyone opposed to this being core rather than a new feature?
  • JB: Only needs to be a feature if it's a hardware/backend limitation.

@Kangz Kangz modified the milestones: Milestone 1, Milestone 2 Oct 2, 2025

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Kangz Kangz Kangz left review comments
@kainino0x kainino0x Awaiting requested review from kainino0x
@jimblandy jimblandy Awaiting requested review from jimblandy
@mwyrzykowski mwyrzykowski Awaiting requested review from mwyrzykowski

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

api WebGPU API proposal

Projects

None yet

Milestone

Milestone 2

Development

Successfully merging this pull request may close these issues.

4 participants

@toji @jimblandy @Kangz @kainino0x