Missing and incorrect uniform block types

[brianchirls]

Hi, thanks for this very helpful library. I found a few issues with the type annotations:

1. uniformBlockSpec is mis-typed as uniformBlockSpace on ProgramInfo
   Here. This shows up as an incorrect property name in the documentation and in code editor auto-complete.

2. UniformBlockSpec is missing blockSpecs: { [key: string]: BlockSpec } property.
   Looks like this exists in an old file but is no longer in the source, even though the property is still created by createUniformBlockSpecFromProgram. I can't find the commit where it was removed.

3. UniformData is missing a couple properties: name and used
   Both are created by createUniformBlockSpecFromProgram. name seems to be optional, since it's not added to built-in uniforms. Not sure if that's deliberate though.

[greggman]

Thanks.

38a6009

UniformData.used is not a thing and built-in uniforms are skipped (or should be)

twgl.js/src/programs.js

Line 1069 in e48e3e1

if (isBuiltIn(uniformInfo)) {

[brianchirls]

Thank you for the fix!

You're right; my mistake. used is on BlockSpec, not UniformData.

I'm actually running into some problems with that built-in check. I was gonna do some more research and file a separate issue. Why is that a break rather than continue? and shouldn't it skip adding the built-in rather than add it and leave it unnamed?

[greggman]

Why is that a break rather than continue?

That would be a bug 😅

Do you have a shader where a built in shows up? I've never actually seen one. It would be helpful to test.

[brianchirls]

Yes, I do, but it will take me some time to isolate a test case for you.

I'm working with the WEBGL_multi_draw extension, so the built-in is gl_DrawID. There are some uniforms that show up after the break, including the arrays that make a uniform for every single entry, which is unmanageable as is. I'd like to be able to offer a recommendation for how to handle it, and that's where more I need to do more research.

[greggman]

Thanks! I was able to repo the issue locally and write a kind of test for it. The test failed until I fixed things though it only failed by luck that gl_DrawID was not the last uniform.

I pushed up a new version

2340129

[greggman]

To be clear, I think, but not 100% sure, there is no reason to remove the built in from the uniform data being gathered. It won't be used and fixing the code so it skips getting info for that uniform is a minor headache (not a big deal but the loop index won't match uniform index).

I know calling getUniformLocation on a built in is illegal which I always found to be a strange part of the spec. Why return a built-in as a uniform if you can't actually set it. Maybe there's a reason

[brianchirls]

This works much better!

Agreed about the strange aspect of the spec.

This is a big help. I'm going to try to get multi-draw and UBOs working together in a non-trival case, and if I can suggest a good way to handle the massive arrays here, I'll file another issue. Thanks again.

[greggman]

I think I see what you're getting at. If you make

uniform Foo {
  mat4 m[4];
}

You'll get one setter m but if you have a struct

struct Bar {
  mat4 m;
};
uniform Foo {
  Bar b[4];
};

You'll get b[0].m, b[1].m, b[2].m, b[3].m

I'm open to suggestions but off the top of my head, at some point if you're doing something too complicated you should consider maybe twgl isn't the solution? Or, just update the data yourself via someUniformBlockInfo.asFloat ? I mean, in the example above 4 structs of a mat4 having one setter might be fine but as soon as you add more fields to Bar it's just too much work?

If Bar was

struct Bar {
  float f;
  vec2 v2;
  mat4 m;
};

I guess you could try to make a setter that took an object

twgl.setBlockUniforms(someUniformBlockInfo, {
  "b": [
     { f: 11, v2: [ 21, 21], m: [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16,], },
     { f: 11, v2: [ 21, 21], m: [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16,], },
     { f: 11, v2: [ 21, 21], m: [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16,], },
     { f: 11, v2: [ 21, 21], m: [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16,], },
  ],
});

Sounds like overkill to me. If you're doing something that complicated maybe you want to put the values in the buffer yourself?

Just thinking out loud. I'm not sure what you were doing, only guessing based on your comments. If there as a simple and not too large solution I'm open to it.

[brianchirls]

Yes, I think we're on the same page here. I think I'm gonna implement my own solution in place of createUniformBlockInfoFromProgram for now that will probably look like a setter that takes an index into the array as well as the value.

