1069417 - Introduce strongly-typed matrix classes

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Botond Ballo [:botond]]

These would complement our other strongly-typed units well.

What I have in mind is viewing a matrix as a transformation from one coordinate system to another, and therefore templating matrix classes on the source and target units,

For example, we would have a Matrix4x4Typed<SourceUnits, TargetUnits>. Its TransformBounds() method would take a RectTyped<SourceUnits> and return a RectTyped<TargetUnits>.

Comment 1

[Botond Ballo [:botond]]

The general approach I plan to take is:

  - Rename Matrix4x4 to Matrix4x4Typed, and give it two template parameters, 
    SourceUnits and TargetUnits.

  - Adjust the signatures of the methods of Matrix4x4Typed appropriately.
    For example, TransformBounds() would take a RectTyped<SourceUnits>
    instead of a RectTyped<UnknownUnits>.

  - Make Matrix4x4 a typedef for Matrix4x4Typed<UnknownUnits, UnknownUnits>.
    If my reasoning is sound, the original signatures of the methods of
    Matrix4x4 should fall out automatically from this, and no changes to
    client code should be necessary, allowing a gradual transition.

Comment 2

[Matt Woodrow (:mattwoodrow)]

Is this going to work with the changes in bug 1217012?

The to/from coordinate systems of the actual matrix will be float based, but we need to do intermediate calculations in doubles to avoid precision loss.

Comment 3

