Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

[Impeller] Enforce a minimum stroke width#35576

Merged
auto-submit[bot] merged 2 commits into
flutter-team-archive:mainfrom
flar:impeller-min-stroke-width
Aug 23, 2022
Merged

[Impeller] Enforce a minimum stroke width#35576
auto-submit[bot] merged 2 commits into
flutter-team-archive:mainfrom
flar:impeller-min-stroke-width

Conversation

@flar

@flar flar commented Aug 21, 2022

Copy link
Copy Markdown
Contributor

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.

@flutter-dashboard

Copy link
Copy Markdown

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.

@flar
flar requested a review from bdero August 21, 2022 05:27
@flar

flar commented Aug 21, 2022

Copy link
Copy Markdown
Contributor Author

To expand upon the "more complete" solution described in the description, the vertex program does the following operations:

offset = normal * size
output = mvp(position + offset)

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:

output = mvp(position) + stroke_mvp(normal)

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.

@chinmaygarde chinmaygarde changed the title Enforce a minimum stroke width in Impeller [Impeller] Enforce a minimum stroke width Aug 22, 2022
@zanderso

Copy link
Copy Markdown
Contributor

@bdero friendly ping. I patched this in and observed that the hairlines in the demo app are drawn.

@zanderso
zanderso requested a review from chinmaygarde August 23, 2022 18:04
@flar

flar commented Aug 23, 2022

Copy link
Copy Markdown
Contributor Author

I was hoping to get eyes from @bdero on the feasibility of the alternate solutions.

@flar

flar commented Aug 23, 2022

Copy link
Copy Markdown
Contributor Author

Also, I should add a test...

@flar flar added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 23, 2022
@auto-submit

auto-submit Bot commented Aug 23, 2022

Copy link
Copy Markdown
Contributor
  • The status or check suite Linux Android AOT Engine has failed. Please fix the issues identified (or deflake) before re-applying this label.

@auto-submit auto-submit Bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Aug 23, 2022
@zanderso

Copy link
Copy Markdown
Contributor

The presubmit failure was an infra issue, so I clicked re-run.

@flar

flar commented Aug 23, 2022

Copy link
Copy Markdown
Contributor Author

The presubmit failure was an infra issue, so I clicked re-run.

So did I about an hour ago. Did it recur?

@zanderso

Copy link
Copy Markdown
Contributor

Yeah, looks like it's still happening. I suspect it's related to flutter/flutter#110112

@flar

flar commented Aug 23, 2022

Copy link
Copy Markdown
Contributor Author

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...

@zanderso

Copy link
Copy Markdown
Contributor

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.

@flar
flar force-pushed the impeller-min-stroke-width branch from 351aec1 to b2b18f6 Compare August 23, 2022 22:37
@flar flar added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 23, 2022
@auto-submit
auto-submit Bot merged commit 2ecccf1 into flutter-team-archive:main Aug 23, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 24, 2022
zanderso pushed a commit to flutter/flutter that referenced this pull request Aug 24, 2022
* 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)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

autosubmit Merge PR when tree becomes green via auto submit App e: impeller needs tests

Development

Successfully merging this pull request may close these issues.

3 participants