Introduce strongly-typed matrix classes

RESOLVED FIXED in Firefox 45

Status

()

defect
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: botond, Assigned: botond)

Tracking

Trunk
mozilla45
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 fixed)

Details

Attachments

(6 attachments, 1 obsolete attachment)

(Assignee)

Description

5 years ago
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>.
(Assignee)

Comment 1

4 years ago
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.
Assignee: nobody → botond
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.
(Assignee)

Comment 3

4 years ago
(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.
(Assignee)

Comment 4

3 years ago
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.)
Attachment #8691060 - Flags: feedback?(bas)
Comment on attachment 8691060 [details] [diff] [review]
Generalize Matrix4x4 to Matrix4x4Typed<SourceUnits, TargetUnits>

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

Looks sane to me!
Attachment #8691060 - Flags: feedback?(bas) → feedback+
(Assignee)

Comment 6

3 years ago
This is the original patch, with a number of mistakes (that I discovered while trying to actually use the typed matrix classes) fixed.
Attachment #8691060 - Attachment is obsolete: true
Attachment #8693886 - Flags: review?(bas)
(Assignee)

Comment 7

3 years ago
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.
Attachment #8693887 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 8

3 years ago
Attachment #8693888 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 9

3 years ago
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).
Attachment #8693890 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 10

3 years ago
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.)
Attachment #8693894 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 11

3 years ago
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.
Attachment #8693895 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 12

3 years ago
(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.)
Attachment #8693887 - Flags: review?(bugmail.mozilla) → review+
Attachment #8693888 - Flags: review?(bugmail.mozilla) → review+
(Assignee)

Comment 13

3 years ago
(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 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).
Attachment #8693890 - Flags: review?(bugmail.mozilla) → review+
Attachment #8693886 - Flags: review?(bas) → review+
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?
Attachment #8693894 - Flags: review?(bugmail.mozilla) → review+
Attachment #8693895 - Flags: review?(bugmail.mozilla) → review+
(Assignee)

Comment 16

3 years ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
(Assignee)

Comment 18

3 years ago
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.
(Assignee)

Comment 19

3 years ago
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)
(Assignee)

Comment 20

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

Comment 22

3 years ago
(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.
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
(Assignee)

Comment 26

3 years ago
(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.
(Assignee)

Updated

3 years ago
Blocks: 1236750

Updated

3 years ago
Depends on: 1261310
(Assignee)

Updated

3 years ago
No longer depends on: 1261310
(Assignee)

Updated

2 years ago
No longer depends on: 1376522
You need to log in before you can comment on or make changes to this bug.