[Botond Ballo [:botond]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #2)
> Is this going to work with the changes in bug 1217012?

I expect so. We may have to move the definitions of some functions from Matrix.cpp into Matrix.h (because now they'll be templated over more parameters than we can reasonably explicitly instantiate for), but that's already the case without the changes in bug 1217012.

Comment 4

[Botond Ballo [:botond]]

Attachment: Generalize Matrix4x4 to Matrix4x4Typed<SourceUnits, TargetUnits>

This patch implements the approach described in comment 1.

As part of this change, I moved methods of Matrix4x4 that were previously non-templates (or templated only on an 'F' parameter which could be double or float, and explicitly instantiated for both) but now depend on the new 'SourceUnits' and 'TargetUnits' template parameters, from Matrix.cpp to Matrix.h. (I don't think explicitly specializing them for all SourceUnits/TargetUnits combinations is reasonable, as there would be too many.)

Bas, do you think this is acceptable, or should I try harder to keep the code in Matrix.cpp? (I could, for example, extract a base class Matrix4x4Base which is not templated, and keep most of these functions there.)

Comment 5

[Bas Schouten (:bas.schouten)]

Comment on attachment 8691060 [details] [diff] [review]
Generalize Matrix4x4 to Matrix4x4Typed<SourceUnits, TargetUnits>

Review of attachment 8691060 [details] [diff] [review]:
-----------------------------------------------------------------

Looks sane to me!

Comment 6

[Botond Ballo [:botond]]

Attachment: Part 1 - Generalize Matrix4x4 to Matrix4x4Typed<SourceUnits, TargetUnits>

This is the original patch, with a number of mistakes (that I discovered while trying to actually use the typed matrix classes) fixed.

Comment 7

[Botond Ballo [:botond]]

Attachment: Part 2 - Add typedefs for some typed matrices

The approach I'm taking here is to only write typedefs for unit combinations that I actually used so far (in the remaining patches of this bug), and then we can add typedefs as we need them.

If you prefer to pre-declare typedefs for all unit combinations instead, I can do that.

Comment 8

[Botond Ballo [:botond]]

Attachment: Part 3 - Add a ViewAs() overload for matrices

Comment 9

[Botond Ballo [:botond]]

Attachment: Part 4a - Modify TransformTo() and related functions to use typed matrices

This patch modifies the various TransformTo(), TransformVector(), UntransformTo(), and UntransformVector() functions to use typed matrices.

Rather than the typical "let's add typed versions of the functions alongside the untyped versions so we can transition callers gradually" approach, these functions had few enough callers that I decided to just transition them all at once, and not keep around untyped versions.

I separated out the patch that changes the functions themselves (this one) from the patch that updates the call sites (part 4b) for review purposes, but I intend to land them as one patch (as the first doesn't compile without the second).

Comment 10

[Botond Ballo [:botond]]

Attachment: Part 4b - Modify callers of TransformTo() and related functions to use typed matrices

This updates the call sites of TransformTo() & co. as per the previous patch.

All call sites are in APZ or related code, and I propagated the type changes to a few places (e.g. InputBlockState::mTransformToApzc).

There are other places in APZ code where continue to use untyped matrices; these will require some more thought. (For example, it's not clear what the type of AsyncPanZoomController::mAncestorTransform should be.)

Comment 11

[Botond Ballo [:botond]]

Attachment: Part 5 - Remove the explicit template argument of TransformTo() and related functions

Now that TransformTo() & co. take typed matrices as arguments, the information about the target unit type is encoded in the arguments, and it's redundant to specify it explicitly.

Accordingly, I removed the explicit template argument from call sites. Note that there is no loss of type safety as a result, because the target type is determined by the argument matrix type, and if you try to assign the result to a quantity with the wrong units, you'll get an error.

As part of this change, I renamed TransformTo() and UntransformTo() and TransformBy() and UntransformBy() because while e.g.

   TransformTo<LayerPixel>(matrix, point)

made sense,

   TransformTo(matrix, point)

no longer does, but

   TransformBy(matrix, point)

does.

Comment 12

[Botond Ballo [:botond]]

(Yes, I went back to posting patch files rather than using MozReview. I miss the ability to write "notes to the reviewer" as I post each patch.)

Comment 13

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #10)
> All call sites are in APZ or related code, and I propagated the type changes
> to a few places (e.g. InputBlockState::mTransformToApzc).

Oh, there's another thing I should mention about this patch:

At some point, we'll need to decide what types to use for a layer's CSS transform and a layer's async transform. This patch didn't require representing those as typed matrices in many places, but there were a couple of places where I treated the async transform as a ParentLayerToParentLayerMatrix4x4. Let me know what you think about that; I'm open to alternatives.

Comment 14

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8693890 [details] [diff] [review]
Part 4a - Modify TransformTo() and related functions to use typed matrices

Review of attachment 8693890 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM. There's a TransformVector and UntransformVector pair of functions whose opening brace should be on a new line (sorry for not providing quoted context but I can't double-click on a line in Fennec... I guess that is the one way MozReview is better :p).

Comment 15

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8693894 [details] [diff] [review]
Part 4b - Modify callers of TransformTo() and related functions to use typed matrices

Review of attachment 8693894 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/client/TiledContentClient.cpp
@@ +1404,5 @@
>                                     const ViewTransform& aAPZTransform,
>                                     const LayerRect& aClip)
>  {
> +  LayerToParentLayerMatrix4x4 transform = aTransformToCompBounds *
> +      ParentLayerToParentLayerMatrix4x4::FromUnknownMatrix(aAPZTransform);

Any particular reason you didn't use ViewAs here?

Comment 16

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #15)
> Comment on attachment 8693894 [details] [diff] [review]
> Part 4b - Modify callers of TransformTo() and related functions to use typed
> matrices
> 
> Review of attachment 8693894 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/client/TiledContentClient.cpp
> @@ +1404,5 @@
> >                                     const ViewTransform& aAPZTransform,
> >                                     const LayerRect& aClip)
> >  {
> > +  LayerToParentLayerMatrix4x4 transform = aTransformToCompBounds *
> > +      ParentLayerToParentLayerMatrix4x4::FromUnknownMatrix(aAPZTransform);
> 
> Any particular reason you didn't use ViewAs here?

Nope. Will change for consistency.

Comment 17

[Botond Ballo [:botond]]

Addressed review comments locally.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab6a4973e2ab

Comment 18

[Botond Ballo [:botond]]

Some tests related to 3D transforms are failing:

  layout/base/tests/test_preserve3d_sorting_hit_testing.html
  layout/reftests/transform-3d/sorting-2a.html
  layout/reftests/transform-3d/sorting-3a.html

I probably made a mistake somewhere in the first patch.

Comment 19

[Botond Ballo [:botond]]

Looks like I replaced a

  Point2D(point4D.x, point4D.y)

with a

  point4D.As2DPoint()

which is not equivalent! As2DPoint() does:

  Point2D(point4d.x / point4d.w, point4d.y / point4d.w)

Comment 20

[Botond Ballo [:botond]]

I also had a mistake in Matrix4x4Typed::operator*(Point3D) where I introduced a variable 'result' but failed to use it where I inteded.

Comment 21

[Botond Ballo [:botond]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7ff2f5849168

Comment 22

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #21)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=7ff2f5849168

Looking much better. Rebased across bug 1021845 and landed.

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d3d7337cf3f0
https://hg.mozilla.org/integration/mozilla-inbound/rev/998b13d6e17f
https://hg.mozilla.org/integration/mozilla-inbound/rev/bad076546d10
https://hg.mozilla.org/integration/mozilla-inbound/rev/1995fa8bbf43

Comment 24

[Botond Ballo [:botond]]

(In reply to Pulsebot from comment #23)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/d3d7337cf3f0
> https://hg.mozilla.org/integration/mozilla-inbound/rev/998b13d6e17f
> https://hg.mozilla.org/integration/mozilla-inbound/rev/bad076546d10
> https://hg.mozilla.org/integration/mozilla-inbound/rev/1995fa8bbf43

I had a typo in the bug number for the third patch. My apologies. Here's the inbound link for it:

https://hg.mozilla.org/integration/mozilla-inbound/rev/a8d8fc095ebc

Comment 25

[Nigel Babu [:nigelb]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c0f3de2d37b4ef542f61aa5cabf16ac176b0aef1
Bug 1069417 - Fix an error introduced when rebasing across bug 1021845 to reopen a CLOSED TREE. r=bustage

Comment 26

[Botond Ballo [:botond]]

(In reply to Nigel Babu [:nigelb] from comment #25)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> c0f3de2d37b4ef542f61aa5cabf16ac176b0aef1
> Bug 1069417 - Fix an error introduced when rebasing across bug 1021845 to
> reopen a CLOSED TREE. r=bustage

This was because bug 1021845 introduced a call to UntransformTo(), which I needed to change to UntransformBy(). It didn't cause a merge conflict when rebasing, so I missed it.

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d3d7337cf3f0
https://hg.mozilla.org/mozilla-central/rev/998b13d6e17f
https://hg.mozilla.org/mozilla-central/rev/bad076546d10
https://hg.mozilla.org/mozilla-central/rev/1995fa8bbf43
https://hg.mozilla.org/mozilla-central/rev/c0f3de2d37b4