[Impeller] Support 'matrix' parameter for color sources #35400
Conversation
ColdPaleLight
changed the title
ColdPaleLight
force-pushed
the
color_source_matrix
branch
from
45dfb5c to
9995a75
Compare
bdero
left a comment
bdero
left a comment
There was a problem hiding this comment.
This is looking great, thanks! Just a few comments about shader lib conventions and vertex shaders.
| void main() { | ||
| gl_Position = vert_info.mvp * vec4(position, 0.0, 1.0); | ||
| v_texture_coords = texture_coords; | ||
| interpolated_vertices = position; |
There was a problem hiding this comment.
Nit: I know this convention wasn't introduced by this PR, but here and everywhere else: Maybe just call this v_position instead of interpolated_vertices? I've been replacing "vertex" in shaders lately with less ambiguous terms.
| #ifndef TRANSFORM_GLSL_ | ||
| #define TRANSFORM_GLSL_ | ||
|
|
||
| vec2 transform(mat4 matrix, vec2 point) { |
There was a problem hiding this comment.
Please follow the naming convention present in blending.glsl for utilities with type variation. I'd recommend: IPVec2TransformPosition to distinguish from, say, IPVec3TransformDirection, which would be similar except with 0 for the homogeneous coordinate. Also, please add a doc string for all functions added to the shader library.
There was a problem hiding this comment.
Renaming is complete.
Would you mind giving me a doc string? Since I'm not a native speaker, I'm not sure how to describe it.
There was a problem hiding this comment.
Sure, something like this would be good:
/// Returns the Cartesian coordinates of `position` in `transform` space.
IPVec2TransformPosition(mat4 transform, vec2 position) {
|
|
||
| void main() { | ||
| float len = length(interpolated_vertices - gradient_info.center); | ||
| vec2 transformed_vertices = transform(gradient_info.matrix, interpolated_vertices); |
There was a problem hiding this comment.
Here and everywhere else, this transform could just be done in the vertex shader before throwing it over to the fragment shader.
There was a problem hiding this comment.
Also, it seems the gradient vertex shaders have turned out to be identical. If they continue being identical, consider deleting radial_gradient_fill and sweep_gradient_fill and just use the gradient_fill vertex shader for all of the pipelines.
ColdPaleLight
added
the
autosubmit
2 participants
fix flutter/flutter#109384
Pre-launch Checklist
writing and running engine tests.
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.