flutter / flutter

Flutter makes it easy and fast to build beautiful apps for mobile and beyond
https://flutter.dev
BSD 3-Clause "New" or "Revised" License
166.53k stars 27.59k forks source link

Hairline stroke painting can be antialiased across fractional pixel boundaries #59798

Open tvolkert opened 4 years ago

tvolkert commented 4 years ago

Steps to reproduce

Run the following app:

import 'package:flutter/material.dart';

void main() {
  runApp(HairlineNotSoMuch());
}

class HairlineNotSoMuch extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
    return Padding(
      padding: EdgeInsets.all(50),
      child: Column(
        textDirection: TextDirection.ltr,
        crossAxisAlignment: CrossAxisAlignment.start,
        children: [
          CustomPaint(
            size: Size(99, 99),
            painter: BoxPainter(),
          ),
        ],
      ),
    );
  }
}

class BoxPainter extends CustomPainter {
  @override
  void paint(Canvas canvas, Size size) {
    final Paint paint = Paint()
      ..style = PaintingStyle.stroke
      ..strokeWidth = 0
      ..color = Colors.white;
    final Rect rect = Rect.fromCenter(
      center: Offset(size.width / 2, size.height / 2),
      width: size.width,
      height: size.height,
    );
    canvas.drawRect(rect, paint);
  }

  @override
  bool shouldRepaint(CustomPainter oldDelegate) => true;
}

Expected behavior

The documentation on Paint.strokeWidth says that a zero width "corresponds to a hairline width", and a further discussion of hairline widths in BorderSide.width says "Setting width to 0.0 will result in a hairline border. This means that the border will have the width of one physical pixel."

Given that (and no discussion of antialiasing), you expect such the code above to produce a 1-physical-pixel border.

Actual behavior

The above code yields the following result (on my mac-os embedding; saw similar results on iOS and Android emulator):

Screen Shot 2020-06-18 at 1 47 24 PM

This shows a 2-physical-pixel border.

Note: setting isAntiAlias = false on the paint causes it to render to one physical pixel. If the behavior outlined so far is indeed working as intended, then we should fix BorderSide.toPaint() to turn off anti-aliasing for the hairline border case. The current code is:

https://github.com/flutter/flutter/blob/6db22118ad7192d2719816688b65cef325db528b/packages/flutter/lib/src/painting/borders.dart#L179-L182

tvolkert commented 4 years ago

Discussed a little bit with @Hixie, and he points out that turning off antialiasing won't work because the line might be rotated (which would look crummy if not antialiased). Likewise, rounded corners would look bad.

I wonder if we could create a single-child widget that would lay its child out on whole-pixel boundaries. This would let the developer consciously choose when to pixel-snap.

tvolkert commented 4 years ago

Looks like there's precedent in other frameworks for that kind of thing. e.g. https://wpf.2000things.com/tag/pixel-snapping/

goderbauer commented 4 years ago

Pixel-snapping is an interesting idea.

For this bug, we need to at least update the documentation you linked to to include an explanation about how AA may effect the hairline (since it may in fact not just be one pixel wide).

goderbauer commented 4 years ago

I wonder if we could create a single-child widget that would lay its child out on whole-pixel boundaries. This would let the developer consciously choose when to pixel-snap.

This could maybe be an Align like widget that is physical-pixel aware and aligns the child to whole-pixel boundaries.

Hixie commented 4 years ago

Such a widget would have to use a Layer, probably, and basically would be like a Transform that nudged things left or right to round onto the nearest pixel boundary. Probably would have to specify which axis to do it on. Not sure what we'd do in cases where the widget is rotated or in a perspective transform or whatnot. Assert? Do nothing?

tvolkert commented 4 years ago

Here's an example of the manifestation fo this in a real app:

Designer specification

Screen Shot 2020-06-20 at 1 14 27 PM

Implemented design

Screen Shot 2020-06-20 at 1 14 39 PM

You also get irregular behavior on different border sides (these screen captures scaled 300%):

Designer specification

