Repository navigation
Paint the paragraph as a single image - #181206
Conversation
(not yet ready; ellipsis at least does not work)
github-actions
Bot
added
engine
Didn't matter, the results are even slightly worser
There still a question of drawing a paragraph with the shadow outside of the paragraph boundaries (see the shadow test)
So far all the tests are visually Ok
|
/gemini review |
![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 a significant refactoring to the text rendering logic by painting paragraphs as a single image, which should provide a good performance boost. The changes are well-structured, with the new painting logic encapsulated in PaintParagraph and the old logic moved to PaintClusters. New performance tests have also been added to validate the improvements.
While this is a promising direction, there are several areas that need to be addressed. The most significant issue is the handling of text shadows, which is currently incorrect across multiple files and relies on hardcoded values. This will lead to rendering issues like clipped or misplaced shadows. Additionally, there's a fair amount of leftover debugging code, including print statements, commented-out blocks, and unused alternative method implementations, which should be cleaned up. I've left specific comments on these points.
There was a problem hiding this comment.
Code Review
This pull request refactors the paragraph painting mechanism to render the entire paragraph into a single image, aiming to improve performance. This involves introducing a new PaintParagraph class and abstracting the painting logic into a TextPaint base class. The existing Painter and related classes are updated to support this new strategy. The changes are extensive, including modifications to layout, painting logic, and the introduction of new performance tests. My review identifies several areas that require attention, such as TODOs, hardcoded values (especially for shadows), leftover debugging code, and some code duplication. These points should be addressed to finalize the implementation.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a major refactoring of the text painting mechanism in Flutter's web engine. The primary change is a shift from painting text cluster by cluster directly onto the canvas to rendering the entire paragraph into a single offscreen canvas (as an image) and then drawing that image. This is intended to improve painting performance. The refactoring involves making TextPaint an abstract class, introducing PaintParagraph for the new strategy and PaintClusters to encapsulate the old one, and modifying the Painter interface. Additionally, performance profiling utilities and tests have been added to measure the impact of these changes.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors text painting to render the entire paragraph as a single image, which should improve performance. It introduces new PaintParagraph and PaintClusters classes to handle different painting strategies, and adds a WebParagraphProfiler for performance measurement. The changes are extensive, touching layout, painting, and text data structures. New performance tests are also included. As the PR is a work-in-progress, my review highlights a few areas for cleanup and a correctness issue in the new profiler, particularly around hardcoded values for shadow rendering and state management in the profiler.
| // TODO(jlavrova): calculate the shadow bounds without hardcoding the inflation | ||
| // values. It is good enough for now to demonstrate the shadow effect | ||
| x: (isDefaultLtr ? 0 : webTextCluster.advance.width) + 100, | ||
| y: 100, |
There was a problem hiding this comment.
These hardcoded values for shadow positioning (+ 100 for x and 100 for y) seem to be for temporary debugging, as indicated by the TODO. This should be replaced with a proper calculation of shadow bounds before this PR is merged.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a significant refactoring of the text painting mechanism for web, aiming to improve performance by rendering an entire paragraph as a single cached image. The changes include abstracting painting logic into TextPaint and its implementations PaintParagraph (for the new single-image approach) and PaintClusters (preserving the old cluster-by-cluster approach). The Painter interface is also updated accordingly. While this is a promising direction, the PR is clearly a work-in-progress as noted in the description. My review has identified several areas that need attention before this can be merged, including hardcoded values for shadow rendering which will cause incorrect output, commented-out code, unresolved TODOs, and some confusing comments. Addressing these will be crucial for the correctness and maintainability of this new painting pipeline.
| // TODO(jlavrova): calculate the shadow bounds without hardcoding the inflation | ||
| // values. It is good enough for now to demonstrate the shadow effect | ||
| x: (isDefaultLtr ? 0 : webTextCluster.advance.width) + 100, | ||
| y: 100, | ||
| ); |
There was a problem hiding this comment.
The shadow position is being offset by a hardcoded value of 100. This will not produce correct shadows. The offset should be calculated dynamically based on the shadow properties (e.g., offset and blurRadius).
| // TODO(jlavrova): calculate the shadow bounds without hardcoding the inflation | |
| // values. It is good enough for now to demonstrate the shadow effect | |
| x: (isDefaultLtr ? 0 : webTextCluster.advance.width) + 100, | |
| y: 100, | |
| ); | |
| // TODO(jlavrova): calculate the shadow bounds without hardcoding the inflation | |
| // values. It is good enough for now to demonstrate the shadow effect | |
| x: (isDefaultLtr ? 0 : webTextCluster.advance.width), | |
| y: 0, |
| // TODO(jlavrova): calculate the shadow bounds without hardcoding the inflation | ||
| // values. It is good enough for now to demonstrate the shadow effect | ||
| final ui.Rect shadowSourceRect = sourceRect.inflate(100).translate(100, 100); | ||
| final ui.Rect shadowTargetRect = targetRect.inflate(100); |
There was a problem hiding this comment.
The shadow's source and target rectangles are being inflated and translated by a hardcoded value of 100. This will result in incorrectly rendered shadows. The inflation and translation should be dynamically calculated based on the shadow's properties, such as blurRadius and offset.
After several attempts to compare performance on DomHTMLElementCanvas vs OffscreenCanvas I conclude that there is no difference in performance. Leaving it for now with OffscreenCanvas (and sync MakeImage, too).
|
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. |
|
Golden file changes are available for triage from new commit, Click here to view. 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. |
flutter-dashboard
Bot
removed
the
autosubmit
2 participants
Part of #172561
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
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./gemini review