add pixel snapping conditional on presence of raster cache

[jonahwilliams]

The assumption of the raster cache is that we can replace any layer or display list with a rasterized version of itself. At very low DPR, this is only true if that layer or display list is aligned to physical pixels. Even if we ensure that the texture of the layer or display list is rasterized pixel aligned, when we go to the draw that entry we may find the offset is not physical pixel aligned. Thus we can either snap it to a pixel, leaving it in a different location than the un-rasterized version, or we can draw it in the correct location with obvious aliasing issues. Snapping to a different physical pixel may not work if partial repaint is enabled, since the reported damage would be different than the painted area.

Unfortunately, that leaves us in a bit of a bind. It means we can't get rid of pixel snapping as long as we have the raster cache. Instead of adding back the define, use the presence of a raster cache to determine whether to pixel snap. This will still prevent flutter tester regressions caused by pixel snapping changes

flutter/flutter#111145

[jpnurmi]

Thanks for working on this! With f4cd894 and the latest FW main, I didn't notice any jumps while ad-hoc testing a few real apps. 👍

The only remaining issue I noticed, which might be related to #35930 (comment), is that there's still something off with (even unanimated) opacity layers. The jump is still visible in one of the earlier test cases that deliberately used fractional padding and opacity to toggle between cached and non-cached rendering.

main.dart

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

void main() {
  runApp(const MaterialApp(
    // checkerboardRasterCacheImages: true,
    home: MyHomePage(),
  ));
}

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

  @override
  State<MyHomePage> createState() => _MyHomePageState();
}

class _MyHomePageState extends State<MyHomePage> {
  bool _opaque = true;

  @override
  void initState() {
    super.initState();
    Timer.periodic(const Duration(seconds: 1), (timer) {
      setState(() => _opaque = !_opaque);
    });
  }

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      backgroundColor: Colors.red,
      body: Padding(
        padding: const EdgeInsets.all(55.5),
        child: Opacity(
          opacity: _opaque ? 1 : 0.95,
          child: Container(
            color: Colors.white,
            child: const Center(
              child: Text(
                'Flutter',
                style: TextStyle(fontSize: 256, color: Colors.blue),
              ),
            ),
          ),
        ),
      ),
    );
  }
}

[jonahwilliams]

Try with this patch applied to the framework, this ensures that we leave the opacity layers in the tree. Without it, we may snap to a different pixel value which is noticeable on desktop (well low DPR, we could revisit making this conditional on monitor DPR)

diff --git a/packages/flutter/lib/src/rendering/proxy_box.dart b/packages/flutter/lib/src/rendering/proxy_box.dart
index 8f97008d17..14e7c2e6cb 100644
--- a/packages/flutter/lib/src/rendering/proxy_box.dart
+++ b/packages/flutter/lib/src/rendering/proxy_box.dart
@@ -883,7 +883,7 @@ class RenderOpacity extends RenderProxyBox {
        super(child);

   @override
-  bool get alwaysNeedsCompositing => child != null && (_alpha > 0 && _alpha < 255);
+  bool get alwaysNeedsCompositing => child != null && _alpha > 0;

   int _alpha;

@@ -949,11 +949,6 @@ class RenderOpacity extends RenderProxyBox {
       layer = null;
       return;
     }
-    if (_alpha == 255) {
-      // No need to keep the layer. We'll create a new one if necessary.
-      layer = null;
-      return super.paint(context, offset);
-    }

     assert(needsCompositing);
     layer = context.pushOpacity(offset, _alpha, super.paint, oldLayer: layer as OpacityLayer?);
@@ -1060,7 +1055,7 @@ mixin RenderAnimatedOpacityMixin<T extends RenderObject> on RenderObjectWithChil
     _alpha = ui.Color.getAlphaFromOpacity(opacity.value);
     if (oldAlpha != _alpha) {
       final bool? wasRepaintBoundary = _currentlyIsRepaintBoundary;
-      _currentlyIsRepaintBoundary = _alpha! > 0 && _alpha! < 255;
+      _currentlyIsRepaintBoundary = _alpha! > 0;
       if (child != null && wasRepaintBoundary != _currentlyIsRepaintBoundary) {
         markNeedsCompositingBitsUpdate();
       }

[jpnurmi]

Try with this patch applied to the framework, this ensures that we leave the opacity layers in the tree.

Yes, that helps! It's all looking good now 👍

[dnfield]

LGTM

[dnfield]

nit: I think this can just be has_raster_cache() const { return has_raster_cache_; }.

[jonahwilliams]

Done

[dnfield]

Consider documenting why we do this so next time we go to remove it we know what ot test.

[jonahwilliams]

Done

[zanderso]

From PR triage: This is blocked on the tree closure @ flutter/flutter#111193. The clang-tidy failure was a clang-tidy segfault, so I clicked re-run for it.