Screen Shot 2020-06-20 at 1 17 06 PM

Implemented design

Screen Shot 2020-06-20 at 1 17 15 PM

Without the widget we're talking about, there's no way to fix these issues. What's worse: (a) they're very common in desktop environments because of window resizing, and (b) as the user resizes the window, the borders will jump between 1-pixel and 2-pixel-AA, which draws the user's eye to the issue.

tvolkert commented 4 years ago

@Hixie could the widget not use localToGlobal combined with devicePixelRatio to know where to place its child - without using layers? It'd be O(depth) for layout, which would be slow, but we could document it as such, much like we do with IntrinsicColumnWidth?

Hixie commented 4 years ago

without layers you can't know where you are in the coordinate space (especially when moving)

tvolkert commented 4 years ago

as the user resizes the window, the borders will jump between 1-pixel and 2-pixel-AA, which draws the user's eye to the issue.

Here's a demonstration of what that looks like:

ezgif-6-f6dabc5dc8a9

tvolkert commented 4 years ago

Actually, thinking about this more, isn't this be a bug in the dart:ui layer? The reason for the AA is that the thickness of the stroke causes it to span real-pixel boundaries... but with a thickness of zero, that should never happen - that's the whole point of hairline thickness.

@reed-at-google

Hixie commented 4 years ago

if you draw a hairline on a pixel boundary, which side would you want it to paint on? either side would be wrong.

tvolkert commented 4 years ago

I was picturing each pixel as covering a range of physical coordinate values. e.g. pixel 0 covers 0<=P<1, pixel 1 covers 1<=P<2, etc. So if you painted a hairline at coordinate x=1.0 (on the border of the first and second x-pixels), it would paint on the second pixel.

(whereas a stroke thickness of 1 would paint half in the first pixel and half in the second)

tvolkert commented 4 years ago

A weird manifestation of this is that you can have a width-1 border starting at a whole-pixel boundary (in the following example, assume a device-pixel-ratio of 1), and it paints in 1 pixel:

class BorderApp extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
    return ColoredBox(
      color: Color(0xffffffff),
      child: Padding(
        padding: EdgeInsets.all(10),
        child: DecoratedBox(
          decoration: BoxDecoration(
            border: Border.all(color: Color(0xff000000), width: 1),
          ),
        ),
      ),
    );
  }
}

Screen Shot 2020-06-22 at 8 06 07 AM

But if you then lower the width of the border to zero, you get the following AA'd border:

Screen Shot 2020-06-22 at 8 07 26 AM

This intuitively seems weird for the developer... "I started at the same x,y coordinates, said to use a smaller value for width... and the border looks wider." The reason for this is that for the width-1 border, we (correctly) offset its starting x,y coordinates by half the stroke width:

https://github.com/flutter/flutter/blob/599566177736e6b30af922a8f06e8a260acafc36/packages/flutter/lib/src/painting/box_border.dart#L240

Whereas for the 0-width border, we (again, correctly) don't. But the end result is super counter-intuitive for the app developer.

This also would mean that if we created this layout widget we're talking about, we'd need to provide to parameter to say "layout at the physical half-pixel" (or some such facility), otherwise developers would run into the behavior above.

In any world, this should all be documented thoroughly 🙂

Hixie commented 4 years ago

I was picturing each pixel as covering a range of physical coordinate values. e.g. pixel 0 covers 0<=P<1, pixel 1 covers 1<=P<2, etc. So if you painted a hairline at coordinate x=1.0 (on the border of the first and second x-pixels), it would paint on the second pixel.

The problem with any snapping like this is that it lines up poorly.

Consider a hairline from 0.9,0 to 0.9,10, that then hits a hairline that starts at 0.9,10 and goes to 1.9,20. If you snap vertical lines, then it won't line up with the slightly angled line any more. If you snap the coordinates of any line, then it won't line up with paths. If you snap any coordinate of a path, then your paths will look super-ugly when you're animating them.

tvolkert commented 4 years ago

