Add support for fetching array uniforms by name#180647
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces support for fetching array uniforms by name, which simplifies setting uniform arrays significantly. The changes are well-structured, introducing a UniformArray class and refactoring uniform index retrieval for better clarity. My review includes a few suggestions:
- A correction for a copy-paste error in the documentation.
- A proposal to add a
lengthproperty to theUniformArrayinterface on the web for API consistency. - A recommendation to expand test coverage for the CanvasKit implementation to match the thoroughness of the native tests.
11ed206 to
8256ed6
Compare
gaaclarke
left a comment
There was a problem hiding this comment.
lgtm once @harryterkelsen is happy!
harryterkelsen
left a comment
There was a problem hiding this comment.
Thanks for your patience and for contributing code to the Flutter engine. I have some comments
| final program = CkFragmentProgram.fromBytes('test', data); | ||
| shader = program.fragmentShader() as CkFragmentShader; | ||
| }); | ||
| test('getUniformVec2', () { |
There was a problem hiding this comment.
These tests all check that setting values on the by-name float arrays doesn't crash. I think we should also check that after setting the values, if we get the float arrays again, the values are set to what we just set them to. Otherwise, these tests will pass even if set() is a no-op.
There was a problem hiding this comment.
We don't have a mechanism to do this (read uniform values) in the current fragmentshader API.
It wouldn't be too hard to write a getFloat(int index) API and expand the UniformXSlot API based on that, but. I'd want to check with the rest of the engine team before moving forward with that kind of change.
@gaaclarke for thoughts.
There was a problem hiding this comment.
Checking the result isn't all that helpful because what really matters is that that value is propagated to the GPU. That would require golden tests. These classes were designed to be things you create in the widget tree declaratively, not storage classes so I think adding getters to them is contrary to how we want them used.
It would be nice to have a golden tests that runs through this. I'm not sure if there is one off the top of my mind. Eventually as the framework adopts this we'll get some. You can file an issue to follow up since this meets the standard set by the alternative API.
There was a problem hiding this comment.
Oh, actually we do have tests like golden tests, check out FragmentShader simple shader renders correctly. You can do something like that.
There was a problem hiding this comment.
I'm not sure if goldens are the best solution for testing initial implementation here, but they should serve as kind of a regression test?
|
I created a PR to move the existing tests to web_ui and committed the existing test shaders to github to make things less bad here. Will rebase on top of that #180910 |
|
autosubmit label was removed for flutter/flutter/180647, because - The status or check suite Linux analyze has failed. Please fix the issues identified (or deflake) before re-applying this label. |
|
autosubmit label was removed for flutter/flutter/180647, because - The status or check suite Linux linux_host_engine_test has failed. Please fix the issues identified (or deflake) before re-applying this label. |
Adds support for fetching objects representing array Uniforms of type
float, vec2, vec3, and vec4. Happy new year!
Before this change, setting an array of uniforms was verbose.
```glsl
uniform vec3[2] uColors;
```
```dart
shader.setFloat(0, 1.0);
shader.setFloat(1, 0.0);
shader.setFloat(2, 1.0);
shader.setFloat(3, 0.0);
shader.setFloat(4, 1.0);
shader.setFloat(5, 0.0);
```
After this change, we have:
```glsl
uniform vec3[2] uColors;
```
```dart
final colors = shader.getUniformVec3Array("uColors");
colors[0].set(1.0, 0.0, 1.0);
colors[1].set(0.0, 1.0, 0.0);
```
- Adds a class UniformArray to represent uniform arrays in Dart.
- Adds a bunch of methods that construct these arrays for various
datatypes
- Adds tests
- Also removes some of the awkwardness in the _getUniformFloatIndex
function by replacing that function entirely
## Pre-launch Checklist
- [x ] I read the [Contributor Guide] and followed the process outlined
there for submitting PRs.
- [ x] I read the [Tree Hygiene] wiki page, which explains my
responsibilities.
- [ x] I read and followed the [Flutter Style Guide], including
[Features we expect every widget to implement].
- [ x] I signed the [CLA].
- [ x] I listed at least one issue that this PR fixes in the description
above.
- [ x] I updated/added relevant documentation (doc comments with `///`).
- [ x] I added new tests to check the change I am making, or this PR is
[test-exempt].
- [ x] I followed the [breaking change policy] and added [Data Driven
Fixes] where supported.
- [ x] All existing and new tests are passing.
Adds support for fetching objects representing array Uniforms of type
float, vec2, vec3, and vec4. Happy new year!
Before this change, setting an array of uniforms was verbose.
```glsl
uniform vec3[2] uColors;
```
```dart
shader.setFloat(0, 1.0);
shader.setFloat(1, 0.0);
shader.setFloat(2, 1.0);
shader.setFloat(3, 0.0);
shader.setFloat(4, 1.0);
shader.setFloat(5, 0.0);
```
After this change, we have:
```glsl
uniform vec3[2] uColors;
```
```dart
final colors = shader.getUniformVec3Array("uColors");
colors[0].set(1.0, 0.0, 1.0);
colors[1].set(0.0, 1.0, 0.0);
```
- Adds a class UniformArray to represent uniform arrays in Dart.
- Adds a bunch of methods that construct these arrays for various
datatypes
- Adds tests
- Also removes some of the awkwardness in the _getUniformFloatIndex
function by replacing that function entirely
## Pre-launch Checklist
- [x ] I read the [Contributor Guide] and followed the process outlined
there for submitting PRs.
- [ x] I read the [Tree Hygiene] wiki page, which explains my
responsibilities.
- [ x] I read and followed the [Flutter Style Guide], including
[Features we expect every widget to implement].
- [ x] I signed the [CLA].
- [ x] I listed at least one issue that this PR fixes in the description
above.
- [ x] I updated/added relevant documentation (doc comments with `///`).
- [ x] I added new tests to check the change I am making, or this PR is
[test-exempt].
- [ x] I followed the [breaking change policy] and added [Data Driven
Fixes] where supported.
- [ x] All existing and new tests are passing.
Adds support for fetching objects representing array Uniforms of type float, vec2, vec3, and vec4. Happy new year!
Before this change, setting an array of uniforms was verbose.
After this change, we have:
Pre-launch Checklist
///).