greggman / webgpu-utils

Some helpers for webgpu
https://greggman.github.io/webgpu-utils/
MIT License
200 stars 10 forks source link

Setting nested structure views doesn't work. #1

Closed lokokung closed 1 year ago

lokokung commented 1 year ago

I figured I'd file these issues here for now so I don't forget, (if I have time later on, might just assign this to myself too and try to fix them)...

When I was implementing some testing for the sorting stuff, I ran into 2 issues that would be nice quality of life fixes:

1) Calling StructuredView.set with nested inputs doesn't seem to work (and nothing is set). Here is an example of what I mean by it:

const structDef = makeShaderDataDefinitions(`struct S { data: array<vec2u, 2> }`);
const struct = makeStructuredView(structDef);
struct.set({ data: [[0, 1], [2, 3]] });

Naively, I would expect that to just work, but instead, it seems like the set call will fail silently and writing the arrayBuffer to the GPU will just result in 0s. I was able to workaround this by converting the last line to (something of the like):

struct.set({ data: [[0, 1], [2, 3]].flat(Infinity) });

2) Padded elements still require users to understand them. The simplest example is with vec3s. This sort of coincides with the previous issue as well. Here's an example of what I think would be the "nice" API:

const structDef = makeShaderDataDefinitions(`struct S { data: array<vec3u, 2> }`);
const struct = makeStructuredView(structDef);
struct.set({ data: [[1, 2, 3], [4, 5, 6]] });

If we use the workaround from above, this will fail as the data will be set incorrectly because vec3s are internally implemented as vec4s. So instead the workaround for this is to specify an extra element for each vec3 before the flatten (assuming we use the previous workaround), i.e.:

struct.set({ data: [[1, 2, 3, 0], [4, 5, 6, 0]].flat(Infinity) });
greggman commented 1 year ago

Thanks for the bug report!!

for 1, sadness. I thought I had tests for that :( Maybe it's certain kinds of nested structures that are failing or maybe I forgot to check actually setting them?

Hmmm: I found the test: https://github.com/greggman/webgpu-utils/blob/main/test/tests/webgpu-utils-test.js#L243

I'll try your example

for 2 I'm not sure I'm convinced. I think it would be too slow for an array of vec3 to be [[1,2,3],[4,5,6],[7,8,9]] instead of [1,2,3,0,4,5,6,0,7,8,9,0]. Maybe the setter can handle both? but I think the source should just be a Float32Array. Just like I wouldn't want a texture to be . There'd be 100x more meta data than actual data.

const textureData = [
  [ [r,g,b,a], [r,g,b,a], [r,g,b,a] ],
  [ [r,g,b,a], [r,g,b,a], [r,g,b,a] ],
  [ [r,g,b,a], [r,g,b,a], [r,g,b,a] ],
]
greggman commented 1 year ago

So I hacked something in and your examples seem to work.

I'm not sure what to change going forward though.

In the current design there is no metadata used in set. set sees either a typedarray, an array, or an object.

It basically just does

    set(data, views) {
      if (views is typedArray) {

        // we're at a leaf
        views.set(data);

      } else if (Array.isArray(views)) {

        // we have an array of some type (struct/mat4x4) but not vec3,f32 
        // so just recursively call `set`, once for each element

        data.forEach((dataElem, i) => set(dataElem, views[i]));

      } else {

        // it must be an object representing names of structure fields
        // so just recursively call `set`, once for each property

        for (const [key, value] of Object.entries(data)) {
           const view = views[key];
           if (view) {
               set(value, view)
           }

     }
}

The point is, when it gets to views.set it doesn't know if the thing it's trying to set is an f32 array, a vec3 array, etc..

My hacky solution is to trust the user. If you pass in [[1, 2, 3], [4, 5, 6]] then you must be trying to set an array of vec3. If you pass in [[1, 2], [3 ,4]] then you must be trying to set an array of vec2. You can still pass in [1, 2, 3, 4] for the first case [1, 2, 3, ., 4, 5, 6, .]

Anyway, it works for your examples

I'm not sure how much to try to make it generate errors and how much to try to make it fast. For example, I could try to pass in 3 parallel things to set.

  1. the data values
  2. the views
  3. a data description

But that's going to be measurably slower.

I could also try to augment the views with symbols or something they know what type they represent. Or I could keep a map of types or views. Those would all make it slower.

Also, it ignores mismatches. In other words, if you have this

struct {
  foo: f32,
  bar: f32,
};

and you do set({foo: 123, other: 456}) it won't complain about other. Maybe that's bad but it's intentional from WebGL days when you'd edit the shader and not want to have to edit your code just because you removed a uniform. Maybe that's bad for WebGPU though.

lokokung commented 1 year ago

Just as an update, I updated on my side and it works! Thanks for the fix for (1). Agree with you assessment about (2), so not sure what the best thing to do atm.

greggman commented 1 year ago

(2) should work too