Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Framework needs to be aware of physical pixels #116278

Closed
knopp opened this issue Nov 30, 2022 · 22 comments
Closed

Framework needs to be aware of physical pixels #116278

knopp opened this issue Nov 30, 2022 · 22 comments

Comments

@knopp
Copy link
Member

knopp commented Nov 30, 2022

I'm filing this as separate from #111302, because I don't believe it is necessarily canvas related. Trying to fix this when painting to canvas is too late.

Framework currently acts as if it is rendering to surface with infinite resolution. However in practice this is not the case and on many (mostly desktop, but also mobile) devices this results in degraded experience. Below are comparison screenshot of simple screen with two rectangles having 1px (logical) border with various DPI scalings.

HTML
<html>
<head></head>
<body style="background-color: black; font-family: sans-serif;">
    <div style="margin-left: 20; margin-top: 20; border: 1px solid yellow; display: inline-flex; width: 64px; height: 44px; box-sizing: border-box; background-color: rgba(255, 166, 0, 0.428); align-items: center; justify-content: center;">
        <div style="display: inline-block; border: 1px solid yellow; padding: 5px; color: white;">
            Hello
        </div>
    </div>
</body>
</html>
Flutter
import 'package:flutter/material.dart';

void main() {
  runApp(
    DefaultTextStyle(
      style: const TextStyle(
        fontSize: 11,
        color: Colors.white,
        decoration: TextDecoration.none,
        fontWeight: FontWeight.normal,
      ),
      child: Align(
        alignment: Alignment.topLeft,
        child: Padding(
          padding: const EdgeInsets.only(left: 20, top: 20),
          child: SizedBox(
            width: 64,
            height: 44,
            child: Container(
              decoration: BoxDecoration(
                  color: const Color.fromRGBO(255, 166, 0, 0.428),
                  border: Border.all(color: Colors.yellow, width: 1)),
              child: Center(
                child: Container(
                  decoration: BoxDecoration(
                      border: Border.all(color: Colors.yellow, width: 1)),
                  padding: const EdgeInsets.all(5),
                  // This is to make sure text has even height and can be properly shown
                  // at least on 1x scaling
                  child: Padding(
                    padding: const EdgeInsets.only(bottom: 1.0),
                    child: const Text(
                      'Hello',
                      textDirection: TextDirection.ltr,
                    ),
                  ),
                ),
              ),
            ),
          ),
        ),
      ),
    ),
  );
}

Comparison of Flutter (left) vs Web Browser (right)

100% scaling

100

Here the borders are identical (though did need to massage the Flutter code with additional padding so that text had even height and could be centered vertically without blur; see the very last screenshot).

125% scaling

125

Web image is crisp (border is still 1 physical pixel), Flutter borders are blurred.

150% scaling

150

Web image still has one physical pixel border, Flutter blurry. With 175% scaling, the border is on web still 1 physical pixel, though in some applications (WinUI) this is where 1 logical pixel turns into two physical pixels.

200% scaling

200

Here Flutter and Web finally look identical.

Bonus image: Flutter at 100%, without extra 1px padding below the text:

100-blurry

Ideally, unless there is extra transform in hierarchy (i.e. scale on rotate), all layers that engine receive should be already properly pixel aligned.

@knopp
Copy link
Member Author

knopp commented Nov 30, 2022

Here is the example from above written in a way where it renders correctly at any scale (just like the web version). It is possible to do, but it shouldn't be this much work. And if I want to use Row or Column with Flexible elements for example, I'd have to write a version of Flex, which is physical pixel aware.

I updated this with version that use extension method for "pixel snapped" values. I don't think this is too horrible :), but it shouldn't be necessary.

import 'package:flutter/material.dart';
import 'package:flutter/rendering.dart';

void main() {
  runApp(
    MaterialApp(
      home: Builder(
        builder: (context) {
          final q = MediaQuery.of(context);
          print('Rebuild at ${q.devicePixelRatio}');
          return DefaultTextStyle(
            style: const TextStyle(
              fontSize: 11,
              color: Colors.white,
              decoration: TextDecoration.none,
              fontWeight: FontWeight.normal,
            ),
            child: Align(
              alignment: Alignment.topLeft.ps,
              child: Container(
                margin: EdgeInsets.only(left: 19.ps, top: 20.ps),
                width: 70.ps,
                height: 44.ps,
                decoration: BoxDecoration(
                    color: const Color.fromRGBO(255, 166, 0, 0.428),
                    border: Border.all(color: Colors.yellow, width: 1.ps)),
                child: Align(
                  alignment: Alignment.center.ps,
                  child: Container(
                    decoration: BoxDecoration(
                        border: Border.all(color: Colors.yellow, width: 1.ps)),
                    padding: EdgeInsets.all(5.ps),
                    child: const PixelSnapSize(
                      child: Text(
                        'Hello',
                        textDirection: TextDirection.ltr,
                      ),
                    ),
                  ),
                ),
              ),
            ),
          );
        },
      ),
    ),
  );
}

