Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@bdero
Copy link
Member

@bdero bdero commented Jul 22, 2023

Resolves flutter/flutter#128624.

It turns out that SkFont::getBounds() snaps results to integers on Android, but not iOS. By scaling the font up and scaling the resulting per-glyph bounds back down, we can ensure that the results are always precise enough.

I also found errors with our usage of the computed bounds, but those were comparatively minor fixes.

Before:

before.mp4

After:

after.mp4

Minimal repro app:

import 'dart:ui' as ui;

import 'package:flutter/material.dart';

void main() {
  runApp(const MyApp());
}

class Painter extends CustomPainter {
  Painter(this.scale, this.textSize);

  double scale = 1;
  double textSize = 1;

  @override
  void paint(Canvas canvas, Size size) {
    debugPrint("Scale: $scale  FontSize: $textSize");

    ui.ParagraphBuilder b = ui.ParagraphBuilder(
        ui.ParagraphStyle(fontSize: textSize, fontStyle: FontStyle.normal))
      ..pushStyle(ui.TextStyle(color: Colors.black))
      ..addText("i");
    ui.Paragraph p = b.build();
    p.layout(ui.ParagraphConstraints(width: size.width));

    canvas.scale(scale);
    canvas.drawParagraph(p, const Offset(0, 0));
  }

  @override
  bool shouldRepaint(covariant CustomPainter oldDelegate) {
    return true;
  }
}

class MyApp extends StatefulWidget {
  const MyApp({super.key});

  @override
  State<MyApp> createState() => _MyAppState();
}

class _MyAppState extends State<MyApp> {
  double scale = 1;
  double textSize = 10;

  @override
  Widget build(BuildContext context) => MaterialApp(
        debugShowCheckedModeBanner: false,
        home: Scaffold(
          body: Flex(
            direction: Axis.vertical,
            children: [
              const SizedBox.square(dimension: 50),
              Slider(
                  min: 0,
                  max: 10,
                  value: scale,
                  onChanged: (double value) {
                    setState(() {
                      scale = value;
                    });
                  }),
              Slider(
                  min: 0,
                  max: 50,
                  value: textSize,
                  onChanged: (double value) {
                    setState(() {
                      textSize = value;
                    });
                  }),
              Expanded(child: CustomPaint(painter: Painter(scale, textSize))),
            ],
          ),
        ),
      );
}

@bdero bdero self-assigned this Jul 22, 2023
@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

SkScalar x,
SkScalar y) {
const auto text_frame = TextFrameFromTextBlob(blob);
const auto text_frame = TextFrameFromTextBlob(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this will work against @dnfield s work to defer computing the text scale. Seems like this is strictly an improvement for now, but we should probably also defer the initial scale value?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, good catch. Using the scale here could indeed cause problems. Text frames drawn through DrawPicture might not end up having precise enough glyph bounds depending on how much the elements of the picture get scaled up.

We can get away with just using a decently large number here instead of the scale, so I went ahead and removed the usage and checked to make sure it's still working with unreasonably huge glyphs on Android.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After doing this, there are no longer any overlapping characters in the MaybeHasOverlapping test. The accompanying comment was:

Characters probably have overlap due to low fidelity text metrics, but this could be fixed.

So if there happens to be an accompanying bug I guess we can close that too. :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh interesting. So we don't actually end up rasterizing at a higher scale, we're essentially just tricking Skia into giving higher fidelity metrics.

I don't think I filed a bug for that particular overlap issue, I would update the expectation and delete the TODO.

@bdero bdero force-pushed the bdero/fix-glyph-bounds-android branch from aee636b to 33586eb Compare July 22, 2023 19:34
@bdero bdero force-pushed the bdero/fix-glyph-bounds-android branch from 33586eb to 7750000 Compare July 22, 2023 20:01
@flutter-dashboard
Copy link

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.

Changes reported for pull request #43919 at sha 7750000

Copy link
Contributor

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@bdero
Copy link
Member Author

bdero commented Jul 22, 2023

There are no golden changes listed. ¯_(ツ)_/¯ Merging this.

@bdero bdero merged commit 4ad970f into flutter:main Jul 22, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

No open projects
Archived in project

Development

Successfully merging this pull request may close these issues.

[Impeller] Vulkan: Top line of text is cropped sometimes in wonderous app

3 participants