Implement UberSDF lines to replace LineContents-based AA lines#188514
Conversation
github-actions
Bot
added
engine
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces line rendering using the UberSDF pipeline in Impeller, adding support for different stroke caps (butt, square, and round) and applying shape transforms to SDF entities. It also includes corresponding unit tests. Feedback points out a compilation issue in canvas.cc where Matrix::Invert() returns a std::optional<Matrix> but is passed directly to SetEffectTransform which expects a Matrix type.
gaaclarke
left a comment
gaaclarke
left a comment
There was a problem hiding this comment.
Code looks good to me. I want to see the goldens to double check what existing coverage we have for this. The test might be a duplicate. I know we have something like this but it may be lines with gradients on them?
It would be interesting to consider what difference we'd have if we used the sdOrientedBox sdf for these cases. Is this suboptimal to that?
With respect to deleting the old line renderer. We might want to consider keeping it since it has a lower bar for execution on mobile devices. The case against it is that we've never had enough motivation to switch it over. Let's look at the old issue that introduced it and the PR. If i remember correctly we had a nice before and after shot and these do look better in a meaningful way. Maybe it's worth finally promoting that (and fixing this bug for it).
| ASSERT_TRUE(OpenPlaygroundHere(builder.Build())); | ||
| } | ||
|
|
||
| TEST_P(AiksTest, CanRenderLines) { |
There was a problem hiding this comment.
How does this relate to our existing line drawing tests? Seems like this should have existed but maybe it was overlooked in the line rendering change. I guess we'll potentially see when the golden results come.
There was a problem hiding this comment.
We have some other tests which draw lines, but none which draw them at different opacities like this.
The various existing line drawing tests, along with this one, are kind of spread out and aren't very consistent/organized so we could probably do some refactoring to make this better organized. But that's outside the scope of this PR
| if ((renderer_.GetContext()->GetFlags().use_sdfs || | ||
| renderer_.GetContext()->GetFlags().antialiased_lines) && |
There was a problem hiding this comment.
no action required: The antialias line rendering is a lot more faster than the ubersdf. I wonder if we should keep it around since we could use that on mobile. We never have though and there is this bug in it.
There was a problem hiding this comment.
It seems as if the developer asked for AA lines and only that, then we're suddenly using the Uber code in a configuration/environment that never used to use that algorithm. My take:
If SDF flag is set, use uber.
If not and the AA flag is set, use AA lines shader.
Otherwise use MSAA lines.
OR...
If AA flag is set, use the AA lines shader.
If not and SDF flag is set, use uber.
Otherwise use MSAA lines.
The main difference is what should happen if both flags are set. Are they setting both because they want SDF if possible, but if not then at least do the AA lines? Or are they setting it because they want SDF in general, but for AA lines they want the AA line shader for performance?
Basically, are they both set because they're covering all the bases?
Or are they both set because they want AA and faster AA lines?
In either case, there should be a path through here that gets to the old AA lines code for legacy apps that only specified the AA lines flag and didn't want to buy into SDF.
There was a problem hiding this comment.
My intention is to completely remove the AA lines shader (LineContents). Here's my case. Let me know if you disagree and you really want to keep it.
1. I don't believe there's anyone that is specifically requesting this behavior.
The original flag to enable this is
iOS: FLTAntialiasLines boolean, default NO
Android: io.flutter.embedding.android.ImpellerAntialiasLines boolean, default false
These flags are not in our public docs or advertised to users anywhere. So they are completely undiscoverable.
We have a comment here suggesting that a user try it out as an experimental feature to see if it improved their line rendering. The user said that enabling this flag did not make any difference. Looking at the code sample in the OP, the use case for this user is drawing multiple lines in a Path. This case is not handled by LineContents. So I think that explains why the user did not see any difference with LineContents enabled.
As far as I know, this is the only occurrence where these flags have ever been suggested or mentioned to anyone. It did not help that user's case, so we can pretty safely assume that user isn't dependent upon this LineContents behavior.
2. It's quite buggy, which was revealed quickly after it did become enabled by default for some users.
#183809 expanded the UberSDF flag to also enable LineContents. Then eventually UberSDF was enabled by default on MacOS and Windows. Only after this point, some users would start getting LineContents by default.
Soon after that, we started getting bug reports from users: #188329 and #188911.
LineContents has some noticeable rendering issues, and these remained undiscovered until it was actually enabled by default for some users. I think this reinforces point 1 above: No one has been specifically enabling LineContents (otherwise these issues would have been noticed sooner by users).
If we did want to support LineContents for theoretical users who really do want LineContents, we would have to fix these issues.
3. LineContents is experimental
In our code, the LineContents flag comes with comments stating that it's "experimental". e.g.:
flutter/engine/src/flutter/common/settings.h
Lines 244 to 245 in e9b3f19
flutter/engine/src/flutter/shell/common/switch_defs.h
Lines 301 to 303 in e9b3f19
And in the only case where this flag was advertised to public users (#138682 (comment)), we state that it is experimental.
So removing something that is clearly experimental shouldn't be a big deal to any users who might actually intentionally want this specific behavior. And really, I don't think there are any of these users.
4. There are a lot (IMO too many) ways to render the same line-like shape.
- Tessellation-based line
- Tessellation-based thin rectangle
- Line inside a Path (tessellation based for now)
- UberSDF thin rectangle
- LineContents
These are all different ways to draw the same line-like shape. Each of them are used in different situations, but each are used frequently in common cases.
Each of them have very different rendering logic. This makes the surface area for potential bugs much larger. And we end up with unexpected inconsistencies when rendering with one method vs another. Some of the results of this are demonstrated in #188590.
I think reducing this multitude of ways to draw a line would be a good improvement. It reduces bug surface area, reduces redundant (often complex) geometric logic, and increases rendering consistency.
If we really do want to support SDF AA lines without fully enabling UberSDF, I think the better way to do that is by making a separate line shader that consists of only the line-drawing subset of UberSDF. This way it can share code with UberSDF, without requiring all of UberSDF to be enabled. And its logic would be reusing UberSDF's logic. So we wouldn't have to support an entirely different line implementation that is quite complex (see the logic in LineContents and line.frag) and error-prone (see point 2 above).
There was a problem hiding this comment.
I'm all for removing it. I wasn't involved in the original decision to add it or where the justification came from so I can't speak for that. My comments were simply based on its current presence and how that should be managed until we remove it.
There was a problem hiding this comment.
I don't think it needs a current presence. After this PR it is completely vestigial and can't be enabled. I'll have a subsequent PR to actually delete the code. As I said in the OP, the only reason I'm not deleting it in this PR is to keep PRs more focused for easier review.
|
There's something odd going on with UberSDF and thin lines. You can see some of this in the golden results, but I don't think it's directly related to this PR. This PR would exacerbate the issue by making UberSDF used for more draw calls. I will investigate this further. |
I filed #188590 and a bunch of sub-issues regarding weirdness and inconsistencies I see when drawing lines with UberSDF rectangles (like this PR will expand) vs LineContents vs tessellation. Each of these rendering methods has some pretty noticeable issues. I think we should figure out how to address these before deciding whether we want this PR to switch LineContents lines to use UberSDF rectangles. |
…ce contents transform
|
Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change). If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
I'm going to move this to a draft while we investigate the golden changes. |
|
This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
flar
left a comment
flar
left a comment
There was a problem hiding this comment.
Some minor changes might be needed.
| if ((renderer_.GetContext()->GetFlags().use_sdfs || | ||
| renderer_.GetContext()->GetFlags().antialiased_lines) && |
There was a problem hiding this comment.
It seems as if the developer asked for AA lines and only that, then we're suddenly using the Uber code in a configuration/environment that never used to use that algorithm. My take:
If SDF flag is set, use uber.
If not and the AA flag is set, use AA lines shader.
Otherwise use MSAA lines.
OR...
If AA flag is set, use the AA lines shader.
If not and SDF flag is set, use uber.
Otherwise use MSAA lines.
The main difference is what should happen if both flags are set. Are they setting both because they want SDF if possible, but if not then at least do the AA lines? Or are they setting it because they want SDF in general, but for AA lines they want the AA line shader for performance?
Basically, are they both set because they're covering all the bases?
Or are they both set because they want AA and faster AA lines?
In either case, there should be a path through here that gets to the old AA lines code for legacy apps that only specified the AA lines flag and didn't want to buy into SDF.
| renderer_.GetContext()->GetFlags().antialiased_lines) && | ||
| IsCompatibleWithSDFRendering(paint)) { | ||
| // Draw the line as a filled rectangle with width=line_length and | ||
| // height=stroke_width. |
There was a problem hiding this comment.
Given the size of this code, it might be better for readability to move it into a separate method. The old "switch to AA lines" code was small enough to leave inline, but understanding the top-level choices for rendering technique would benefit from having a skeleton in this method that redirects to a sub-method for this lengthy UberSDF setup.
| // position of the input line. | ||
|
|
||
| // Unit vector along the line. Fallback to (1, 0) if length is 0. | ||
| Point u = |
There was a problem hiding this comment.
nit, when dealing with location-less vectors I prefer the Vector2 type to Point even though they are type-wise identical.
| Point u = | ||
| line_length > 0.0f ? ((p1 - p0) / line_length) : Point(1.0f, 0.0f); | ||
| // Unit vector perpendicular to the line. | ||
| Point perp = Point(-u.y, u.x); |
There was a problem hiding this comment.
Point/Vector2 have PerpendicularRight/Left methods for quadrant rotations. It's not a very advanced operation, but the names make it more readable (even without a comment) and makes it clear which direction you are turning for understanding subsequent code.
|
|
||
| // Unit vector along the line. Fallback to (1, 0) if length is 0. | ||
| Point u = | ||
| line_length > 0.0f ? ((p1 - p0) / line_length) : Point(1.0f, 0.0f); |
There was a problem hiding this comment.
If butt cap, is the operation supposed to draw anything on zero length?
There was a problem hiding this comment.
Butt caps with zero length don't draw anything. UberSDF does handle this properly already. But based on your comment I added an early exit check above for optimization.
| // | ||
| // Color source exposes its transform inverted, so calculate (shape -> | ||
| // canvas -> color) and use it to set color source's inverse transform. | ||
| Matrix shape_to_canvas = shape_transform.value(); |
There was a problem hiding this comment.
Would this be simpler by having CSC provide an "additional transform" property that it mixes in to its transform math at the appropriate point in the equation?
There was a problem hiding this comment.
i.e. rather than getting it, modifying it, then putting it back via Get/SetIET?
There was a problem hiding this comment.
Done. I simplified this by making Paint::CreateContents() take a geometry_transform. Then I can just pass shape_transform to CreateContents() here.
| /*reuse_depth=*/reuse_depth); | ||
| } | ||
| AddRenderEntityWithFiltersToCurrentPass(entity, geometry.get(), paint, | ||
| /*reuse_depth=*/reuse_depth); |
There was a problem hiding this comment.
I know this was just a copy/paste, but reuse_depth=reuse_depth is a tautology?
| pipelines_->circle.CreateDefault(*context_, options); | ||
| if (context_->GetFlags().use_sdfs) { | ||
| if (context_->GetFlags().use_sdfs || | ||
| context_->GetFlags().antialiased_lines) { |
There was a problem hiding this comment.
This interacts with the logic that we gave feedback on above, so I'l have to rethink it depending on what is decided based on that. But, I think this just makes sure the Uber pipelines are always more frequently initialized which shouldn't hurt anything...
| if (transform_scaling.x == 0.0f || transform_scaling.y == 0.0f) { | ||
| return std::nullopt; | ||
| } |
There was a problem hiding this comment.
Has a point. min_local_size.IsFinite() can also help with that.
| min_transformed_size.height / transform_scaling.y); | ||
| Size current_size = GetSize(); | ||
| Size expanded_size = current_size.Max(min_local_size); | ||
| return MakeEllipseBounds(GetCenter(), Point(expanded_size.width * 0.5f, |
There was a problem hiding this comment.
OK, I give. Can you file a bug on the name of this constructor?
|
Also, there's enough room to do "horizontal, nearly horizontal, 45 degrees (or 30 and 60), nearly vertical, vertical" variants on the golden test. |
|
Why do the HexagonExperimentAntialiasLines lines vary so much in size from the reference golden? |
Done
The reference goldens use LineContents, which has a known issue with making lines overly thick. See my comment about the goldens in #188514 (comment) and the linked issue. All the SDF and AA lines goldens have overly thick lines. I don't know why the hexagon one is even thicker than others. It draws lines with higher stroke width than others, so the amount of over-thickening may be related to the stroke width. I don't think it's worth digging into where exactly the issue in the LineContents logic is unless we plan to support and fix LineContents. |
flar
left a comment
flar
left a comment
There was a problem hiding this comment.
Questions and a grammar nit. I'll leave it up to you, LGTM either way (except nobody wants a grammar mistake?)
| if (!GetCurrentTransform().HasPerspective2D()) { | ||
| auto [expanded, alpha_scaled_color] = ExpandRectToPixelMinimum( | ||
| rect, paint.color, GetCurrentTransform() * rect_to_line_transform, | ||
| // Don't scale alpha stroke width is 0. This draws a hairline that is |
There was a problem hiding this comment.
Missing "if" - "if stroke width is 0"?
| /// | ||
| /// If the transform has a scaling factor of zero in either dimension, | ||
| /// it cannot be expanded to the minimum size. This returns nullopt. | ||
| [[nodiscard]] constexpr std::optional<TRect<T>> ExpandToMinSize( |
| ASSERT_TRUE(OpenPlaygroundHere(builder.Build())); | ||
| } | ||
|
|
||
| TEST_P(AiksTest, CanRenderLinesWithCapsAnglesAndAlphas) { |
There was a problem hiding this comment.
Nearly all cases are rendered with Translate+Scale. Is it worth adding some test variants that also render under a Rotate'd transform?
There was a problem hiding this comment.
This test draws rotated lines in a non-rotated canvas:
builder.DrawLine(DlPoint(0.0f, 0.0f),
DlPoint(cos(angle.radians) * line_length,
sin(angle.radians) * line_length),
paint);And the thin line test draws axis-aligned lines on a rotated canvas:
So I think we have good coverage of lines that involve rotation transforms.
There was a problem hiding this comment.
The current test draws tilted lines, but it doesn't verify that the new matrix setup code works in a rotated contextual coordinate system.
The previous test is only testing thin lines so the decorations and how they relate to the lines isn't really visible. But, it would indicate that the math does work in the context of a rotated coordinate system and I don't see any reason to doubt that the cap decorations, if visible, would be off.
There was a problem hiding this comment.
Also, DrawLinesRenderCorrectly draws lines with caps in a rotated coordinate system, so that is covered too.
flutter/engine/src/flutter/impeller/display_list/aiks_dl_path_unittests.cc
Lines 614 to 623 in c7d4687
We've got bunches of tests that test various things, but they're all kind of scattered so it's not easy to pinpoint if there is a test that a covers specific case.
flutter-dashboard
Bot
removed
the
autosubmit
flutter/flutter@846664b...fc1ad95 2026-07-15 [email protected] [iOS] Migrate FlutterKeyboardInsetManager to Swift (flutter/flutter#189425) 2026-07-15 [email protected] Roll Dart SDK from 05bf153370c4 to 0c408ff6dce9 (4 revisions) (flutter/flutter#189487) 2026-07-15 [email protected] Implement UberSDF lines to replace LineContents-based AA lines (flutter/flutter#188514) 2026-07-15 [email protected] Fix stale eagerWinner reference in GestureArenaManager when rejected before arena close (flutter/flutter#187008) 2026-07-15 [email protected] Disable some Windows tests that are flaking on CI (flutter/flutter#189477) 2026-07-15 dmgr Added unified check-run user manual (flutter/flutter#189453) 2026-07-15 [email protected] Roll Skia from 88954ef8f36d to ab2410bc857c (9 revisions) (flutter/flutter#189474) 2026-07-14 [email protected] [agents] Refactor shepherd-prs skill into a pure Markdown runbook using native gh CLI (flutter/flutter#189095) 2026-07-14 [email protected] Add missing name to mirroring workflow (flutter/flutter#189439) 2026-07-14 [email protected] Add support for WASM deferred loading. (flutter/flutter#189308) 2026-07-14 [email protected] fix `templateDefaultGradleVersion` todo (flutter/flutter#189466) 2026-07-14 [email protected] Roll Fuchsia Linux SDK from oOETA0ISPouDt2xBo... to lLFbh5kFWbUGgC9Ek... (flutter/flutter#189469) 2026-07-14 [email protected] Use ServicesBinding.instance.exitApplication instead of exit(0) in multiple_windows example (flutter/flutter#189364) 2026-07-14 [email protected] Add CpuArch to the Device class (flutter/flutter#189207) 2026-07-14 [email protected] Fix space formatting in cherry-pick label for flutter_cp.dart (flutter/flutter#189463) 2026-07-14 [email protected] Roll pub packages (flutter/flutter#189454) 2026-07-14 [email protected] [flutter_tools] Format plugin example template to match dart format (flutter/flutter#188382) 2026-07-14 [email protected] Move renamed x64->ARM benchmarks out of bringup (flutter/flutter#189400) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
b-luk
added
cp: beta
This pull request is created by [automatic cherry pick workflow](https://github.com/flutter/flutter/blob/main/docs/releases/Flutter-Cherrypick-Process.md#automatically-creates-a-cherry-pick-request) Please fill in the form below, and a flutter domain expert will evaluate this cherry pick request. This is the first in a set of 3 PRs (#188821, #189224, and #188514) that will be cherry picked in order to fix a set of line-related issues when Impeller with SDF rendering is enabled. ### Issue Link: What is the link to the issue this cherry-pick is addressing? #188329 #188911 #188593 ### Impact Description: What is the impact (ex. visual jank on Samsung phones, app crash, cannot ship an iOS app)? Does it impact development (ex. flutter doctor crashes when Android Studio is installed), or the shipping of production apps (the app crashes on launch). This information is for domain experts and release engineers to understand the consequences of saying yes or no to the cherry pick. Thin ines (including lines rendered with `canvas.drawLine`, thin line-like rectangles rendered with `canvas.drawRect`, and paths drawn with `canvas.drawPath` that consist of a single line or thin rectangle) have various highly visible rendering issues: - opacity is ignored - lines with stroke width less than 1 pixel are incorrectly clamped up to be ~2 pixels wide - lines with stroke width more than 1 pixel are overly thick, with an added thickness proportional to their intended width This issue is present when impeller is enabled Impeller with SDF rendering. In the beta channel, Impeller with SDF rendering is the default when building apps for desktop platforms: macOS, Linux, and Windows. ### Changelog Description: Explain this cherry pick: * In one line that is accessible to most Flutter developers. * That describes the state prior to the fix. * That includes which platforms are impacted. See [best practices](https://github.com/flutter/flutter/blob/main/docs/releases/Hotfix-Documentation-Best-Practices.md) for examples. [flutter/188329](#188329), [flutter/188911](#188911), [flutter/188593](#188593): Multiple line rendering issues when Impeller with SDF rendering is enabled (default enabled on desktop platforms) ### Workaround: Is there a workaround for this issue? Disable Impeller, falling back to Skia rendering. ### Risk: What is the risk level of this cherry-pick? ### Test Coverage: Are you confident that your fix is well-tested by automated tests? ### Validation Steps: What are the steps to validate that this fix works? After all 3 PRs are cherry picked, verify the sample apps in the relevant issues (#188329 and #188911) render with expected opacity and with expected line widths.
This pull request is created by [automatic cherry pick workflow](https://github.com/flutter/flutter/blob/main/docs/releases/Flutter-Cherrypick-Process.md#automatically-creates-a-cherry-pick-request) Please fill in the form below, and a flutter domain expert will evaluate this cherry pick request. This is the second in a set of 3 PRs (#188821, #189224, and #188514) that will be cherry picked in order to fix a set of line-related issues when Impeller with SDF rendering is enabled. The first PR was cherry picked and merged with #189517 ### Issue Link: What is the link to the issue this cherry-pick is addressing? #188329 #188911 #188593 ### Impact Description: What is the impact (ex. visual jank on Samsung phones, app crash, cannot ship an iOS app)? Does it impact development (ex. flutter doctor crashes when Android Studio is installed), or the shipping of production apps (the app crashes on launch). This information is for domain experts and release engineers to understand the consequences of saying yes or no to the cherry pick. Thin ines (including lines rendered with `canvas.drawLine`, thin line-like rectangles rendered with `canvas.drawRect`, and paths drawn with `canvas.drawPath` that consist of a single line or thin rectangle) have various highly visible rendering issues: - opacity is ignored - lines with stroke width less than 1 pixel are incorrectly clamped up to be ~2 pixels wide - lines with stroke width more than 1 pixel are overly thick, with an added thickness proportional to their intended width This issue is present when impeller is enabled Impeller with SDF rendering. In the beta channel, Impeller with SDF rendering is the default when building apps for desktop platforms: macOS, Linux, and Windows. ### Changelog Description: Explain this cherry pick: * In one line that is accessible to most Flutter developers. * That describes the state prior to the fix. * That includes which platforms are impacted. See [best practices](https://github.com/flutter/flutter/blob/main/docs/releases/Hotfix-Documentation-Best-Practices.md) for examples. [flutter/188329](#188329), [flutter/188911](#188911), [flutter/188593](#188593): Multiple line rendering issues when Impeller with SDF rendering is enabled (default enabled on desktop platforms) ### Workaround: Is there a workaround for this issue? Disable Impeller, falling back to Skia rendering. ### Risk: What is the risk level of this cherry-pick? ### Test Coverage: Are you confident that your fix is well-tested by automated tests? ### Validation Steps: What are the steps to validate that this fix works? After all 3 PRs are cherry picked, verify the sample apps in the relevant issues (#188329 and #188911) render with expected opacity and with expected line widths.
b-luk
added
the
cp: beta
…es (#189537) This pull request is created by [automatic cherry pick workflow](https://github.com/flutter/flutter/blob/main/docs/releases/Flutter-Cherrypick-Process.md#automatically-creates-a-cherry-pick-request) Please fill in the form below, and a flutter domain expert will evaluate this cherry pick request. This is the third in a set of 3 PRs (#188821, #189224, and #188514) that will be cherry picked in order to fix a set of line-related issues when Impeller with SDF rendering is enabled. The first PR was cherry picked and merged with #189517 The second PR was cherry picked and merged with #189529 ### Issue Link: What is the link to the issue this cherry-pick is addressing? #188329 #188911 #188593 ### Impact Description: What is the impact (ex. visual jank on Samsung phones, app crash, cannot ship an iOS app)? Does it impact development (ex. flutter doctor crashes when Android Studio is installed), or the shipping of production apps (the app crashes on launch). This information is for domain experts and release engineers to understand the consequences of saying yes or no to the cherry pick. Thin ines (including lines rendered with `canvas.drawLine`, thin line-like rectangles rendered with `canvas.drawRect`, and paths drawn with `canvas.drawPath` that consist of a single line or thin rectangle) have various highly visible rendering issues: - opacity is ignored - lines with stroke width less than 1 pixel are incorrectly clamped up to be ~2 pixels wide - lines with stroke width more than 1 pixel are overly thick, with an added thickness proportional to their intended width This issue is present when impeller is enabled Impeller with SDF rendering. In the beta channel, Impeller with SDF rendering is the default when building apps for desktop platforms: macOS, Linux, and Windows. ### Changelog Description: Explain this cherry pick: * In one line that is accessible to most Flutter developers. * That describes the state prior to the fix. * That includes which platforms are impacted. See [best practices](https://github.com/flutter/flutter/blob/main/docs/releases/Hotfix-Documentation-Best-Practices.md) for examples. [flutter/188329](#188329), [flutter/188911](#188911), [flutter/188593](#188593): Multiple line rendering issues when Impeller with SDF rendering is enabled (default enabled on desktop platforms) ### Workaround: Is there a workaround for this issue? Disable Impeller, falling back to Skia rendering. ### Risk: What is the risk level of this cherry-pick? ### Test Coverage: Are you confident that your fix is well-tested by automated tests? ### Validation Steps: What are the steps to validate that this fix works? After all 3 PRs are cherry picked, verify the sample apps in the relevant issues (#188329 and #188911) render with expected opacity and with expected line widths.
…used (flutter#189619) After flutter#188514, `LineContents` can no longer be enabled. This deletes its code and its associated `antialiased_lines` flag. See flutter#188514 (comment) for some more context. ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [AI contribution guidelines] and understand my responsibilities, or I am not using AI tools. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [ ] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance. **Note**: The Flutter team is currently trialing the use of [Gemini Code Assist for GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [AI contribution guidelines]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#ai-contribution-guidelines [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
3 participants
Implements UberSDF lines, which are drawn by making an UberSDF rect or roundrect and applying a translation/rotation transform.
Fixes #188329
Fixes #188911
Fixes #188593 (by replacing and obsoleting LineContents)
After this, LineContents can be removed entirely. I will do this in a follow-up to keep this PR focused on the UberSDF line implementation.
Adds a new golden test that draws lines with varying caps/angles/opacities.
Before:
After:
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.