Add `RenderObject.computeTransformToRenderObject` #130192

LongCatIsLooong · 2023-07-08T01:32:06Z

Basically a getTransformTo that doesn't require ancestor to be an ancestor of this. This is prompted by #129969: whether there's an OverlayPortal between a descendant widget and an ancestor widget is sometimes an implementation detail, so the descendant widget or the ancestor widget should not make the assumption that their render objects are also a descendant/ancestor of each other.

@chunhtai I'd like to use the new method to implement Selectable.getTransformTo next. When the new method returns null then Selectable.getTransformTo should return a zero matrix?

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is test-exempt.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

goderbauer · 2023-07-10T17:14:43Z

packages/flutter/lib/src/rendering/object.dart

+    return transform;
+  }
+
+  /// Returns the paint transform that allows this `RenderObject` to perform


Suggested change

/// Returns the paint transform that allows this `RenderObject` to perform

/// Returns the paint transform that allows this [RenderObject] to perform

(and elsewhere where the name of a link-able class is used)

goderbauer · 2023-07-10T17:16:10Z

packages/flutter/lib/src/rendering/object.dart

+  ///  * [getTransformTo] which does the same, but only searches up the tree.
+  ///  * [paintsChild] which this method uses to determine whether
+  ///    a `RenderObject` is in an offscreen subtree.
+  Matrix4? computeTransformToRenderObject(RenderObject toRenderObject) {


For my own understanding: Why does this need new API instead of just making getTransformTo behave this way?

I was a bit concerned that someone would actually depend on the current behavior of getTransformTo but since it just asserts I guess that's unlikely. I'll change the behavior of getTransformTo instead.

goderbauer · 2023-07-10T17:17:50Z

packages/flutter/lib/src/rendering/object.dart

+    return null;
  }

+  static bool _computePaintTranfrom(RenderObject fromRenderObject, Matrix4 fromTransform, RenderObject toRenderObject, Matrix4 toTransform) {


nit: maybe document what the boolean return value means. It's not clear from the method name.

Actually, documenting some of the parameters would also be helpful, especially the differences between the two transforms that this produces.

For my own understanding (and probably good to document): fromTransform is the transformation from the fromRenderObject to a potential common ancestor and toTransform is the transformation from that common ancestor to the toRenderObject - then if we multiply these two we end up with the transformation to allow the fromRenderObject to paint in the toRenderObject?

goderbauer · 2023-07-10T17:27:15Z

packages/flutter/lib/src/rendering/object.dart

+    if (identical(fromRenderObject, toRenderObject)) {
+      return true;
+    }
+    final bool fromLowerThanTo = fromRenderObject.depth >= toRenderObject.depth;


uber-nit: variable name says lowerThan - logic allows for equal. Is the equal important here?

goderbauer · 2023-07-10T17:33:48Z

packages/flutter/lib/src/rendering/object.dart

+  /// See also:
+  ///
+  ///  * [getTransformTo] which does the same, but only searches up the tree.
+  ///  * [paintsChild] which this method uses to determine whether


Does getTransformTo consider this or is it another difference?

goderbauer · 2023-07-10T17:47:23Z

packages/flutter/lib/src/rendering/object.dart

+    return null;
  }