double snapToPhysicalPixel(double value) {
  if (!value.isFinite) {
    return value;
  }
  final double ratio =
      WidgetsBinding.instance.renderView.configuration.devicePixelRatio;
  return (value * ratio - 0.1).round() / ratio;
}

double ceilToPhysicalPixel(double value) {
  if (!value.isFinite) {
    return value;
  }
  final double ratio =
      WidgetsBinding.instance.renderView.configuration.devicePixelRatio;
  return (value * ratio).ceil() / ratio;
}

extension PixelSnapNum on num {
  double get ps {
    return snapToPhysicalPixel(toDouble());
  }

  double pixelCeil() {
    return ceilToPhysicalPixel(toDouble());
  }
}

extension OffsetPixelSnap on Offset {
  Offset get ps => Offset(dx.ps, dy.ps);
}

extension PixelSnap on Alignment {
  Alignment get ps => PixelAlignment(x, y);
}

class PixelAlignment extends Alignment {
  const PixelAlignment(super.x, super.y);

  @override
  Offset alongOffset(Offset other) {
    return super.alongOffset(other).ps;
  }

  @override
  Offset alongSize(Size other) {
    return super.alongSize(other).ps;
  }

  @override
  Offset withinRect(Rect rect) {
    return super.withinRect(rect).ps;
  }
}

/// Extends size of underlying widget to align to pixel boundaries
class PixelSnapSize extends SingleChildRenderObjectWidget {
  const PixelSnapSize({
    super.key,
    required super.child,
  });

  @override
  RenderObject createRenderObject(BuildContext context) {
    return _RenderPixelSnapSize(null);
  }
}

class _RenderPixelSnapSize extends RenderShiftedBox {
  _RenderPixelSnapSize(super.child);

  @override
  double computeMinIntrinsicWidth(double height) {
    return super.computeMinIntrinsicWidth(height).pixelCeil();
  }

  @override
  double computeMaxIntrinsicWidth(double height) {
    return super.computeMaxIntrinsicWidth(height).pixelCeil();
  }

  @override
  double computeMinIntrinsicHeight(double width) {
    return super.computeMinIntrinsicHeight(width).pixelCeil();
  }

  @override
  double computeMaxIntrinsicHeight(double width) {
    return super.computeMaxIntrinsicHeight(width).pixelCeil();
  }

  @override
  Size computeDryLayout(BoxConstraints constraints) {
    if (child == null) {
      return constraints.constrain(Size.zero);
    }
    child!.layout(constraints, parentUsesSize: true);
    return constraints.constrain(
        Size(child!.size.width.pixelCeil(), child!.size.height.pixelCeil()));
  }

  @override
  void performLayout() {
    if (child == null) {
      size = constraints.constrain(Size.zero);
      return;
    }
    child!.layout(constraints, parentUsesSize: true);
    size = constraints.constrain(
        Size(child!.size.width.pixelCeil(), child!.size.height.pixelCeil()));
  }
}

@whiskeyPeak
Copy link
Contributor

whiskeyPeak commented Nov 30, 2022

I think I'm be running in a very similar issue as well.

From the picture below, you can see that the right border of the right container is quasi-invisible and that both left and right borders of the left containers are blurry.

Screenshot from 2022-11-30 18-24-28

Here's a picture showing how it ought look:

Screenshot from 2022-11-30 18-25-35

In the app, we're using the the width of the window to determine that width of the right container, whilst still having both containers positioned in the centre of the screen. This causes weird pixel rendering (like above) when resizing the window.

repro code:

import 'package:flutter/material.dart';

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

class MyApp extends StatelessWidget {
  const MyApp({Key? key}) : super(key: key);

  @override
  Widget build(BuildContext context) {
    return const MaterialApp(
      home: HomePage(),
    );
  }
}

class HomePage extends StatefulWidget {
  const HomePage({Key? key}) : super(key: key);

  @override
  State<HomePage> createState() => _HomePageState();
}

class _HomePageState extends State<HomePage> {
  @override
  Widget build(BuildContext context) {
    return Scaffold(
      body: Row(
        mainAxisAlignment: MainAxisAlignment.center,
        children: [
          Padding(
            padding: const EdgeInsets.only(top: 20.0),
            child: Container(
              width: 500,
              height: 200,
              decoration: BoxDecoration(
                border: Border.all(
                  color: Colors.black,
                ),
                color: Colors.grey,
              ),
            ),
          )
        ],
      ),
    );
  }
}

Resizing the width of the window adds to the border's width and makes it blurry. I gave this a try in a plain GTK App with a GTK box and the border there doesn't change when resizing the window.

@jonahwilliams
Copy link
Member

