Repository navigation
[CP-beta]UberSDF thin roundrect handling - #189529
Conversation
This fixes thin UberSDF roundrect drawing, in the same way that flutter#188821 fixed it for normal rects. Prerequisite for using UberSDF for lines. - Factors out some of the rect-upscaling code in DrawRect to an UpscaleRect helper function, so it can be shared by DrawRect and DrawRoundRect. - Adds a roundRectPixelSize function in uber_sdf.frag, similar to rectPixelSize but for round rects. - Adds unit tests for UpscaleRect, and a new golden tests for "DrawLinesWithFilledRoundRects" ## Screenshot ### DrawLinesWithFilledRoundRects Before: <img width="1024" height="800" alt="roundrect ubersdf before" src="https://github.com/user-attachments/assets/c490318f-0579-4425-82ec-ca5a7fd51d89" /> After: <img width="1024" height="800" alt="roundrect ubersdf after" src="https://github.com/user-attachments/assets/cd7bc780-fbc8-46aa-a034-6a9c1db69ad9" /> ## Video ### DrawLinesWithFilledRoundRects Before: https://github.com/user-attachments/assets/612af65d-bce5-45d2-a1aa-7636a60f3254 After: https://github.com/user-attachments/assets/4090d72c-eba9-4e28-8c8a-495bcce9b82b ## 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. - [ ] 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
|
@b-luk please fill out the PR description above, afterwards the release team will review this request. |
|
This pull request was opened from and to a release candidate branch. This should only be done as part of the official Flutter release process. If you are attempting to make a regular contribution to the Flutter project, please close this PR and follow the instructions at Tree Hygiene for detailed instructions on contributing to Flutter. Reviewers: Use caution before merging pull requests to release branches. Ensure the proper procedure has been followed. |
There was a problem hiding this comment.
Code Review
This pull request refactors the minimum size upscaling logic for SDF rendering in Impeller. It introduces a helper method Canvas::UpscaledRect to handle the upscaling of thin or small rectangles to a minimum of 1 pixel, and applies this logic to both DrawRect and DrawRoundRect paths. Additionally, it introduces a specialized roundRectPixelSize function in the uber_sdf.frag shader to improve pixel size calculations for rounded rectangles, updates Mali offline compiler statistics, and adds corresponding unit tests. There are no review comments, and I have no further feedback to provide.
e216f0f
into
flutter:flutter-3.47-candidate.0
…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.
This pull request is created by automatic cherry pick workflow
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 withcanvas.drawRect, and paths drawn withcanvas.drawPaththat consist of a single line or thin rectangle) have various highly visible rendering issues: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:
See best practices for examples.
flutter/188329, flutter/188911, flutter/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.