Closed
Bug 1063224
Opened 10 years ago
Closed 10 years ago
APZ uses DPI in calculations involving coordinates that aren't in global screen space
Categories
(Core :: Panning and Zooming, defect)
Tracking
()
RESOLVED
FIXED
mozilla35
People
(Reporter: botond, Assigned: botond)
Details
Attachments
(7 files, 6 obsolete files)
(deleted),
patch
|
kats
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
kats
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
kats
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
kats
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
kats
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
kats
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
kats
:
review+
|
Details | Diff | Splinter Review |
APZCTreeManager::GetDPI() is used in three places in AsyncPanZoomController and Axis [1] [2] [3].
In each case, we multiply the DPI by a quantity in inches (or something related like inches per second) - a unit that only makes sense in the context of global screen space.
However, in each case, we compare the resulting quantity to a quantity in an APZC's local screen space - a coordinate system that can potentially be (very) different from global screen space.
We should fix this, by converting the second quantity into global screen space (with the tree manager's help) before performing the comparison.
[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/AsyncPanZoomController.cpp?rev=9fe5fe620802#820
[2] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/AsyncPanZoomController.cpp?rev=9fe5fe620802#1637
[3] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/Axis.cpp?rev=b54f8eca9cb9#51
Assignee | ||
Comment 1•10 years ago
|
||
Attachment #8485287 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 2•10 years ago
|
||
Attachment #8485288 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 3•10 years ago
|
||
Attachment #8485289 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 4•10 years ago
|
||
Attachment #8485290 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 5•10 years ago
|
||
Attachment #8485291 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 6•10 years ago
|
||
Attachment #8485292 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 7•10 years ago
|
||
Attachment #8485293 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 8•10 years ago
|
||
failure try |
Updated•10 years ago
|
Attachment #8485287 -
Flags: review?(bugmail.mozilla) → review+
Comment 9•10 years ago
|
||
Comment on attachment 8485289 [details] [diff] [review]
Patch 3 - Make some more things strongly typed in APZ code
Review of attachment 8485289 [details] [diff] [review]:
-----------------------------------------------------------------
::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +1782,5 @@
> angle = fabs(angle); // range [0, pi]
>
> float breakThreshold = gfxPrefs::APZAxisBreakoutThreshold() * APZCTreeManager::GetDPI();
>
> + if (fabs(aDelta.x) > breakThreshold || fabs(aDelta.y) > breakThreshold) {
The fact that you can use fabs on a ScreenCoord here without any trouble seems to imply that you don't need gfx::Abs at all. Coord<float> has an implicit constructor, so you should be able to do ScreenCoord foo = fabs(bar) where bar is also a ScreenCoord. Does that not work?
Comment 10•10 years ago
|
||
Comment on attachment 8485291 [details] [diff] [review]
Patch 5 - Give AsyncPanZoomController a mechanism for converting between local and global screen coordinates
Review of attachment 8485291 [details] [diff] [review]:
-----------------------------------------------------------------
::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ +979,5 @@
>
> +Matrix4x4
> +APZCTreeManager::TransformFromGlobalScreenCoordinates(const AsyncPanZoomController* aApzc) const {
> + Matrix4x4 transformToApzc;
> + Matrix4x4 transformToGecko; // ignored
We already have a bunch of places where we do this sort of thing (call GetInputTransforms with an ignored transformToGecko). It seems to me it would be better to just make the transformToGecko parameter optional in GetInputTransforms. Or maybe even split it into two separate functions, which might make it clearer.
Matrix4x4 GetScreenToApzcTransform(const AsyncPanZoomController*) const;
Matrix4x4 GetApzcToGeckoTransform(const AsyncPanZoomController*) const;
Comment 11•10 years ago
|
||
Leaving the reviews for now as the above changes might be fairly significant so I don't want to spend time reviewing details if the overall structure will change.
Assignee | ||
Comment 12•10 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #9)
> Comment on attachment 8485289 [details] [diff] [review]
> Patch 3 - Make some more things strongly typed in APZ code
>
> Review of attachment 8485289 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: gfx/layers/apz/src/AsyncPanZoomController.cpp
> @@ +1782,5 @@
> > angle = fabs(angle); // range [0, pi]
> >
> > float breakThreshold = gfxPrefs::APZAxisBreakoutThreshold() * APZCTreeManager::GetDPI();
> >
> > + if (fabs(aDelta.x) > breakThreshold || fabs(aDelta.y) > breakThreshold) {
>
> The fact that you can use fabs on a ScreenCoord here without any trouble
> seems to imply that you don't need gfx::Abs at all.
(aDelta.x is no longer a ScreenCoord, but your point below is still valid.)
> Coord<float> has an
> implicit constructor, so you should be able to do ScreenCoord foo =
> fabs(bar) where bar is also a ScreenCoord. Does that not work?
That does appear to work - thanks!
I remember having tried this before, and
fabs(mPos - mStartPos)
giving a compiler error, so I had to change it to
fabs((mPos - mStartPos).value)
but I guess that was back when mPos and mStartPos were integers, and now they are floats thanks to bug 1046993.
I'll update the patches to get rid of Abs() and just use fabs().
Assignee | ||
Comment 13•10 years ago
|
||
Attachment #8486048 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Updated•10 years ago
|
Attachment #8485288 -
Attachment is obsolete: true
Attachment #8485288 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 14•10 years ago
|
||
Attachment #8486057 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Updated•10 years ago
|
Attachment #8485290 -
Attachment is obsolete: true
Attachment #8485290 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Updated•10 years ago
|
Attachment #8485289 -
Attachment is obsolete: true
Attachment #8485289 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Updated•10 years ago
|
Attachment #8486057 -
Attachment description: Patch 4 - Add a bit more infrastructure to Axis → Patch 3 - Add a bit more infrastructure to Axis
Assignee | ||
Comment 15•10 years ago
|
||
Attachment #8486058 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 16•10 years ago
|
||
Attachment #8486059 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 17•10 years ago
|
||
Attachment #8486060 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 18•10 years ago
|
||
Attachment #8486061 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Updated•10 years ago
|
Attachment #8485291 -
Attachment is obsolete: true
Attachment #8485291 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Updated•10 years ago
|
Attachment #8485292 -
Attachment is obsolete: true
Attachment #8485292 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Updated•10 years ago
|
Attachment #8485293 -
Attachment is obsolete: true
Attachment #8485293 -
Flags: review?(bugmail.mozilla)
Updated•10 years ago
|
Attachment #8486048 -
Flags: review?(bugmail.mozilla) → review+
Comment 19•10 years ago
|
||
Comment on attachment 8486058 [details] [diff] [review]
Patch 4 - Split APZCTreeManager::GetInputTransforms() into GetScreenToApzcTransform() and GetApzcToGeckoTransform()
Review of attachment 8486058 [details] [diff] [review]:
-----------------------------------------------------------------
Much nicer, thanks!
::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ +1309,5 @@
> The APZCs also obviously have LT, LN, PT, and PN, so all of the above transformation combinations
> required can be generated.
> */
> +
> +/**
Use /*
The "long comment" won't show up in generated docs, but this comment will if you use /**, which means it will refer to something that is not present.
@@ +1329,5 @@
> Matrix4x4 ancestorUntransform = aApzc->GetAncestorTransform();
> ancestorUntransform.Invert();
> // asyncUntransform is LA.Inverse()
> Matrix4x4 asyncUntransform = aApzc->GetCurrentAsyncTransform();
> asyncUntransform.Invert();
Remvoe asyncUntransform here and just declare it inside the loop, since it's not actually used at this point
@@ +1355,5 @@
> +
> + return result;
> +}
> +
> +/**
/*
Attachment #8486058 -
Flags: review?(bugmail.mozilla) → review+
Comment 20•10 years ago
|
||
Comment on attachment 8486059 [details] [diff] [review]
Patch 5 - Give AsyncPanZoomController a mechanism for converting between local and global screen coordinates
Review of attachment 8486059 [details] [diff] [review]:
-----------------------------------------------------------------
::: gfx/layers/apz/src/AsyncPanZoomController.h
@@ +328,5 @@
>
> + /**
> + * Convert the vector |aVector|, rooted at the point |aAnchor|, from
> + * this APZC's local screen coordinates into global screen coordinates.
> + * The anchor is necessary because with 3D tranforms, the local of the vector
local? Not sure that's the word you meant to use. Ditto other comment below
Attachment #8486059 -
Flags: review?(bugmail.mozilla) → review+
Updated•10 years ago
|
Attachment #8486060 -
Flags: review?(bugmail.mozilla) → review+
Comment 21•10 years ago
|
||
Comment on attachment 8486061 [details] [diff] [review]
Patch 7 - Use global screen coordinates when working with quantities in inches
Review of attachment 8486061 [details] [diff] [review]:
-----------------------------------------------------------------
::: gfx/layers/apz/src/Axis.cpp
@@ +50,5 @@
> float newVelocity = mAxisLocked ? 0.0f : (float)(mPos - aPos) / (float)(aTimestampMs - mPosTimeMs);
> if (gfxPrefs::APZMaxVelocity() > 0.0f) {
> + ScreenPoint maxVelocity = MakePoint(gfxPrefs::APZMaxVelocity() * APZCTreeManager::GetDPI());
> + mAsyncPanZoomController->ToLocalScreenCoordinates(&maxVelocity, mAsyncPanZoomController->PanStart());
> + newVelocity = std::min(newVelocity, maxVelocity.Length());
This makes me realize the max-velocity clamp is a per-axis max, which means diagonal pans might exceed the max-velocity. I don't really care though.
Attachment #8486061 -
Flags: review?(bugmail.mozilla) → review+
Updated•10 years ago
|
Attachment #8486057 -
Flags: review?(bugmail.mozilla) → review+
Comment 22•10 years ago
|
||
Sweet, nice work :)
Assignee | ||
Comment 23•10 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #20)
> ::: gfx/layers/apz/src/AsyncPanZoomController.h
> @@ +328,5 @@
> >
> > + /**
> > + * Convert the vector |aVector|, rooted at the point |aAnchor|, from
> > + * this APZC's local screen coordinates into global screen coordinates.
> > + * The anchor is necessary because with 3D tranforms, the local of the vector
>
> local? Not sure that's the word you meant to use. Ditto other comment below
Thanks for catching that. I meant "location", but my mind must have been occupied too much by the words "local" and "global" :)
Assignee | ||
Comment 24•10 years ago
|
||
landing |
https://hg.mozilla.org/integration/mozilla-inbound/rev/3065e0ac5ab3
https://hg.mozilla.org/integration/mozilla-inbound/rev/2549a67a8041
https://hg.mozilla.org/integration/mozilla-inbound/rev/3220317cf03a
https://hg.mozilla.org/integration/mozilla-inbound/rev/600d8514b1f4
https://hg.mozilla.org/integration/mozilla-inbound/rev/0016208a9220
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a9879a291a9
https://hg.mozilla.org/integration/mozilla-inbound/rev/879dcad4aded
Assignee | ||
Comment 25•10 years ago
|
||
Addressed review comments and landed.
Comment 26•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/3065e0ac5ab3
https://hg.mozilla.org/mozilla-central/rev/2549a67a8041
https://hg.mozilla.org/mozilla-central/rev/3220317cf03a
https://hg.mozilla.org/mozilla-central/rev/600d8514b1f4
https://hg.mozilla.org/mozilla-central/rev/0016208a9220
https://hg.mozilla.org/mozilla-central/rev/8a9879a291a9
https://hg.mozilla.org/mozilla-central/rev/879dcad4aded
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in
before you can comment on or make changes to this bug.
Description
•