[Impeller] Fixes text scaling issues again, this time with perspective #43662
Conversation
This reverts commit 255ef6c.
|
(Added one more commit cause I realized I forgot the branch where we can't render to the onscreen pass) |
|
Something isn't adding up here:
|
I'm not sure if we're rendering wrong - the list wheel viewport stuff (cupertino date picker) looks ok to me. But when you translate something with perspective you may be affecting the scale. We're definitely broken today but just getting lucky because the scales are close enough where it's not making a huge difference - but we're drawing text into the glyph atlas in the perspective case at one scale and then rendering it at another. |
| } | ||
|
|
||
| auto type = frame_.GetAtlasType(); | ||
| auto scale = entity.DeriveTextScale(); |
There was a problem hiding this comment.
Rather than using the render-time entity to derive the correct scale of the text, why not record the derived scale (or even the full matrix) to TextContents when it's being built by Aiks? We know the CTM in that moment. Then, we just use that value for both the rendering lookup + PopulateGlyphAtlas without having to worry about fuzzy lookup problems.
There was a problem hiding this comment.
TextContents could technically get shared between entities given it's a shared_ptr, but in practice we don't share contents between different rendering operations today.
And if we did, we'd likely only do this for color sources, not special rendering operations that cross color source concerns with geometry concerns like with text.
|
Closing this in favor of #43695 |
…when a save layer is involved (#43695) Alternative to #43662 Records the basis vector of the current CTM into text contents and reuses that rather than trying to get it again at render time. That method breaks if perspective is involved in the CTM and a subpass gets translated, which can modify the scale (and rotation) of the matrix. We're definitely not doing things quite right with perspective here, but the real fix to that is to either record the fully transformed glyph into the atlas or to use SDF/path based rendering. Fixes flutter/flutter#130476 I still have some concerns about how `EntityPass::Render` is a mix of mutations and constness but we can try to address that independently. I expect this to re-improve the regression noted in flutter/flutter#130594
…when a save layer is involved (flutter#43695) Alternative to flutter#43662 Records the basis vector of the current CTM into text contents and reuses that rather than trying to get it again at render time. That method breaks if perspective is involved in the CTM and a subpass gets translated, which can modify the scale (and rotation) of the matrix. We're definitely not doing things quite right with perspective here, but the real fix to that is to either record the fully transformed glyph into the atlas or to use SDF/path based rendering. Fixes flutter/flutter#130476 I still have some concerns about how `EntityPass::Render` is a mix of mutations and constness but we can try to address that independently. I expect this to re-improve the regression noted in flutter/flutter#130594
Difference from last time is in f19025a.
Fixes flutter/flutter#130476
The failure case is when we have a subpass that has a perspective matrix. Translating a perspective matrix may result in a scale change, since you're potentially translating it across the Z plane to account for perspective. This was missed before because I looked at the code and thought "ah it's just a translate so it's ok."
That commit adds a test that previously would fail.
The meat of this change is to wait to populte the glyph atlas until after we have constructed the entity list that we will actually render. To make that work, I'm capturing the entities, the pass context, and the render pass for that entity, iterating that list once to populate the glyph atlas (since these are the entities with the correct matrix), and then a second time to call render (and potentially end the inline pass context).