Separating cursor and viewport calculations in the desktop canvas

remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java

Index: remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java
diff --git a/remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java b/remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java
index 35f9ee6709c1beefaa4272a8919d23fdb0364f99..9a15b0139f3796431e8bdd8c529c2598340f27b1 100644
--- a/remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java
+++ b/remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java
@@ -22,12 +22,19 @@ public class DesktopCanvas {
     private final RenderData mRenderData;
 
     /**
+     * Represents the actual center of the viewport.  This value needs to be a pair of floats so the
+     * desktop image can be positioned with sub-pixel accuracy for smoother panning animations at
+     * high zoom levels.
+     */
+    private PointF mViewportPosition = new PointF();
+
+    /**
      * Represents the desired center of the viewport.  This value may not represent the actual
      * center of the viewport as adjustments are made to ensure as much of the desktop is visible as
      * possible.  This value needs to be a pair of floats so the desktop image can be positioned
      * with sub-pixel accuracy for smoother panning animations at high zoom levels.
      */
-    private PointF mViewportPosition = new PointF();
+    private PointF mCursorPosition = new PointF();
 
     /**
      * Represents the amount of space, in pixels, used by System UI.
@@ -40,67 +47,47 @@ public class DesktopCanvas {
     }
 
     /**
-     * Shifts the viewport by the passed in deltas (in image coordinates).
+     * Sets the desired center position of the viewport.
      *
-     * @param useScreenCenter Determines whether to use the desired viewport position or the actual
-     *                        center of the screen for positioning.
-     * @param deltaX The distance (in image coordinates) to move the viewport along the x-axis.
-     * @param deltaY The distance (in image coordinates) to move the viewport along the y-axis.
-     * @return A point representing the new center position of the viewport.
+     * @param newX The new x coordinate value for the desired center position.
+     * @param newY The new y coordinate value for the desired center position.
      */
-    public PointF moveViewportCenter(boolean useScreenCenter, float deltaX, float deltaY) {
-        PointF viewportCenter;
-        if (useScreenCenter) {
-            viewportCenter = getTrueViewportCenter();
-        } else {
-            viewportCenter = new PointF(mViewportPosition.x, mViewportPosition.y);
-        }
-        viewportCenter.offset(deltaX, deltaY);
-
-        RectF bounds = new RectF(0, 0, mRenderData.imageWidth, mRenderData.imageHeight);
-
-        if (viewportCenter.x < bounds.left) {
-            viewportCenter.x = bounds.left;
-        } else if (viewportCenter.x > bounds.right) {
-            viewportCenter.x = bounds.right;
-        }
-
-        if (viewportCenter.y < bounds.top) {
-            viewportCenter.y = bounds.top;
-        } else if (viewportCenter.y > bounds.bottom) {
-            viewportCenter.y = bounds.bottom;
-        }
-
-        mViewportPosition.set(viewportCenter);
-
-        return viewportCenter;
+    public void setViewportCenter(float newX, float newY) {

[Lambros, 2016/09/27 02:16:37]

This should probably be called setCursorAndViewportPositions().
It sets both mCursorPosition and mViewportPosition to the given point.
Then it constrains both values to lie within their allowed regions.

The doc for this public method should make it clear that the viewport is
not actually moved (the matrix is not transformed). This breaks the claimed
attribute of mViewportPosition being the actual center of the viewport.

[joedow, 2016/09/27 20:09:29]

Updating the name is fine, but I'd prefer to name it based on the intention of
the caller (in this case they are setting the cursor position which also adjusts
the viewport.)

I didn't change the calling pattern in my original CL (i.e. callers always
called repositionImage() after calling this method), but the second comment no
longer applies as I've made additional changes.

+        // First set the cursor position since its potential values are a superset of the viewport.
+        mCursorPosition.set(newX, newY);
+        constrainPointToBounds(mCursorPosition, getImageBounds());
+
+        // Now set the viewport position based on the cursor.
+        mViewportPosition.set(mCursorPosition);
+        constrainPointToBounds(mViewportPosition, getViewportBounds());
     }
 
     /**
-     * Sets the desired center position of the viewport.
+     * Shifts the viewport by the passed in values (in image coordinates).
      *
-     * @param newX The new x coordinate value for the desired center position.
-     * @param newY The new y coordinate value for the desired center position.
+     * @param deltaX The distance (in image coordinates) to move the viewport along the x-axis.
+     * @param deltaY The distance (in image coordinates) to move the viewport along the y-axis.
      */
-    public void setViewportPosition(float newX, float newY) {
-        mViewportPosition.set(newX, newY);
+    public void moveViewportCenter(float deltaX, float deltaY) {

[Lambros, 2016/09/27 02:16:37]

Clarify this does not actually move the viewport - the matrix is not changed.
Maybe name it offsetViewportAndSetCursorPosition()?
And reserve the word 'move' for methods that transform the matrix?
(see moveViewportToPoint() which does transform the matrix, but leaves
mViewportPosition untouched)

[joedow, 2016/09/27 20:09:29]

Acknowledged.

+        // Offset and adjust the viewport center position to fit the screen.
+        mViewportPosition.offset(deltaX, deltaY);
+        constrainPointToBounds(mViewportPosition, getViewportBounds());
+
+        // We don't need to constrain the cursor position as the viewport position is always within
+        // the bounds of the potential cursor positions.
+        mCursorPosition.set(mViewportPosition);
     }
 
     /**
-     * Returns the true center position of the viewport (in image coordinates).
+     * Shifts the cursor by the passed in values (in image coordinates) and adjusts the viewport.
      *
-     * @return A point representing the true center position of the viewport.
+     * @param deltaX The distance (in image coordinates) to move the cursor along the x-axis.
+     * @param deltaY The distance (in image coordinates) to move the cursor along the y-axis.
+     * @return A point representing the new cursor position.
      */
-    private PointF getTrueViewportCenter() {
-        // Find the center point of the viewport on the screen.
-        float[] viewportPosition = {
-                ((float) mRenderData.screenWidth / 2), ((float) mRenderData.screenHeight / 2)};
-
-        // Convert the screen position to an image position.
-        Matrix screenToImage = new Matrix();
-        mRenderData.transform.invert(screenToImage);
-        screenToImage.mapPoints(viewportPosition);
-        return new PointF(viewportPosition[0], viewportPosition[1]);
+    public PointF moveViewportWithCursor(float deltaX, float deltaY) {

[Lambros, 2016/09/27 02:16:36]

Maybe offsetCursorAndSetViewportPosition() ?

[joedow, 2016/09/27 20:09:29]

Renamed based on intention of the caller.

+        setViewportCenter(mCursorPosition.x + deltaX, mCursorPosition.y + deltaY);
+        return new PointF(mCursorPosition.x, mCursorPosition.y);
     }
 
     /**
@@ -136,44 +123,136 @@ public class DesktopCanvas {
         if (screenToImageScale > 1.0f) {
             mRenderData.transform.setScale(screenToImageScale, screenToImageScale);
         }
-
-        repositionImage(false);
     }
 
     /**
      * Repositions the image by translating it (without affecting the zoom level).
-     *
-     * @param centerViewport Determines whether the viewport will be translated to the desired
-     *                       center position before being adjusted to fit the screen boundaries.
      */
-    public void repositionImage(boolean centerViewport) {
+    public void repositionImage() {
         // The goal of the code below is to position the viewport as close to the desired center

[Lambros, 2016/09/27 02:16:36]

The phrase 'desired center' is how you've described mCursorPosition. However,
the code below uses mViewportPosition.

[joedow, 2016/09/27 20:09:29]

Acknowledged.

         // position as possible whilst keeping as much of the desktop in view as possible.
         // To achieve these goals, we first position the desktop image at the desired center
         // point and then re-position it to maximize the viewable area.
-        if (centerViewport) {
-            // Map the current viewport position to screen coordinates.
-            float[] viewportPosition = {mViewportPosition.x, mViewportPosition.y};
-            mRenderData.transform.mapPoints(viewportPosition);
+        moveViewportToPoint(mViewportPosition);

[Lambros, 2016/09/27 02:16:37]

This call modifies the transformation matrix so that mViewportPosition is the
actual center of the viewport. This implies that previously it was not, which
implies that this
method acts to restore a consistent internal state.

[joedow, 2016/09/27 20:09:28]

Acknowledged.

+        translateImageToFitScreen();

[Lambros, 2016/09/27 02:16:37]

But this call will modify the matrix *without* updating mViewportPosition,
breaking the internal state again.

Except that it actually doesn't (?) modify the matrix, because mViewportPosition
is
constrained to some bounds that make this a no-op.

[joedow, 2016/09/27 20:09:29]

Acknowledged.

+    }
+
+    /**
+     * Repositions the image by translating and zooming it, to keep the zoom level within sensible
+     * limits. The minimum zoom level is chosen to avoid black space around all 4 sides. The
+     * maximum zoom level is set arbitrarily, so that the user can zoom out again in a reasonable
+     * time, and to prevent arithmetic overflow problems from displaying the image.
+     *
+     * @param centerOnCursor Determines whether the viewport will be translated to the desired
+     *                       center position before being adjusted to fit the screen boundaries.
+     */
+    public void repositionImageWithZoom(boolean centerOnCursor) {
+        // Avoid division by zero in case this gets called before the image size is initialized.
+        if (mRenderData.imageWidth == 0 || mRenderData.imageHeight == 0) {
+            return;
+        }
 
-            float viewportTransX = ((float) mRenderData.screenWidth / 2) - viewportPosition[0];
-            float viewportTransY = ((float) mRenderData.screenHeight / 2) - viewportPosition[1];
+        // Zoom out if the zoom level is too high.
+        float currentZoomLevel = mRenderData.transform.mapRadius(1.0f);
+        if (currentZoomLevel > MAX_ZOOM_FACTOR) {
+            mRenderData.transform.setScale(MAX_ZOOM_FACTOR, MAX_ZOOM_FACTOR);
+        }
 
-            // Translate so the viewport is displayed in the middle of the screen.
-            mRenderData.transform.postTranslate(viewportTransX, viewportTransY);
+        // Get image size scaled to screen coordinates.
+        float[] imageSize = {mRenderData.imageWidth, mRenderData.imageHeight};
+        mRenderData.transform.mapVectors(imageSize);
+
+        if (imageSize[0] < mRenderData.screenWidth && imageSize[1] < mRenderData.screenHeight) {
+            // Displayed image is too small in both directions, so apply the minimum zoom
+            // level needed to fit either the width or height.
+            float scale = Math.min((float) mRenderData.screenWidth / mRenderData.imageWidth,
+                                   (float) mRenderData.screenHeight / mRenderData.imageHeight);
+            mRenderData.transform.setScale(scale, scale);
         }
 
+        if (centerOnCursor) {
+            moveViewportToPoint(mCursorPosition);
+        }
+
+        translateImageToFitScreen();
+
+        // Update our position members to reflect the new position of the image.
+        PointF newPosition;
+        if (centerOnCursor) {
+            newPosition  = new PointF(mCursorPosition.x, mCursorPosition.y);
+        } else {
+            float screenCenterX = (float) mRenderData.screenWidth / 2;
+            float screenCenterY = (float) mRenderData.screenHeight / 2;
+            float[] mappedPoints = {screenCenterX, screenCenterY};
+            Matrix screenToImage = new Matrix();
+            mRenderData.transform.invert(screenToImage);
+            screenToImage.mapPoints(mappedPoints);
+            newPosition = new PointF(mappedPoints[0], mappedPoints[1]);
+        }
+        setViewportCenter(newPosition.x, newPosition.y);
+    }
+
+    /**
+     * Updates the given point such that it refers to a coordinate within the bounds provided.
+     *
+     * @param point The point to adjust, the original object is modified.
+     * @param bounds The bounds used to constrain the point.
+     */
+    private void constrainPointToBounds(PointF point, RectF bounds) {
+        if (point.x < bounds.left) {
+            point.x = bounds.left;
+        } else if (point.x > bounds.right) {
+            point.x = bounds.right;
+        }
+
+        if (point.y < bounds.top) {
+            point.y = bounds.top;
+        } else if (point.y > bounds.bottom) {
+            point.y = bounds.bottom;
+        }
+    }
+
+    /** Returns a region which defines the set of valid cursor values in image space. */
+    private RectF getImageBounds() {
+        return new RectF(0, 0, mRenderData.imageWidth, mRenderData.imageHeight);
+    }
+
+    /** Returns a region which defines the set of valid viewport center values in image space. */
+    private RectF getViewportBounds() {

[Lambros, 2016/09/27 02:16:37]

IIUC, this is precisely the set of points for which translateImageToFitScreen()
would be a no-op if the viewport were already centered on the point.

I think this fact is crucial to understanding where you're going with this CL :)
I think you're headed for a place where translateImageToFitScreen() is always a
no-op so it can be removed completely?

[joedow, 2016/09/27 20:09:29]

I was moving to a place where multiple transforms weren't needed as the
coordinates would never be messed up.  I think the diffing tool made the change
seem much larger than it actually was but I've done a bit more work just to get
rid of the need for the additional transformations in this CL.

+        float[] screenVectors = {
+                ((float) mRenderData.screenWidth / 2), ((float) mRenderData.screenHeight / 2)};
+        Matrix screenToImage = new Matrix();
+        mRenderData.transform.invert(screenToImage);
+        screenToImage.mapVectors(screenVectors);
+        return new RectF(screenVectors[0], screenVectors[1],
+                mRenderData.imageWidth - screenVectors[0],
+                mRenderData.imageHeight - screenVectors[1]);
+    }
+
+    /** Centers the viewport on the passed in point. */
+    private void moveViewportToPoint(PointF point) {
+        // Map the current viewport position to screen coordinates.
+        float[] viewportPosition = {point.x, point.y};
+        mRenderData.transform.mapPoints(viewportPosition);
+
+        float viewportTransX = ((float) mRenderData.screenWidth / 2) - viewportPosition[0];
+        float viewportTransY = ((float) mRenderData.screenHeight / 2) - viewportPosition[1];
+
+        // Translate so the viewport is displayed in the middle of the screen.
+        mRenderData.transform.postTranslate(viewportTransX, viewportTransY);
+    }
+
+    /** Repositions the image by translating it to maximize the amount of viewable content. */
+    private void translateImageToFitScreen() {
         // Get the coordinates of the desktop rectangle (top-left/bottom-right corners) in
         // screen coordinates. Order is: left, top, right, bottom.
         RectF rectScreen = new RectF(0, 0, mRenderData.imageWidth, mRenderData.imageHeight);
         mRenderData.transform.mapRect(rectScreen);
 
         float leftDelta = rectScreen.left;
-        float rightDelta =
-                rectScreen.right - mRenderData.screenWidth + mSystemUiOffsetPixels.right;
+        float rightDelta = rectScreen.right - mRenderData.screenWidth;
         float topDelta = rectScreen.top;
-        float bottomDelta =
-                rectScreen.bottom - mRenderData.screenHeight + mSystemUiOffsetPixels.bottom;
+        float bottomDelta = rectScreen.bottom - mRenderData.screenHeight;
         float xAdjust = 0;
         float yAdjust = 0;
 
@@ -201,40 +280,4 @@ public class DesktopCanvas {
 
         mRenderStub.setTransformation(mRenderData.transform);
     }
-
-    /**
-     * Repositions the image by translating and zooming it, to keep the zoom level within sensible
-     * limits. The minimum zoom level is chosen to avoid black space around all 4 sides. The
-     * maximum zoom level is set arbitrarily, so that the user can zoom out again in a reasonable
-     * time, and to prevent arithmetic overflow problems from displaying the image.
-     *
-     * @param centerViewport Determines whether the viewport will be translated to the desired
-     *                       center position before being adjusted to fit the screen boundaries.
-     */
-    public void repositionImageWithZoom(boolean centerViewport) {
-        // Avoid division by zero in case this gets called before the image size is initialized.
-        if (mRenderData.imageWidth == 0 || mRenderData.imageHeight == 0) {
-            return;
-        }
-
-        // Zoom out if the zoom level is too high.
-        float currentZoomLevel = mRenderData.transform.mapRadius(1.0f);
-        if (currentZoomLevel > MAX_ZOOM_FACTOR) {
-            mRenderData.transform.setScale(MAX_ZOOM_FACTOR, MAX_ZOOM_FACTOR);
-        }
-
-        // Get image size scaled to screen coordinates.
-        float[] imageSize = {mRenderData.imageWidth, mRenderData.imageHeight};
-        mRenderData.transform.mapVectors(imageSize);
-
-        if (imageSize[0] < mRenderData.screenWidth && imageSize[1] < mRenderData.screenHeight) {
-            // Displayed image is too small in both directions, so apply the minimum zoom
-            // level needed to fit either the width or height.
-            float scale = Math.min((float) mRenderData.screenWidth / mRenderData.imageWidth,
-                                   (float) mRenderData.screenHeight / mRenderData.imageHeight);
-            mRenderData.transform.setScale(scale, scale);
-        }
-
-        repositionImage(centerViewport);
-    }
 }