I'm working with an example right now that has ~2000 entries in uniformData, and it's a lot of setter function to have to look up by string keys. If I come up with something that works well, I'll try to share it here.

[greggman]

So I got curious. It wasn't too hard to write a setter like the one I described above. I only wrote it for regular uniforms so far but it works like this. Given uniforms like this

    struct Extra {
      float f;
      vec4 v4;
      vec3 v3;
    };
    struct Light {
      float intensity;
      vec4 color;
      float nearFar[2];
      Extra extra[2];
    };
    uniform Light lights[2];

You can set them like this

twgl.setUniformTree(programInfo, {
  lights:[
    { intensity: 1.5, color: [1, 0, 0, 1], nearFar: [0.1, 1.0], extra: [ f: 0.1, v4: [1.2, 3.4, 1.2, 1.4], v3: [0, 1, 2], },
    { intensity: 2.3, color: [0, 0, 1, 1], nearFar: [0.1, 1.0], extra: [ f: 0.7, v4: [3.1, 7.9, 0.6, 5.8], v3: [4, 5, 6], },
  ],
});

I don't think it would be hard to make it work for uniform blocks too.

But ..... is that better? For example, if you want to set only the color of the 2nd light, with setUniform you'd do

twgl.setUniform(programInfo, {
  'lights[1].color': [0, 1, 0, 1],
});

Where as I don't think there is any way to do that with twgl.setUniformTree. I guess I could make this work

twgl.setUniformTree(programInfo, {
  lights:[
    ,   // undefined entry for light[0]
    { color: [0, 1, 0, 1], },
  ],
});

I'm not sure it's important to be able to set individual things. I can certainly keep your own object around

const lightUniforms = {
  lights:[
    { intensity: 1.5, color: [1, 0, 0, 1], nearFar: [0.1, 1.0], extra: [ f: 0.1, v4: [1.2, 3.4, 1.2, 1.4], v3: [0, 1, 2], },
    { intensity: 2.3, color: [0, 0, 1, 1], nearFar: [0.1, 1.0], extra: [ f: 0.7, v4: [3.1, 7.9, 0.6, 5.8], v3: [4, 5, 6], },
  ],
};

and then set just the color of the 2nd light with

lightUniforms.lights[1].color = [0, 1, 0, 1];

...

twgl.setUniformTree(programInfo, lightUniforms);

#187

[greggman]

Well I ended up checking it in for both regular uniforms and uniform blocks. Unlike the example above you just use setUniforms and setBlockUniforms like normal except you can pass them an object with the fields and they'll update whatever fields you specify.

You can now specify any valid path so for example, given

    struct Extra {
      float f;
      vec4 v4;
      vec3 v3;
    };
    struct Light {
      float intensity;
      vec4 color;
      float nearFar[2];
      Extra extra[2];
    };
    uniform Light lights[2];

If you just want to update the second light you can do

twgl.setUinform(progInfo, {
  "lights[1]": { intensity: 1.5, color: [1, 0, 0, 1], nearFar: [0.1, 1.0], extra: [ f: 0.1, v4: [1.2, 3.4, 1.2, 1.4], v3: [0, 1, 2], },
});

But, ATM, not for leafs. In other words you can't do this

twgl.setUinform(progInfo, {
  "lights[1].nearFar[1]": 123,     // BAD, because nearFar is a leaf
  "lights[0].nearFar": [45, 76],   // GOOD
});

That's been true forever with twgl. If you had

uniform float someFloat[3];
uniform vec4 someVec4;

You had to provide all 3 values for someFloat

twgl.setUniforms(prgInfo, { someFloat: [11, 22, 33] })

There was no way to set just someFloat[1] or someFloat[2]. Just like there's no way to just set someVec4.y

[brianchirls]

Wow, that's awesome. I will check it out. Thanks again!

[brianchirls]

Looks like you have a small code syntax typo in README:

twgl.setUniforms(progInfo, {
  lights[1]: { intensity: 5.0, shininess: 100, color: [1, 0, 0, 1] },
});

should be:

twgl.setUniforms(progInfo, {
  'lights[1]': { intensity: 5.0, shininess: 100, color: [1, 0, 0, 1] },
});

Specifically, the property key should be a string.

[greggman]

thanks! fixed