[Impeller] Improve resolution of text scaling. #43533
Conversation
This patch does a few things: - Changes ownership of the LazyGlyphAtlas to ContentContext from Canvas. This means that drawings with multiple canvases (e.g. calls to DrawPicture) share the same lazy atlas. - Moves the scale property from Font::Metrics to FontGlyphPair. Font::Metrics contains properties related to the font, whereas scale has to do with the properties of the drawing at render time and is independent of the font. - Makes the lazy atlas manage FontGlyphPair::Set rather than vectors of TextFrames. - Makes the determination of font scaling at EntityPass::Render rather than Canvas::DrawTextFrame. The scaling may be altered by calls to Canvas::DrawPicture by render time and otherwise get missed. This is done via a new method: Contents::PopulateGlyphAtlas, which is only implemented by TextContents in this patch. This improves over prior attempts: LazyGlyphAtlas ownership is now unambiguous, and glyph scaling happens correctly for all rendered glyphs regardless of the order of canvas operations in Aiks. Fixes flutter/flutter#130142
jonahwilliams
left a comment
There was a problem hiding this comment.
This approach LGTM, assuming there are no serious performance changes on apps with large numbers of entities.
|
auto label is removed for flutter/engine, pr: 43533, due to - The status or check suite Mac mac_host_engine has failed. Please fix the issues identified (or deflake) before re-applying this label. |
| } | ||
|
|
||
| auto type = frame_.GetAtlasType(); | ||
| auto scale = entity.GetTransformation().GetMaxBasisLengthXY(); |
There was a problem hiding this comment.
Nit: Might make sense to have a static Scalar TextContents::DeriveTextScaleFromEntity(const Entity& entity) const or maybe even Scalar Entity::DeriveTextScale() const and use that in both spots. Our way of computing the scale has changed a few times.
|
|
||
| // --------------------------------------------------------------------------- | ||
| // Step 4: Record the positions in the glyph atlas of the newly added | ||
| // Step 3a: Record the positions in the glyph atlas of the newly added |
There was a problem hiding this comment.
Because there are two branches that go down here that both were reusing the same step numbers.
|
auto label is removed for flutter/engine, pr: 43533, due to - The status or check suite Mac mac_host_engine has failed. Please fix the issues identified (or deflake) before re-applying this label.
|
|
auto label is removed for flutter/engine, pr: 43533, due to - The status or check suite Linux linux_arm_host_engine has failed. Please fix the issues identified (or deflake) before re-applying this label. |
|
auto label is removed for flutter/engine, pr: 43533, due to - The status or check suite Mac mac_host_engine has failed. Please fix the issues identified (or deflake) before re-applying this label. |
|
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. |
This patch does a few things:
This improves over prior attempts: LazyGlyphAtlas ownership is now unambiguous, and glyph scaling happens correctly for all rendered glyphs regardless of the order of canvas operations in Aiks.
Fixes flutter/flutter#130142