[Impeller] texture coordinates implementation #39781
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
still needs effect transform |
|
I should double check, but I suppose a reasonable way to interpret the texture coordinates for color sources without sizes is to take the maximum coverage from the texture coordinates. I'll do some experiments with Skia. |
jonahwilliams
force-pushed
the
texture_coordinates_2
branch
from
0232c60 to
888feb2
Compare
|
Added fast path for ImageShaders and use texture coordinate extent for other shader types. Not going to spend much more time on that, I don't think its important. Need to do effect transform still. |
| (texture_coord.y() - origin.y) / size.height); | ||
| // From experimentation we need to clamp these values to < 1.0 or else | ||
| // there can be flickering. | ||
| vertex_data[i] = { |
There was a problem hiding this comment.
Observed flickering while scrolling when using vertices + texture coordinates and a gradient in a subpass. Geometry and stencil were correct
jonahwilliams
force-pushed
the
texture_coordinates_2
branch
from
273a005 to
ef491d7
Compare
|
Okay, i have hacked in support for the effect transform for textures! |
jonahwilliams
force-pushed
the
texture_coordinates_2
branch
from
ef491d7 to
0fef90e
Compare
|
I removed the position_uv vertex shader because it doesn't do anything that the texture_fill.vert shader doesn't do |
jonahwilliams
force-pushed
the
texture_coordinates_2
branch
from
0fef90e to
e109ec4
Compare
impeller/aiks/canvas.cc
Outdated
| if (!vertices->HasVertexColors() && | ||
| (!vertices->HasTextureCoordinates() || | ||
| (vertices->HasTextureCoordinates() && | ||
| paint.color_source_type == Paint::ColorSourceType::kImage))) { |
There was a problem hiding this comment.
nit: I think this and the if below would read a little clearer if there was a method on Vertices to help sort these things out.
There was a problem hiding this comment.
There's not really a good place to put this. it depends on the color source type which can't be used outside of aiks, and the vertices are a geometry object defined in entity. It can't go on the contents because it isn't created yet.
There was a problem hiding this comment.
moved this to a static helper function
| .texture_coords = Point(std::clamp(uv.x, 0.0f, 0.995f), | ||
| std::clamp(uv.y, 0.0f, 0.995f)), |
There was a problem hiding this comment.
Can the second constant be 1.0 - kEhCloseEnough?
There was a problem hiding this comment.
(that would avoid introducing a new epsillon, which this feels like)
There was a problem hiding this comment.
Yeah good idea. I was actually surprised a bit by this behavior, maybe I'm actually doing the math wrong?
impeller/tools/malioc_diff.py
Outdated
| # Write the new results to the file given by --before, then exit. | ||
| with open(args.before, 'w') as file: | ||
| json.dump(after_json, file, sort_keys=True) | ||
| json.dump(after_json, file, sort_keys=True, indent=2) |
There was a problem hiding this comment.
Splitting each line out makes the diff view a lot easier to read
There was a problem hiding this comment.
reverted this for now to avoid conflicts
jonahwilliams
added
the
autosubmit
3 participants
A not-particularly-fast implementation of texture coordinates. This is missing a fairly reasonable fast path which is 1. texture coordinates + image shader + no per vertex colors should not require any subpasses.Fast path added.This implementation is a bit complicated by the fact that both the geometetry and the contents need to know the "size" of the color source that the texture coordinates are applied to. From experimentation with Skia, the extent of the gradient is used where possible, as is the size of the image - as the texture coordinates are always given in the local coordinate space.
For solid colors we should probably elide the subpass and bail, but that doesn't seem particularly important. For sweep gradients and runtime effect, its not clear what size we should use so I'm falling back to using the overall geometry coverage.
Fixes flutter/flutter#109956