I don't think the framework is the right place to do this. While you can listen to changes to media query, ultimately your parent transforms may change arbitrarily and without rebuilding/repainting children. You'd only get a chance to compute the correct localToGlobal transform once.

I think we'd essentially need to adjust layout at the engine level.

@knopp
Copy link
Member Author

knopp commented Nov 30, 2022

How would you do this on engine level? Imagine that you have 100 pixel window where you need to place three children side by side, all of them perfectly pixel aligned. That would mean 33, 33 and 34 pixels for each child. This has to be done during layout. The constraints that each child gets have to reflect that and one of the children will be 1 pixel larger. There is no way to do something like this in the engine in a way where it could affect widget layout (at least unless there is a major change of how engine interacts with framework, which would be quite tricky).

What do you mean by "your parent transforms may change arbitrarily and without rebuilding/repainting children."? Is there any way this could happen without having arbitrary transform in the hierarchy?

I mean of course arbitrary transform is going to break pixel snapping. But that's kind of to be expected. However vast majority of application that are concerned with this will most likely have only one scale at root level (devicePixelRatio) and everything else would be just translations. As long as those translations are pixel snapped, everything should layout just fine.

@Hixie
Copy link
Contributor

Hixie commented Nov 30, 2022

I would recommend reframing this in terms of a problem rather than a solution. Progress is much easier to make if we first come to an agreement on what the problem we want to fix is.

@jonahwilliams
Copy link
Member

@Hixie agreed. I think the general problem we're looking at here is "Flutter looks meh on low density screens (particularly desktop)". My understanding for how this has been solved in the past is that the frameworks have been aware of an aligned to physical pixels. This is different from how mobile devices with high dpr screens moved to a logical pixel mapping to support multiple screen sizes. I'm not aware of anything besides Chrome that handle this cross platform in a reasonable way.

@knopp Translations and Scale transforms are used arbitrarily throughout the framework. OffsetLayers absorb parent translations and give children new coordinates starting at 0.0. Expanded may scale children, same with various aligment widgets.

@knopp
Copy link
Member Author

knopp commented Nov 30, 2022

I would recommend reframing this in terms of a problem rather than a solution. Progress is much easier to make if we first come to an agreement on what the problem we want to fix is.

That makes sense. There are several screenshot above comparing pixel scaling in Flutter vs Web Browser. Does that help demonstrating the issue?

This is how border of same logical width is painted:

Pixel scaling logical px physical px Flutter physical px Web Browser physical px WinUI
100 % 1 px 1px 1px 1px
125 % 1px 1.25px 1px 1px
150 % 1px 1.5px 1px 1px
175 % 1px 1.75px 1 px 2px
200% 1px 2px 2px 2px

You can see that other than Flutter it is always snapped at physical pixels. It will not get blurry (like Flutter does on screenshot).

@knopp
Copy link
Member Author

knopp commented Nov 30, 2022

Translation transform should not be problem (as long as the translation amount is snapped to physical pixels). If you scale widgets at arbitrary scale then any kind of pixel alignment is probably not a concern anymore (and I doubt it would be pixel snapped in Chrome either).

@jonahwilliams
Copy link
Member

The problem with snapping at layers is that adding/removing/changing compositing leads to jitter

@knopp
Copy link
Member Author

knopp commented Nov 30, 2022

If you snap at layout level, that snapping will not change if the transform above changes. It will be slightly off (not lead to pixel perfect rendering anymore if the transform above changes that), but still consistent so it shouldn't leat to jitter. But maybe you mean something else.

@jonahwilliams
Copy link
Member

I was talking about at the layer level. It may not matter if you snap at the layout level, but I'm unsure. I'll take your word for it

@Hixie
Copy link
Contributor

Hixie commented Nov 30, 2022

I think the general problem we're looking at here is "Flutter looks meh on low density screens (particularly desktop)".

I think if that's the issue then we should start from a very specific minimal test case and test environment and work from there. "Flutter looks meh" is way too vague.

There are several screenshot above comparing pixel scaling in Flutter vs Web Browser. Does that help demonstrating the issue?

Well, if the code is asking for 1.5 device pixel borders, the rendering looks correct to me. That's sort of what I mean by needing to agree on the problem. Is the problem "material buttons look fuzzy", or is the problem "when i ask for a 1.5 device pixel border i get a 1.5 pixel border", or is the problem something else?

@knopp
Copy link
Member Author

knopp commented Nov 30, 2022

Well, if the code is asking for 1.5 device pixel borders, the rendering looks correct to me. That's sort of what I mean by needing to agree on the problem. Is the problem "material buttons look fuzzy", or is the problem "when i ask for a 1.5 device pixel border i get a 1.5 pixel border", or is the problem something else?

Where do you see the code asking for 1.5 device pixel borders? In both cases (HTML and Flutter) the code is asking for 1px (logical) border.

