Support dynamic offsets #131

cloudhead · 2019-04-23T19:31:29Z

Closes #125

Adds support for dynamic offsets when setting a bind group.

I haven't actually tried if this works with my use-case, but it's fairly straight forward. 😅

cloudhead · 2019-04-23T19:33:51Z

wgpu-rs/src/lib.rs

@@ -776,8 +776,8 @@ impl CommandEncoder {
 }

 impl<'a> RenderPass<'a> {
-    pub fn set_bind_group(&mut self, index: u32, bind_group: &BindGroup) {
-        wgn::wgpu_render_pass_set_bind_group(self.id, index, bind_group.id);
+    pub fn set_bind_group(&mut self, index: u32, bind_group: &BindGroup, offsets: &[u32]) {


The API spec states an optional sequence of u64, however gfx-hal expects u32, so that's why I used that here. I also opted for a slice instead of an Option, as that seems more idiomatic, and the underlying API also expects a slice.

slice is good here, no need for Option

kvark · 2019-04-23T19:46:32Z

Thank you!
We'll need to validate that offsets provided are 1) correspond to dynamic buffer descriptors, and 2) are within the range of those buffers, but that can be done later.

bors r+

Note: travis seems to experience outage on macOS, unfortunately

131: Support dynamic offsets r=kvark a=cloudhead Closes #125 Adds support for dynamic offsets when setting a bind group. I haven't actually tried if this works with my use-case, but it's fairly straight forward. 😅 Co-authored-by: Alexis Sellier <alexis@cloudhead.io>

bors · 2019-04-23T19:57:17Z

Build failed

continuous-integration/travis-ci/push

cloudhead · 2019-04-23T22:24:21Z

I'll look into this - indeed the descriptor type has to be set to UniformBufferDynamic, and I don't think you want to default to that for all Uniform buffers. If I understand correctly, the offsets list will offset into all dynamic buffers, in order. So if the bind group has 5 bindings and 2 of those are dynamic uniform buffers, then the offsets slice will contain 2 elements.

kvark · 2019-04-23T22:53:40Z

Yes, that is correct.

…

On Apr 23, 2019, at 18:24, Alexis Sellier ***@***.***> wrote: I'll look into this - indeed the descriptor type has to be set to UniformBufferDynamic, and I don't think you want to default to that for all Uniform buffers. If I understand correctly, the offsets list will offset into all dynamic buffers, in order. So if the bind group has 5 bindings and 2 of those are dynamic uniform buffers, then the offsets slice will contain 2 elements. — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

kvark · 2019-04-25T15:23:39Z

wgpu-native/src/binding_model.rs

@@ -25,6 +25,7 @@ pub enum BindingType {
    Sampler = 1,
    SampledTexture = 2,
    StorageBuffer = 3,
+    UniformBufferDynamic = 4,


can we add StorageBufferDynamic please since we are at it?

cloudhead · 2019-04-26T18:08:12Z

I'm making progress, however the one issue I'm encountering now is that Binder::provide_entry is caching the uniform buffer, and not re-setting the bind group after I set it with a different offset. I'm going to look more deeply into it, but I suspect the solution might be to not cache dynamic buffers, and always treat them as Provision::Changed. Thoughts?

cloudhead · 2019-04-26T18:31:13Z

Put differently: bind groups with at least one dynamic buffer in them always get re-set. Otherwise it looks like we have to keep track of the offsets in BindGroupPair, for example.

kvark · 2019-04-26T18:33:03Z

Yes, I agree. let's always re-set bind groups containing any dynamic offsets

wgpu-native/src/device.rs

kvark

Looks great to me, thank you!
Might need a bit of squashing, so that each individual commit is buildable.

kvark · 2019-04-26T19:53:29Z

wgpu-native/src/device.rs

                    binding_model::BindingType::StorageBuffer => {
                        device.limits.min_storage_buffer_offset_alignment
                    }
-                    _ => panic!("Mismatched buffer binding for {:?}", decl),
+                    binding_model::BindingType::StorageBufferDynamic => {


nit: merge the arm with the one above?

kvark · 2019-04-26T19:53:49Z

wgpu-rs/src/lib.rs

@@ -776,8 +776,8 @@ impl CommandEncoder {
 }

 impl<'a> RenderPass<'a> {
-    pub fn set_bind_group(&mut self, index: u32, bind_group: &BindGroup) {
-        wgn::wgpu_render_pass_set_bind_group(self.id, index, bind_group.id);
+    pub fn set_bind_group(&mut self, index: u32, bind_group: &BindGroup, offsets: &[u32]) {


slice is good here, no need for Option

Closes gfx-rs#125 * Add support for dynamic offsets when setting a bind group. * Add new binding types: `UniformBufferDynamic` and `StorageBufferDynamic`

cloudhead · 2019-04-26T20:50:07Z

Merged the arms and squashed to one commit 👍

kvark · 2019-04-26T22:34:40Z

Bors r+

…

On Apr 26, 2019, at 16:50, Alexis Sellier ***@***.***> wrote: Merged the arms and squashed to one commit 👍 — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

131: Support dynamic offsets r=kvark a=cloudhead Closes #125 Adds support for dynamic offsets when setting a bind group. I haven't actually tried if this works with my use-case, but it's fairly straight forward. 😅 Co-authored-by: Alexis Sellier <alexis@monadic.xyz>

bors · 2019-04-26T22:54:41Z

Build succeeded

continuous-integration/travis-ci/push

Fix `Subscription` missing in `iced_web`

cloudhead commented Apr 23, 2019

View reviewed changes

cloudhead force-pushed the f/dynamic-offsets branch from 7727098 to 5c8ce4e Compare April 24, 2019 19:05

kvark reviewed Apr 25, 2019

View reviewed changes

cloudhead force-pushed the f/dynamic-offsets branch from 9e873e2 to 25bbf96 Compare April 26, 2019 19:38

cloudhead commented Apr 26, 2019

View reviewed changes

wgpu-native/src/device.rs Show resolved Hide resolved

cloudhead force-pushed the f/dynamic-offsets branch from 1543ab2 to 6ee8cb6 Compare April 26, 2019 19:45

kvark approved these changes Apr 26, 2019

View reviewed changes

cloudhead force-pushed the f/dynamic-offsets branch from e06a50d to 81acd73 Compare April 26, 2019 20:13

Support dynamic offsets

c468840

Closes gfx-rs#125 * Add support for dynamic offsets when setting a bind group. * Add new binding types: `UniformBufferDynamic` and `StorageBufferDynamic`

cloudhead force-pushed the f/dynamic-offsets branch from 81acd73 to c468840 Compare April 26, 2019 20:21

bors bot merged commit c468840 into gfx-rs:master Apr 26, 2019

cloudhead deleted the f/dynamic-offsets branch April 27, 2019 06:41

Patryk27 pushed a commit to Patryk27/wgpu that referenced this pull request Nov 23, 2022

[glsl-new] disable constant expression (gfx-rs#131)

34cf97c

RandyMcMillan pushed a commit to RandyMcMillan/wgpu that referenced this pull request Jun 19, 2024

Merge pull request gfx-rs#131 from hecrj/fix/web-subscription-missing

9cf61fb

Fix `Subscription` missing in `iced_web`

Support dynamic offsets #131

Support dynamic offsets #131

Uh oh!

Conversation

cloudhead commented Apr 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cloudhead Apr 23, 2019

Choose a reason for hiding this comment

Uh oh!

kvark Apr 26, 2019

Choose a reason for hiding this comment

Uh oh!

kvark commented Apr 23, 2019

Uh oh!

bors bot commented Apr 23, 2019

Build failed

Uh oh!

cloudhead commented Apr 23, 2019

Uh oh!

kvark commented Apr 23, 2019 via email

Uh oh!

kvark Apr 25, 2019

Choose a reason for hiding this comment

Uh oh!

cloudhead Apr 26, 2019

Choose a reason for hiding this comment

Uh oh!

cloudhead commented Apr 26, 2019

Uh oh!

cloudhead commented Apr 26, 2019

Uh oh!

kvark commented Apr 26, 2019

Uh oh!

Uh oh!

kvark left a comment

Choose a reason for hiding this comment

Uh oh!

kvark Apr 26, 2019

Choose a reason for hiding this comment

Uh oh!

kvark Apr 26, 2019

Choose a reason for hiding this comment

Uh oh!

cloudhead commented Apr 26, 2019

Uh oh!

kvark commented Apr 26, 2019 via email

Uh oh!

bors bot commented Apr 26, 2019

Build succeeded

Uh oh!

Uh oh!

cloudhead commented Apr 23, 2019 •

edited

Loading