InteractiveViewer Rotation by justinmc · Pull Request #73358 · flutter/flutter

justinmc · 2021-01-05T21:35:15Z

Description

This PR adds support for rotation to InteractiveViewer:

Screen.Recording.2021-01-05.at.11.29.35.AM.mov

I decided that the most intuitive behavior was to prevent any full side of the viewport from viewing beyond the boundary. This creates an intuitive experience in most cases by allowing the user to see beyond the boundary (the pink in the video above), but not to fully move beyond it.

Related Issues

Closes #57698

Tests

I added the following tests:

Rotating with a two-finger rotation gesture works and the angle follows the pointers.
After rotating, a fling gesture still allows inertial animated movement, and colliding with the boundary allows sliding in one direction.
Many unit tests were added for new geometry-related functions I added.
Some unit tests were removed for functions that I replaced.

Breaking Change

Shouldn't be a breaking change, though rotation will be enabled by default. Existing usages of InteractiveViewer that don't want rotation should explicitly set rotateEnabled to false. If anyone thinks that this will negatively affect many users, I'm open to defaulting rotateEnabled to true, though it seems more logical to me that all of the enabled parameters default to true.

… approach, with axis-aligned boundary, depends on size of viewport, but I didn't realize that before

…dge when rotated, but gets weird numbers in corners and parallel...

…at once)

…, though there is probably a solution that corrects cases with two invalid sides

justinmc · 2021-01-06T00:10:35Z


-  /// Returns the closest point to the given point on the given line segment.
-  @visibleForTesting
-  static Vector3 getNearestPointOnLine(Vector3 point, Vector3 l1, Vector3 l2) {


These methods that I removed were involved in my previous iteration of correcting an invalid matrix, which in this PR I have generalized to work with a rotated matrix as well.

justinmc · 2021-01-10T01:40:04Z


-  return _round(largestExcess);
-}
+const double _kTolerance = 100000000000.0;


There are a bunch of places where I had to use rounding in order to get numbers to match. Do we have a standardized way of handling stuff like this somewhere?

get numbers to match

You mean checking for floating-point equality/comparison? As far as I can tell we just check if 2 floating numbers are within an epsilon range, eg:

flutter/packages/flutter/lib/src/physics/utils.dart

Lines 5 to 21 in 02d441e

/// Whether two doubles are within a given distance of each other.

///

/// The `epsilon` argument must be positive and not null.

/// The `a` and `b` arguments may be null. A null value is only considered

/// near-equal to another null value.

bool nearEqual(double? a, double? b, double epsilon) {

assert(epsilon != null);

assert(epsilon >= 0.0);

if (a == null || b == null)

return a == b;

return (a > (b - epsilon)) && (a < (b + epsilon)) || a == b;

}

/// Whether a double is within a given distance of zero.

///

/// The epsilon argument must be positive.

bool nearZero(double a, double epsilon) => nearEqual(a, 0.0, epsilon);

justinmc · 2021-02-04T19:37:06Z

I think the biggest open questions for this PR are:

How can we better handle the rounding errors that crop up in this PR?
Should rotation be enabled by default?

justinmc · 2021-02-04T19:38:19Z

CC @RomanJos in case you're interested in reviewing this.

RomanJos · 2021-02-06T14:43:09Z

Hi, thanks for calling me I appreciate it.
I first want to say that I tried my best to understand the logic of what this pr do but all the math you implemented are kinda hard to me so I did not understood everything !

My understanding of how this widget handle gestures is as follow :

User press with two fingers
_onScaleStart() set _gestureType to null
User move his finger
_onScaleUpdate() call _getGestureType() to set _gestureType to pan, scale or rotate if it is null. Because _gestureType is global it will lock on his value until _onScaleStart() is called again.
depending on the value of _gestureType, this function pan, scale or rotate the matrix with a focalpoint given by the GestureDetector Widget. In case of a rotation or a pan, _validateMatrix() is called to ensure the corner of the child don't goes inside the viewport.
_onScaleEnd() do the inertia.

I have two problems with this logic :

First, locking _gestureType isn't a good things, sometimes I want to rotate but the widget pan instead and vice versa so I need to restart my move. Or if I change my mind while panning and want to rotate instead I can't.
The focal point isn't move with the fingers for a rotation, this feels weird when I move my finger while gestureType is locked in rotation, I expect the center of the rotation to be where I started my gesture but it follow my fingers.

I believe it would be nicer to transform _gestureType into a list of movements and rotate, pan or scale only if the value is present inside. Similarly to how chrome do for panning, if I start a vertical drage I have X px to drag horizontaly to unlock it and move how I want.

