[Impeller] Enforce a minimum stroke width#35576
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. |
|
To expand upon the "more complete" solution described in the description, the vertex program does the following operations: Now the position does need to be transformed by the mvp, but the offset could be broken out into a second delta transform that we could manipulate without moving the anchor points of the path - as so: with stroke_mvp created as a "delta transform" (translations == 0) that both applies the size scaling and also is tweaked so that no transformed vector is smaller than 1.0. The question is - how to tweak the matrix to ensure a minimum vector length? (Also note that we can't tweak mvp itself as that would move the path to new locations, we only want to tweak the vector used to scale the normals...) If the results run slower than the existing code, we could first check the matrix for a uniform scale and instead tweak the stroke_size, and only use this modified form if the matrix was non-uniform and therefore at risk of having vectors in some directions be shorter than others. |
|
@bdero friendly ping. I patched this in and observed that the hairlines in the demo app are drawn. |
|
I was hoping to get eyes from @bdero on the feasibility of the alternate solutions. |
|
Also, I should add a test... |
|
|
The presubmit failure was an infra issue, so I clicked re-run. |
So did I about an hour ago. Did it recur? |
|
Yeah, looks like it's still happening. I suspect it's related to flutter/flutter#110112 |
It looks like the fix for that was merged 8 minutes ago so I poked re-run one more time in case... |
|
My bad. I should have looked at the failure more carefully. This PR needs to be rebased because the recipe is expecting an output that isn't being produced. The expected output will be produced after the rebase. |
351aec1 to
b2b18f6
Compare
* 68beed7 Refactor - Create `Renderer` interface for CanvasKit/HTML switching (flutter-team-archive/engine#35502) * 2ecccf1 [Impeller] Enforce a minimum stroke width (flutter-team-archive/engine#35576)
This is a simplistic approach that just ensures that the stroke width is at least greater than the square root of the matrix determinant. A more complete approach would model the stroke as a transformed (size * unit-oval) and adjust the matrix (as applied only to the offset in the shader) so that it never generated a vector shorter than a pixel.