+  static bool _computePaintTranfrom(RenderObject fromRenderObject, Matrix4 fromTransform, RenderObject toRenderObject, Matrix4 toTransform) {


typo - I think?

Suggested change

static bool _computePaintTranfrom(RenderObject fromRenderObject, Matrix4 fromTransform, RenderObject toRenderObject, Matrix4 toTransform) {

static bool _computePaintTransform(RenderObject fromRenderObject, Matrix4 fromTransform, RenderObject toRenderObject, Matrix4 toTransform) {

goderbauer · 2023-07-10T17:49:05Z

packages/flutter/lib/src/rendering/object.dart

+
+  // Computes the paint transform from this to toRenderObject and applies it on
+  // `transform`.
+  Matrix4 _computePaintTransform(RenderObject toRenderObject, Matrix4 transform) {


The fact that there is a static function basically with the same name is very confusing since they do different things. Should this maybe be _computePaintTransformToAncestor or something like that? and toRenderObject could be renamed ancestor.

goderbauer · 2023-07-10T17:56:13Z

packages/flutter/lib/src/rendering/object.dart

+      return _computePaintTransform(ancestor, identity);
+    } else  {
+      final RenderObject? parent = this.parent;
+      return parent != null ? _computePaintTransformToGlobalCoordinates(parent, identity) : identity;


Logically it seems odd that calculating the path all the way to the root (ancestor == null) requires a completely different code path than calculating it to a given ancestor. If anything, I would have at least expected that the shape of these methods is the same, just with a different stop condition...

I think when ancestor is null the expected behavior of the method is to find the closest RenderView (since RenderView.applyPaintTransform applies the device's pixel ratio)?

I would have expected that it just walks all the way to root node of the pipeline owner without making any assumptions what that root node is. As documented, if people want to convert to physical pixels, they'd have to do that manually by applying the transform of the RenderView.

goderbauer · 2023-07-10T18:04:47Z

packages/flutter/lib/src/rendering/object.dart

+  ///
+  ///  * [computeTransformToRenderObject] which does the same, but the target
+  ///    `RenderObject` does not have to be an ancestor.
  Matrix4 getTransformTo(RenderObject? ancestor) {


For my own understanding: Why refactor this method as well? What is the benefit? Is it faster? (At first I thought it was to make code sharable between the two methods, but that doesn't seem to be the case?)

FWIW, for my personal taste, the old implementation was more self-documenting and easier to follow along what it does. It was also only half as long. The new implementation is less straight-forward and introduces separate code paths depending on the argument...

goderbauer · 2023-07-10T18:08:15Z

packages/flutter/test/rendering/object_test.dart

+    expect(renderObject1.computeTransformToRenderObject(renderObject1), equals(Matrix4.identity()));
+    expect(renderObject11.computeTransformToRenderObject(renderObject11), equals(Matrix4.identity()));
+    expect(renderObject12.computeTransformToRenderObject(renderObject12), equals(Matrix4.identity()));
+  });


Can we add a test that actually compute something meaningful to make sure the implementation actually does what it promises? I could satisfy all these tests by "accidentally" replacing the implementation of computeTransformToRenderObject with:

Matrix4 computeTransformToRenderObject(...) => Matrix4.identity();

goderbauer · 2023-07-10T18:13:02Z

packages/flutter/lib/src/rendering/object.dart

-      renderers[index].applyPaintTransform(renderers[index - 1], transform);
+    final Matrix4 inverseTransform = Matrix4.identity();
+    if (_computePaintTranfrom(this, transform, toRenderObject, inverseTransform) && inverseTransform.invert() != 0) {
+      return inverseTransform..multiply(transform);


Mathematically, this reads confusing. Doesn't multiplying the inverse matrix with the original matrix always give you the identity matrix by definition? So why not shortcut all this and just return Matrix4.identity()? 😄

…ransformTo

LongCatIsLooong · 2023-07-13T00:13:09Z

Updated the code and stuck to the getTransformTo name. I had to change the Selectable interface such that its getTransformTo also takes a named parameter.

goderbauer · 2023-07-13T17:21:35Z

dev/benchmarks/microbenchmarks/lib/ui/get_transform_bench.dart

@@ -0,0 +1,162 @@
+// Copyright 2014 The Flutter Authors. All rights reserved.


I forget how this works: Does this benchmark need to be registered anywhere so it runs regularly and uploads data to skia perf? Or do we automatically run all benchmarks in dev/benchmarks/microbenchmarks/lib/ui?

Added to microbenchmarks.dart

goderbauer · 2023-07-13T17:24:59Z

packages/flutter/lib/src/rendering/object.dart

 import 'debug.dart';
 import 'layer.dart';
 import 'proxy_box.dart';
+import 'view.dart' show RenderView;


This is still a layer violation. RenderView sits on top of object.dart.

(not sure why we have the proxy_box.dart import here, it should be removed as well)

goderbauer · 2023-07-13T17:26:15Z

packages/flutter/lib/src/rendering/object.dart


  /// {@template flutter.rendering.RenderObject.getTransformTo}
-  /// Applies the paint transform up the tree to `ancestor`.
+  /// Applies the paint transform from thie [RenderObject] to the `target`


goderbauer · 2023-07-13T17:27:28Z

packages/flutter/lib/src/rendering/object.dart

-  /// If `ancestor` is null, this method returns a matrix that maps from the
+  /// The `skipIfInUnpaintedSubtree` argument determines whether the method
+  /// should return a zero matrix when either this [RenderObject] or `target` is
+  /// in a subtree where the topmost [RenderObject]'s [RenderObject.paintsChild]


What is the topmost RenderObject?

The code seems to check paintsChild for every RO along the path, so I am unsure what topmost means.

Rephrased. Also changed the implementation

goderbauer · 2023-07-13T17:28:28Z

packages/flutter/lib/src/rendering/object.dart

+  /// in a subtree where the topmost [RenderObject]'s [RenderObject.paintsChild]
+  /// method returns false. In addition to that, this method may also return a
+  /// zero matrix when the paint transform from the common ancestor [RenderObject]
+  /// (of [this] and `target`) to `target` is a singular matrix.


What does it mean in practical terms if it is a singular matrix? What structure would your render tree have if that happens?

Rephrased. If any RenderObject in the common ancestor -> target RenderObject path uses a coordinate system orthogonal to that of its parent then we can't compute the paint transform using applyPaintTransform. I think it's a limitation of the applyPaintTransform API

goderbauer · 2023-07-13T17:29:07Z

packages/flutter/lib/src/rendering/object.dart

+  /// in a subtree where the topmost [RenderObject]'s [RenderObject.paintsChild]
+  /// method returns false. In addition to that, this method may also return a
+  /// zero matrix when the paint transform from the common ancestor [RenderObject]
+  /// (of [this] and `target`) to `target` is a singular matrix.


What does dartdoc link to if you do [this]? The class doc for RenderObject?

Yeah likely but I didn't mean to make that a link. Changed to this.

goderbauer · 2023-07-13T17:37:28Z

packages/flutter/lib/src/rendering/object.dart

-    for (int index = renderers.length - 1; index > 0; index -= 1) {
-      renderers[index].applyPaintTransform(renderers[index - 1], transform);
+
+    assert(toPath.length > 1);


If target == null couldn't the length of the toPath just be 1? what garantees that the while loop above executes at least twice to guarantee > 1?

each list must have more than one element if not null.

goderbauer · 2023-07-13T17:40:36Z

packages/flutter/lib/src/rendering/object.dart

+      toPath[index].applyPaintTransform(toPath[index - 1], toTransform);
    }
-    return transform;
+    if (toTransform.invert() == 0) {


Maybe I am a bit rusty on my matrix math, but why is it for the operation below important that it is invertible? Maybe leave a comment about that?

The implementation does nothing (https://master-api.flutter.dev/flutter/vector_math_64/Matrix4/copyInverse.html) when the matrix is singular. However I think that's because of the limitation of applyPaintTransform.

goderbauer · 2023-07-13T17:45:53Z

packages/flutter/lib/src/rendering/selection.dart

 mixin Selectable implements SelectionHandler {
  /// {@macro flutter.rendering.RenderObject.getTransformTo}
-  Matrix4 getTransformTo(RenderObject? ancestor);
+  Matrix4 getTransformTo(RenderObject? target, { bool skipIfInUnpaintedSubtree = false });


For my own understanding: Why do we need to modify the signature on Selectable as well? Looks like nothing in this PR is actually calling this with the new parameter?

(also, this seems to be uncovered by tests if we need to keep it)

Selectable.getTransformTo isn't a valid override of RenderObject.getTransformTo if a RenderObject uses that mixin. Alternatively I could change Selectable to an abstract class?

That's kinda unfortunate that Selectable and RenderObject interact that way when used together. It's not immediately clear to me why the Selectable interface (when looked at it stand-alone) would need this parameter. Should we have named this method something else (and make implementors forward it to the RenderObject's getTransformTo, if that's what they want to use)?

Or does the caller of this method on Selectable actually need to care about unpainted subtrees and should use this parameter?

@chunhtai Do you have any thoughts on this?

Alternatively I could change Selectable to an abstract class?

How would you then make a RenderObject subclass use this?

There's no implementation in the Selectable mixin so I can turn that into an abstract class? It could be breaking since people would have to replace with with implements

Yes, but wouldn't class Foo extends RenderObject implements Selectable still have the problem that you cannot satisfy both overrides?

If this is abstract class and this method have a different signature, they would have to manually wire it up when they create a renderobject that implements this abstract class.

what is the use case for skipIfInUnpaintedSubtree: false?

wouldn't class Foo extends RenderObject implements Selectable still have the problem that you cannot satisfy both overrides?

According to dartpad that works. My guess is with Selectable creates a synthetic class that tries to override getTransformTo(RenderObject, { bool ....}) with getTransformTo(RenderObject).

what is the use case for skipIfInUnpaintedSubtree: false

idk, maybe in an animation callback where a RenderOpacity's paintsChild returns false but will return true once the render object updates its layout in that frame?

I would probably remove skipIfInUnpaintedSubtree and don't worry about the signature mismatch. It doesn't seem worth it to modify RenderObject API without a solid use case.

I am ok if we convert to abstract class, this seems like an oversight when i implement this class. but it doesn't seem to help much?

…nsformTo

goderbauer

This basically looks good to me, except for the Selectable bit.

goderbauer · 2023-07-17T17:35:51Z

packages/flutter/lib/src/rendering/selection.dart

 mixin Selectable implements SelectionHandler {
  /// {@macro flutter.rendering.RenderObject.getTransformTo}
-  Matrix4 getTransformTo(RenderObject? ancestor);
+  Matrix4 getTransformTo(RenderObject? target, { bool skipIfInUnpaintedSubtree = false });


That's kinda unfortunate that Selectable and RenderObject interact that way when used together. It's not immediately clear to me why the Selectable interface (when looked at it stand-alone) would need this parameter. Should we have named this method something else (and make implementors forward it to the RenderObject's getTransformTo, if that's what they want to use)?

Or does the caller of this method on Selectable actually need to care about unpainted subtrees and should use this parameter?

@chunhtai Do you have any thoughts on this?

Alternatively I could change Selectable to an abstract class?

How would you then make a RenderObject subclass use this?

chunhtai

In case my latest comment is buried in the review, this is the link to my last comment #130192 (comment)

…ransformTo

flutter-dashboard · 2023-10-02T03:55:39Z

This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold.

For more guidance, visit Writing a golden file test for package:flutter.

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

… in the same tree (#148897) This is #130192 but without the additional parameter. Fixes #146764, #148410

… in the same tree (flutter#148897) This is flutter#130192 but without the additional parameter. Fixes flutter#146764, flutter#148410

LongCatIsLooong added 2 commits July 7, 2023 16:59

Add RenderObject.getTransformTo Benchmark

98a25d1

add RenderObject.computeTransformToRenderObject.

212d51b

github-actions bot added c: contributor-productivity Team-specific productivity, code health, technical debt. framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Jul 8, 2023

LongCatIsLooong requested review from chunhtai and goderbauer July 8, 2023 01:35

comments

74b223f

goderbauer reviewed Jul 10, 2023

View reviewed changes

LongCatIsLooong added 3 commits July 11, 2023 15:01

review

fa2277d

review

cb9cce1

Merge remote-tracking branch 'upstream/master' into RenderObject.getT…

4ae903b

…ransformTo

goderbauer reviewed Jul 13, 2023

View reviewed changes

Merge remote-tracking branch 'upstream/main' into RenderObject.getTra…

a8935e8

…nsformTo

LongCatIsLooong force-pushed the RenderObject.getTransformTo branch from 0edb40a to 18bede0 Compare July 16, 2023 12:19

review

0ea8353

LongCatIsLooong force-pushed the RenderObject.getTransformTo branch from 18bede0 to 0ea8353 Compare July 16, 2023 20:06

goderbauer reviewed Jul 17, 2023

View reviewed changes

chunhtai reviewed Jul 27, 2023

View reviewed changes

Merge remote-tracking branch 'upstream/master' into RenderObject.getT…

ab5e83d

…ransformTo

LongCatIsLooong closed this Dec 13, 2023

This was referenced May 22, 2024

Fix nested SingleChildScrollView ensureVisible throws an error in an OverlayPortal #148865

Closed

Allow RenderObject.getTransformTo to take an arbitrary RenderObject in the same tree #148897

Merged

auto-submit bot pushed a commit that referenced this pull request May 22, 2024

Allow RenderObject.getTransformTo to take an arbitrary RenderObject…

a766945

… in the same tree (#148897) This is #130192 but without the additional parameter. Fixes #146764, #148410

	/// Returns the paint transform that allows this `RenderObject` to perform
	/// Returns the paint transform that allows this [RenderObject] to perform

	static bool _computePaintTranfrom(RenderObject fromRenderObject, Matrix4 fromTransform, RenderObject toRenderObject, Matrix4 toTransform) {
	static bool _computePaintTransform(RenderObject fromRenderObject, Matrix4 fromTransform, RenderObject toRenderObject, Matrix4 toTransform) {

		@@ -0,0 +1,162 @@
		// Copyright 2014 The Flutter Authors. All rights reserved.

Add RenderObject.computeTransformToRenderObject #130192

Add RenderObject.computeTransformToRenderObject #130192

Uh oh!

Conversation

LongCatIsLooong commented Jul 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LongCatIsLooong commented Jul 13, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Add `RenderObject.computeTransformToRenderObject` #130192

Add `RenderObject.computeTransformToRenderObject` #130192

LongCatIsLooong commented Jul 8, 2023 •

edited

Loading