This would solve both problem as the focal point will move with the child.

During testing I also found some issue involving _validateMatrix() :

when I rotate really fast, sometimes the assert assert(_lineContains(pointOnLine)) inside findClosestOffsetOnLineSegmentToOffsetOnLine() fail
I have a child with an aspect ration of let's say 1:4 that is rotated a little bit, if I try to move to left or right and I hit the border, the child pan up or down

doc_2021-02-06_15-41-38.mp4

I hope you understood everything, I borrowed a phone from a friend as I don't have one currently so I couldn't test more lol

LongCatIsLooong · 2021-02-05T23:20:13Z

+  const LineSegment(
+    this.p0,
+    this.p1,
+  );


assert p0 != p1 maybe?

Actually this seems to make LineSegment not be able to be const anymore. Is there a way around that?

LongCatIsLooong · 2021-02-05T23:42:00Z

+  /// A special case of the linesIntersectAt method.
+  static Offset _isVerticalAndInterceptsNonVerticalLineAt(LineSegment vertical, LineSegment nonVertical) {
+    // Assert that vertical is indeed vertical.
+    assert(vertical.p0.dx == vertical.p1.dx);


nit: asserts nonVertical is not vertical too maybe?

LongCatIsLooong · 2021-02-06T04:37:22Z

+    }
+    final double d0 = _distanceBetweenPoints(pointOnLine, p0);
+    final double d1 = _distanceBetweenPoints(pointOnLine, p1);
+    return d0.abs() <= d1.abs() ? p0 : p1;


isn't the distance always non-negative?

LongCatIsLooong · 2021-02-06T05:57:32Z

+      );
+    }
+
+    // Otherwise, the closest points on the line segments are the closest points


Is this true? If the 2 lines are not perpendicular, and the intersection is only on one of the line segments, then the closet point on that line segment is not necessarily the intersection right?
e.g.:

_________ \ \

the closest point on the horizontal line is not the (projected) intersection.

You're right, thanks for catching this! I feel like the behavior near the boundary is a bit smoother after fixing this even.
I had a bunch of tests that were slightly off because of this too.

LongCatIsLooong · 2021-02-06T22:55:29Z