This is screenshot of 1px border @ 1px device pixel ratio:
100-blurry

@knopp
Copy link
Member Author

knopp commented Nov 30, 2022

But I think I'm starting to understand. I see this table

Pixel scaling border logical px physical px Flutter physical px Web Browser physical px WinUI
100 % 1 px 1px 1px 1px
125 % 1px 1.25px 1px 1px
150 % 1px 1.5px 1px 1px
175 % 1px 1.75px 1 px 2px
200% 1px 2px 2px 2px

as a problem with Flutter, you seem to consider it "working as expected".

So the problem is more or less "my 1px border looks really fuzzy, while 1px border in Electron looks perfectly sharp". But I'm guessing the answer for the time being at least is that it is the intended behaviour.

@Hixie
Copy link
Contributor

Hixie commented Nov 30, 2022

Where do you see the code asking for 1.5 device pixel borders?

I don't see any code at all, hence my suggesting that we start from a specific code sample showing the problem. :-)

But yes, I consider the 4th and 5th columns in the table you show above to be a bug, and the 3rd column to be the expected result, in the absence of anything else. That doesn't mean I think Flutter UI should be blurry, though. It means that if a UI widget wants a blurry line it should get one. It means that the difference between rotating 0 radians and rotating an infinitesimally small number of radians should be identical (which I bet isn't the case for web, at least, dunno about WinUI).

It's possible to make sure that a border is sharp at every zoom level in Flutter today, by creating a widget that sizes the border based on the device pixel ratio. (That solution also avoids the weirdness during animations.) If our UIs are blurry, then maybe we should be doing that more too. But if that's the issue, we should start with that and some code samples showing the problem, and work from there.

@knopp
Copy link
Member Author

knopp commented Nov 30, 2022

The code is in original post, just needed to be expanded :)

Screenshot 2022-11-30 at 23 21 56

And indeed, in the post after, I put a code example that scales correctly at every level. Doing just what you suggested. It just seemed that it is maybe something that should work out of the box.

But if you consider the column 4 and 5 a bug and column 3 correct behaviour, then there really isn't much else to discuss. I respectfully disagree, but that's about the extent of it :) This is hardly a hill I'm willing to die on, especially given that there are other ways to achieve this.

@knopp
Copy link
Member Author

knopp commented Nov 30, 2022

I think this issue can be closed. This is the expected behavior.

@knopp knopp closed this as not planned Won't fix, can't repro, duplicate, stale Nov 30, 2022
@Hixie
Copy link
Contributor

Hixie commented Nov 30, 2022

Ah, totally missed the "Flutter" bit there, had no idea what it was hiding. :-)

Yeah, that code pretty explicitly says it wants a border that is the width equal to the number of pixels in the devicePixelRatio. That's what we do.

To be clear, I'm not saying we shouldn't change anything here. If UIs are blurry, we should fix that. I'm just saying we should start from an issue that's specifically about the specific UI that's blurry despite not asking for that. (For example, a UI that just has a button in it without giving any dimensions, shouldn't be blurry.)

@knopp
Copy link
Member Author

knopp commented Dec 7, 2022

Since this is out of scope for Flutter, I built a package that makes it relatively easy to build physical pixel-aligned user interfaces in Flutter. @whiskeyPeak, you can give it a go, I think it would solve your problem.

@whiskeyPeak
Copy link
Contributor

Thanks @knopp, I tried out the package and it does fix the issue

Before:

simplescreenrecorder-2022-12-07_18.44.40.mp4

After:

simplescreenrecorder-2022-12-07_18.46.13.mp4

However, I think that relying on a 3rd-party package to have Flutter correctly render the width of the border is a sub-optimal solution at best. As things are right now, the pixel snapping issue is incredibly noticeable (as you can see from the first video) and applies to any widget basing their position on a given screen ratio. If I tell Flutter to draw a border with a 1px width, it should be able to do so. I checked other GUI frameworks like GTK and didn't run into this issue.

GTK:

simplescreenrecorder-2022-12-07_19.02.36.mp4

@knopp
Copy link
Member Author

knopp commented Dec 7, 2022

I don't think GTK is a great example, since it only supports integral scale factors (and entire layout is calculated with integers)

I agree that this should be handled at framework level (hence the issue). WPF on Windows had pixel snapping 10 years ago. But even with that there are still problematic situations. And there are multiple third party WPF components designed to help with pixel snapping...

But given the disagreement of whether be handled by the framework, a package is a pragmatic solution that solves the problem for many situations. But it also possible that it turns out to be too rigid (i.e. maybe there should be a way to disable pixel snapping for parts of hierarchy during animations). At very least it's much easier to iterate, prototype and test new things in external package than the framework itself.

@github-actions
Copy link

github-actions bot commented Mar 5, 2023

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants