[Impeller] Implement Color blend mode#36120
Conversation
| std::min(value.z, threshold), std::min(value.w, threshold)); | ||
| } | ||
|
|
||
| Color blendColor(const Color& src, |
There was a problem hiding this comment.
Not an expert about rendering here, but I wonder whether this is efficient or not? Seems that the cost of a switch has to be performed for each and every color.
There was a problem hiding this comment.
I wouldn't worry about the performance until we have traces that show performance here is a bottleneck. That said, if we did need to change things, we could always up this function to operate over an entire vector at once.
There was a problem hiding this comment.
I wouldn't worry about the performance until we have traces that show performance here is a bottleneck.
You are right!
we could always up this function to operate over an entire vector at once
That is what I was thinking :)
| auto sample_rect = texture_coords_[i]; | ||
| auto matrix = transforms_[i]; | ||
| auto color = (colors_.size() > 0 ? colors_[i] : Color::Black()); | ||
| auto color = (colors_.size() > 0 |
There was a problem hiding this comment.
I don't believe that this is correct - for drawAtlas we blend the colors with the texture sample which must be done in the shader: https://api.flutter.dev/flutter/dart-ui/Canvas/drawAtlas.html
Blend each rectangular portion of the image specified by an entry in the rects argument with its associated entry in the colors list using the blendMode argument (if a color is specified). In this part of the operation, the image part will be considered the source of the operation and the associated color will be considered the destination.
bdero
left a comment
bdero
left a comment
There was a problem hiding this comment.
Looking good so far -- mostly comments about where stuff should live.
92c86f6 to
2e2ff17
Compare
| } | ||
| }; | ||
|
|
||
| auto generator_expected_colors = [](Color& src, |
There was a problem hiding this comment.
I would not use a closure like this to generate the expected colors. A frequent mistake made with this sorts of tests, is that someone writes some code (with a bug), copies and pastes the implementation into the test generator (with the same bug) and asserts that they are the same - which they will be, but still wrong.
Instead each test case should have a specified color that a developer could easily cross reference with an external source
bdero
left a comment
bdero
left a comment
There was a problem hiding this comment.
LGTM after addressing final comments, thanks!
d334e58 to
9695976
Compare
11e82a2 to
8ad01de
Compare
* Add color blend mode * Move Entrity::BlendMode to Color file; Add unittest for blendMode color * Fix some comments * fix comments * delete the redundancy code
5 participants
Implement blend param in DrawVertices and DrawAtlas
Association issue: flutter/flutter#108047