+  // A valid matrix is defined as a matrix where all sides of the viewport
+  // intersect the boundary. This allows the viewport to partially see beyond
+  // the boundary after rotation, but keeps the entire boundary visible.
+  Matrix4 _validateMatrix(Matrix4 matrix, Matrix4 defaultMatrix) {


Would it also achieve the same goal if we compute the bounding box of the viewport and compare it to _boundaryRect to determine the amount of translation we need? That sounds a lot easier than dealing with line segments.

I tried that way first, but I ended up preferring the user experience this way when near the boundary. If I force the axis aligned bounding box of the viewport to fit inside of the boundary, then I won't be able to see the corners of the child. Also, I assume that many uses of InteractiveViewer will have a similarly sized boundary and viewport initially, like a photo viewer, and in that case it's nice to be able to rotate at the default size without being stopped by the boundary.

Maybe this is something that could be configured in the long term since the behavior is different for the two approaches.

Sorry I probably don't fully understand how this works, but from

A valid matrix is defined as a matrix where all sides of the viewport intersect the boundary.

It sounds like a necessary and sufficient condition for _boundaryRect to intersect all 4 edges of nextViewport is:

_boundaryRect.top is in [nextViewport.smallestY, nextViewport.2ndSmallestY]

_boundaryRect.bottm is in [nextViewport.2ndLargestY, nextViewport.largestY]

4, similarly we can get the horizontal constraints for _boundaryRect.left and _boundaryRect.right.

_boundaryRect.bottm - _boundaryRect.top is a constant.

So the problem turns into finding the dx and dy that minimize dx^2 + dy^2 and satisfy all 4 constaints, or one of them in each direction if they can't be satisfied at the same time, i.e.:

_boundaryRect.top + dy in [nextViewport.smallestY, nextViewport.2ndSmallestY] and/or [_boundaryRect.height - nextViewport.largestY, _boundaryRect.height - nextViewport.2ndLargestY]
&&
_boundaryRect.left + dx in [nextViewport.smallestX, nextViewport.2ndSmallestX] and/or [_boundaryRect.width - nextViewport.largestX, _boundaryRect.width - nextViewport.2ndLargestX]

I think you might be on to something, but there is a case where this doesn't work if I understand correctly.

Sorry for the bad drawing but imagine the pen rectangle is the boundary and the pencil rectangle is the viewport:

I would consider this to be invalid because one edge of the viewport does not intersect the boundary. However, _boundaryRect.top is in [nextViewport.smallestY, nextViewport.2ndSmallestY].

There're 4 constraints (2 horizontal and 2 vertical) and to make a viewport valid it has to satisfy all 4. In the drawing the viewport only satisfies 2 (one on each axis): _boundaryRect.top is in [nextViewport.smallestY, nextViewport.2ndSmallestY] but _boundaryRect.bottm is not in [nextViewport.2ndLargestY, nextViewport.largestY] so this is indeed invalid.

ohhh it's neither sufficient or necessary (counter example: https://jamboard.google.com/d/1l4Gg2AAs8d_8fGEAK_diDle-9qSQjYYFPkxpzUXAS84/viewer?f=0). Let me see if it can be improved. I think we can make more use of the fact that the boundary is a Rect to simplify the calculation.

I think the edges of the viewport must fall in the red areas of https://jamboard.google.com/d/1l4Gg2AAs8d_8fGEAK_diDle-9qSQjYYFPkxpzUXAS84/viewer?f=1, but that's not necessary: https://jamboard.google.com/d/1l4Gg2AAs8d_8fGEAK_diDle-9qSQjYYFPkxpzUXAS84/viewer?f=2

justinmc · 2021-02-17T21:52:46Z

@RomanJos Thanks for the thorough review! Your summary of the logic is correct. I've been able to fix some of your points:

_gestureType should be allowed to change. I agree with this. It seems to be how Google Maps works as well. I will try to make this work nicely when I get a chance.
Movable focal point. I was able to fix this pretty easily, thanks for the suggestion! I noticed that Google Maps also allows a moving focal point.
Assertion when rotating fast. I think I have fixed this, could you try again if you get a chance? I think it had to do with InteractiveViewer Rotation #73358 (comment).
Panning up/down when hitting the border. I couldn't reproduce this after making a few code changes, but I'm also not sure it's fixed. Let me know if you reproduce it again if you have time.

I'll make another attempt at fixing the _gestureType problem soon.

RomanJos · 2021-02-22T14:56:05Z

No problem, I tested with the new commits and it looks great! I can't pan up/down by hitting the border anymore, the movable focal point feel a lot more natural. When gestureType will change dynamically it will be even better, sometimes I unconsciously try to zoom when rotating.

The same assert still throw but now on each quarter turns and I think it cause the following behavior, I moved my finger to the top of the screen because it seems to do more crazy things:

(the gif is compressed)

without boundaries margins, my finger doesn't seems to be taken into account only for rotating if it is not on the child area, this is weird since ScaleUpdateDetails.pointerCount is 2 but somehow rotation is 0 inside _getGestureType() :

LongCatIsLooong · 2021-02-25T19:38:18Z

+        final ClosestPoints closestPoints = invalidSide.findClosestPointsRect(_boundaryRect);
+        final Offset difference = closestPoints.a - closestPoints.b;
+        if (magnitudesAligned) {
+          final bool xMisaligned = difference.dx != 0.0 && value.dx != 0.0


nit: equivalent to difference.dx * value.dx < 0 (i.e. having different signs)?

Yes, slick 👍

LongCatIsLooong · 2021-02-25T20:51:30Z

+    );
+
+    // If this matrix is invalid and uncorrectable, then return defaultMatrix.
+    if (!correctedViewportSide01.intersectsRect(_boundaryRect)


so for cases like this: https://jamboard.google.com/d/1l4Gg2AAs8d_8fGEAK_diDle-9qSQjYYFPkxpzUXAS84/viewer?f=1, this will try to correct the matrix by moving the child to the right edge of the viewport because it's farther, then return the default matrix because the top left corner of the child is inside the viewport?

goderbauer · 2021-07-07T22:05:49Z

@justinmc Do you still have plans for this one?

justinmc · 2021-07-09T18:43:41Z

Yes, but I'll close it for now until I have time to look at it again.

JaffaKetchup · 2022-09-11T19:02:27Z

Any chance of this being looked at again? I understand if you don't have the time!

UdaraWanasinghe · 2024-07-31T05:53:09Z

I have completed a similar project at https://github.com/UdaraWanasinghe/android-transform-layout. The issue with your implementation is that it handles translation, scale, and rotation separately based on the GestureType. But it's possible to do it simultaneously.

justinmc added 23 commits July 18, 2020 11:09

Enable rotation again

dfa0e0b

My problem was not realizing the coord system rotates with the child

29d0bf0

One quick test for getAABBWithRotation

5eff2fa

Merge branch 'master' into iv-rotation

280d92f

Getting the aabb with rotation in a reliable way, but viewport seems off

9c6436b

New approach for determining validity of matrix, looks promising. Old…

d85f2b8

… approach, with axis-aligned boundary, depends on size of viewport, but I didn't realize that before

Code cleanup and TODOs around bug and upcoming validation recovery.

43da15d

Fix for vertical line segment intersection

603223b

Merge branch 'master' into iv-rotation

fbdf64f

WIP outline for correction

8560b55

WIP Implementing closest line rect to linesegment

6d89dd5

WIP testing and making corrections, but still seems off

7f506d9

WIP Still testing and correcting algorithm. It works to slide along e…

fed17ef

…dge when rotated, but gets weird numbers in corners and parallel...

Works but not for corners, can't correct them.

b7366d8

Works for corners, but maybe not corners on corners (2 sides invalid …

544bb2f

…at once)

In the corner case, use the original matrix. Everything seems to work…

a5692b2

…, though there is probably a solution that corrects cases with two invalid sides

Validate rotations

991942e

Tests pass again.

7227a80

Inertia slide test with rotation

778f205

OffsetTuple => ClosestPoints

9b20e5d

Remove unused methods and tests

c3aae14

Remove dead code

1537fa7

Merge branch 'master' into iv-rotation

acb8915

justinmc self-assigned this Jan 5, 2021

flutter-dashboard Bot added the framework flutter/packages/flutter repository. See also f: labels. label Jan 5, 2021

google-cla Bot added the cla: yes label Jan 5, 2021

Merge branch 'master' into iv-rotation

5241858

justinmc commented Jan 6, 2021

View reviewed changes

justinmc added 2 commits January 6, 2021 09:02

Code cleanup

f026413

Fix a few outstanding TODOs.

e8b67d0

justinmc commented Jan 10, 2021

View reviewed changes

justinmc marked this pull request as ready for review February 4, 2021 19:35

justinmc requested a review from LongCatIsLooong February 4, 2021 19:36

LongCatIsLooong reviewed Feb 6, 2021

View reviewed changes

justinmc added 2 commits February 17, 2021 11:04

Allow rotation and panning at the same time, similar to scale.

d873112

Fix findClosestOffsetLineSegment and update tests to reflect

b8d38b8

Merge branch 'master' into iv-rotation

f50423d

LongCatIsLooong reviewed Feb 25, 2021

View reviewed changes

Piinks added the c: new feature Nothing broken; request for a new capability label Jun 10, 2021

justinmc closed this Jul 9, 2021

justinmc mentioned this pull request Oct 29, 2021

InteractiveViewer should support rotation #57698

Open

JaffaKetchup mentioned this pull request Jun 9, 2023

[FEATURE] FlutterMapState over-notifies dependent widgets via updateShouldNotify fleaflet/flutter_map#1547

Closed

	/// Whether two doubles are within a given distance of each other.
	///
	/// The `epsilon` argument must be positive and not null.
	/// The `a` and `b` arguments may be null. A null value is only considered
	/// near-equal to another null value.
	bool nearEqual(double? a, double? b, double epsilon) {
	assert(epsilon != null);
	assert(epsilon >= 0.0);
	if (a == null \|\| b == null)
	return a == b;
	return (a > (b - epsilon)) && (a < (b + epsilon)) \|\| a == b;
	}

	/// Whether a double is within a given distance of zero.
	///
	/// The epsilon argument must be positive.
	bool nearZero(double a, double epsilon) => nearEqual(a, 0.0, epsilon);

Conversation

justinmc commented Jan 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Breaking Change

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!

justinmc commented Feb 4, 2021

Uh oh!

justinmc commented Feb 4, 2021

Uh oh!

RomanJos commented Feb 6, 2021

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 Feb 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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 Feb 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LongCatIsLooong Feb 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LongCatIsLooong Feb 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinmc commented Feb 17, 2021

Uh oh!

RomanJos commented Feb 22, 2021

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!

goderbauer commented Jul 7, 2021

Uh oh!

justinmc commented Jul 9, 2021

Uh oh!

JaffaKetchup commented Sep 11, 2022

Uh oh!

UdaraWanasinghe commented Jul 31, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

justinmc commented Jan 5, 2021 •

edited

Loading

LongCatIsLooong Feb 6, 2021 •

edited

Loading

LongCatIsLooong Feb 24, 2021 •

edited

Loading

LongCatIsLooong Feb 24, 2021 •

edited

Loading

LongCatIsLooong Feb 24, 2021 •

edited

Loading