Yep, I was coming up with other ways in which it didn't work last night after I wrote that. I think the only real way forward is to provide the layout widget we're talking about.

Beyond that, the only other slightly workable solution (but not really workable) is to have the app designer create their app in such a way that they avoid partial physical pixels where it matters... but that's a huge burden for the app developer when posed with different devices with different pixel densities, and where it only works if they control the widgets from the root pixel on down.

flar commented 3 years ago

With regard to aligning widgets, aligning each widget independently has less fidelity than if the layout algorithms were pixel aware. Consider a Row or Column with 6 child widgets and spaced evenly, it has to distribute the remaining space into 7 spaces between the widgets which is generally going to lead to every child being on an X/7th sub-pixel boundary. If you wrapped each child with a single child alignment wrapper then half of them are a tiny bit to the left and half of them are a tiny bit to the right. This may not be very visible on a phone that has a DPR of 3.0, but when we get to desktop and are rendering under Windows on screens with a DPR of 1.5, 2.25, etc. this gets more noticeable.

flar commented 3 years ago

Another area where we see this is with widgets that are filtered with a blur. our default blur tile mode is clamp which means the edge pixels are smeared outwards with respect to the sampling. If a container has a decoration that is opaque to the edges and it happens to align with pixel boundaries then it will have a solid edge to its decoration. We are seeing a number of issues submitted where that doesn't happen and if, say, you Center the widget and it positions you on a 0.5 boundary then your edges are half-transparent and you have a fair bit of background showing through on the edges of your widgets. This is really hard for the developer to solve without the layout algorithms being pixel aware.

See: https://github.com/flutter/flutter/issues/64828 See: https://github.com/flutter/flutter/issues/39623

flar commented 3 years ago

Note that, in the case of the layout causing a halo on the blurred widget background decorations, the effect is not "sub-pixel" in size, the halo is as large as the blur radius and quite noticable regardless of DPR.

(Actually it is blur_radius * DPR in size, so it is large and then it also scales up with the DPR.)

lslv1243 commented 3 years ago

Any updates on creating this align to pixel widget? I believe it could help me fix a problem I am having with half pixel drawing. https://github.com/flutter/flutter/issues/67881

flar commented 3 years ago

Any updates on creating this align to pixel widget? I believe it could help me fix a problem I am having with half pixel drawing.

67881

Better pixel snapping can't really help the lack of clamping in the drawImageRect method. Even if the destination rectangle of the image is aligned on pixel boundaries, the fact that the DPR can stretch its representations mean that we can sample the image at slightly above the center of the indicated edge pixel and that would blend in some of the next pixels color.

tvolkert commented 3 years ago

In the Chicago library, I kinda-sorta solved this by building "roundToNearestPixel" (logical pixels) into the widgets themselves. This parameter passes down to the render layer, where the render objects apply rounding during layout. The assumption is that integer logical pixels in a desktop widget set will translate to integer physical pixels.

This largely worked out in the Chicago library, but it still might be handy to have a pixel snapping widget in the framework.

flar commented 3 years ago

Rounding to nearest logical pixel works OK on Mac and many phones, but Windows PCs have very common scale factors of 125%, 225%, 150%, etc. Apple only really produces machines and monitors that revolve around integer multiples, but on the open market random monitors tend not to have even multiples of the standard legacy pixel resolutions (~72-90 DPI).

Also, round to nearest logical pixels prevents you from laying things out on the odd pixels on a Mac.

flutter-triage-bot[bot] commented 11 months ago

This issue is marked P1 but has had no recent status updates.

The P1 label indicates high-priority issues that are at the top of the work list. This is the highest priority level a bug can have if it isn't affecting a top-tier customer or breaking the build. Bugs marked P1 are generally actively being worked on unless the assignee is dealing with a P0 bug (or another P1 bug). Issues at this level should be resolved in a matter of months and should have monthly updates on GitHub.

Please consider where this bug really falls in our current priorities, and label it or assign it accordingly. This allows people to have a clearer picture of what work is actually planned. Thanks!