Implement UberSDF lines to replace LineContents-based AA lines#188514
Implement UberSDF lines to replace LineContents-based AA lines#188514b-luk wants to merge 2 commits into
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).
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.
| 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.
2 participants
Implements UberSDF lines, which are drawn by making an UberSDF rect or roundrect and applying a translation/rotation transform.
Fixes #188329
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.