Closed Bug 923512 Opened 6 years ago Closed 5 years ago

Introduce strongly typed coordinate classes

Categories

(Core :: Graphics: Layers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: botond, Assigned: botond)

References

(Depends on 1 open bug)

Details

Attachments

(3 files, 9 obsolete files)

7.49 KB, patch
kats
: review+
Details | Diff | Splinter Review
40.57 KB, patch
botond
: review+
Details | Diff | Splinter Review
27.47 KB, patch
kats
: review+
Details | Diff | Splinter Review
Our strongly typed point, size, rect, and margin classes have been very useful for catching bugs caused by mismatched units.

One hole in this safety net is single coordinates, for which we still use plain floats. 'Axis' in particular uses single coordinates extensively. Strong typing for these could cause bugs like bug 923431 to be discovered sooner.

I propose that we introduce a new Coord class, templated on a pixel type, which represents a single coordinate. I think we can add this without disturbing any other code for now, and just use it in Axis. We can then gradually introduce it into other code if desired.
Bas, what do you think about this idea?
Flags: needinfo?(bas)
I think it might certainly be a valid idea. Although since this will be mainly used in layout I suppose the layout team's opinion might matter more :).
Flags: needinfo?(bas)
Assignee: nobody → botond
Attached patch WIP patch (obsolete) — Splinter Review
Attached is a WIP patch that:

   - introduces BaseCoord<>, IntCoordTyped<> and CoordTyped<> class templates,
     in similar vein to BasePoint<>, IntPointTyped<> and PointTyped<>

   - modifies APZ's Axis class to use these instead of plain floats

   - make necessary changes to other parts of the codebase to keep it
     compilable

This currently compiles for me locally, with a Linux desktop build, except for an internal compiler error in nsEventStateManager.cpp.
(In reply to Botond Ballo [:botond] from comment #3)
> This currently compiles for me locally, with a Linux desktop build, except
> for an internal compiler error in nsEventStateManager.cpp.

I reported the ICE at https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61959.
(In reply to comment #4)
> (In reply to Botond Ballo [:botond] from comment #3)
> > This currently compiles for me locally, with a Linux desktop build, except
> > for an internal compiler error in nsEventStateManager.cpp.
> 
> I reported the ICE at https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61959.

Living on the edge.  :-)
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #5)
> > I reported the ICE at https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61959.
> 
> Living on the edge.  :-)

I posted a minimal example that reproduces the crash at https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61959#c4. It doesn't do anything novel, it just extends a pattern we already used for one concept (BasePoint/PointTyped) to another (BaseCoord/CoordTyped).
I found a workaround for the ICE: not deriving 'TypedCoord<units>' and 'IntTypedCoord<units>' from 'units'.

The purpose of deriving 'TypedPoint<units>' and 'IntTypedPoint<units>' from 'units' is so that we can do things like 'LayoutDevicePoint::FromUntyped(point)' (with 'FromUntyped' being defined by 'LayoutDevicePixel'). I think we can live with not being able to call functions defined in the pixel classes via 'LayoutDeviceCoord::' and such.
Attached patch WIP patch (obsolete) — Splinter Review
Updated patch to fix ICE. Patch now compiles locally.

Now, let's see if it compilers on other platforms: https://tbpl.mozilla.org/?tree=Try&rev=2306c0af7b54
Attachment #8464279 - Attachment is obsolete: true
Cleaned up the patch a bit. Will wait for Try results before posting for review.
Attachment #8464797 - Attachment is obsolete: true
Fixed some Werror and Windows bustage. ReTrying (build-only): https://tbpl.mozilla.org/?tree=Try&rev=f8f1c55dc774
Fixed more Windows bustage. Trying again, Windows builds only this time: https://tbpl.mozilla.org/?tree=Try&rev=3189dc876731
Updated patch will all bustages fixed. Now builds on all platforms.

Posting for review. A few design notes:

   - I transitioned the 'x' and 'y' fields of the Point classes
     to be Coords rather than plain numbers, because this was
     needed for using coordinate classes effectively in Axis
     (which is my poster child for this change). This had
     repercussions in the rest of the codebase, but they were
     relatively limited, i.e. I only had to make changes in
     a handful of places.

     I plan to similarly transition Size, Rect, and Margin,
     but I propose doing this as a follow-up, and possibly
     after some other parts of the code have been transitioned
     to use Coord, because doing it now would involve a lot
     of changes in many places.

   - Coords are currently implicitly convertible to the numeric
     type they wrap, and I provide operator overloads for most
     mixed-type operations on Coords (e.g. adding a Coord and
     a float). This, again, is to ease the transition to Coords.
     Once enough of the codebase has been transitioned to use
     Coords, most of these can be removed, and the benefits of
     strong typing realized more fully.
Attachment #8464809 - Attachment is obsolete: true
Attachment #8465016 - Flags: review?(bugmail.mozilla)
(In reply to Botond Ballo [:botond] from comment #12)
> Posting for review.

Suggested review order:

  - BaseCoord.h, Coord.h, BasePoint.h, Point.h, Units.h 
    (and moz.build, GfxMessageUtils.h)
    These are the core changes.

  - Axis.h/cpp, AsyncPanZoomController.cpp, APZCTreeManager.cpp
    These demonstrate the coordinate classes in action.

  - Everything else is fallout from changing the types of Point's
    x and y coordinates.
Comment on attachment 8465016 [details] [diff] [review]
Introduce strongly-typed coordinate classes

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

Bas should review the changes in gfx/2d/. Minusing for the Truncate[d] issue, but generally this looks ok to me. Do you know what kind of impact this will have on performance?

::: gfx/2d/Coord.h
@@ +104,5 @@
> +  IntCoordTyped<units> Rounded() const {
> +    return IntCoordTyped<units>(int32_t(floor(this->value + 0.5)));
> +  }
> +  IntCoordTyped<units> Truncated() const {
> +    return IntCoordTyped<units>(int32_t(this->value));

Truncate() and Truncated() will give you different results for negative numbers. floor(-3.5) will give -4 but int32_t(-3.5) will give you -3. At least that's what happens on my mac.

::: gfx/layers/apz/src/Axis.cpp
@@ +22,1 @@
>  namespace mozilla {

Put blank line back. Also in general better to separate the cosmetic changes from the functional changes in this file.

::: gfx/layers/opengl/CompositorOGL.cpp
@@ +760,5 @@
>    // target to stop tests failing - this is only used by the Android browser
>    // UI for its dynamic toolbar.
>    IntPoint origin;
>    if (!mTarget) {
> +    origin = -RoundedToInt(mRenderOffset.ToUnknownPoint());

This adds a rounding where there was none previously. Intentional?
Attachment #8465016 - Flags: review?(bugmail.mozilla)
Attachment #8465016 - Flags: review?(bas)
Attachment #8465016 - Flags: review-
Updated patch; addresses review comments.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #14)
> Do you know what kind of impact this will have on performance?

I would expect no performance impact, as the compiler should easily be able to optimize away the extra class wrapper we're adding.

> ::: gfx/2d/Coord.h
> @@ +104,5 @@
> > +  IntCoordTyped<units> Rounded() const {
> > +    return IntCoordTyped<units>(int32_t(floor(this->value + 0.5)));
> > +  }
> > +  IntCoordTyped<units> Truncated() const {
> > +    return IntCoordTyped<units>(int32_t(this->value));
> 
> Truncate() and Truncated() will give you different results for negative
> numbers. floor(-3.5) will give -4 but int32_t(-3.5) will give you -3. At
> least that's what happens on my mac.

Good catch! I changed both to be int32_t(value) to be consistent.

> ::: gfx/layers/apz/src/Axis.cpp
> @@ +22,1 @@
> >  namespace mozilla {
> 
> Put blank line back. Also in general better to separate the cosmetic changes
> from the functional changes in this file.

Eek! I must have invoked Eclipse's auto-format shortcut, and then saved without realizing it. Fixed.

> ::: gfx/layers/opengl/CompositorOGL.cpp
> @@ +760,5 @@
> >    // target to stop tests failing - this is only used by the Android browser
> >    // UI for its dynamic toolbar.
> >    IntPoint origin;
> >    if (!mTarget) {
> > +    origin = -RoundedToInt(mRenderOffset.ToUnknownPoint());
> 
> This adds a rounding where there was none previously. Intentional?

There was an implicit truncation previously. To preserve the behaviour I changed this to be an (explicit) truncation.
Attachment #8465016 - Attachment is obsolete: true
Attachment #8465016 - Flags: review?(bas)
Attachment #8465556 - Flags: review?(bugmail.mozilla)
Attachment #8465556 - Flags: review?(bas)
Comment on attachment 8465556 [details] [diff] [review]
Introduce strongly-typed coordinate classes

Not sure why this had Kats marked for review twice.
Attachment #8465556 - Flags: review?(bugmail.mozilla)
Comment on attachment 8465556 [details] [diff] [review]
Introduce strongly-typed coordinate classes

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

I looked at all the stuff outside gfx/2d/ and the {Base,}{Point,Coord}.h files inside gfx/2d/. Didn't look at the other changes in gfx/2d/.

::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ +614,5 @@
>      gfx3DMatrix transformToGecko;
>      GetInputTransforms(apzc, transformToApzc, transformToGecko);
>      gfx3DMatrix outTransform = transformToApzc * transformToGecko;
> +    aOutTransformedPoint->x = LayoutDeviceIntCoord(aPoint.x);
> +    aOutTransformedPoint->y = LayoutDeviceIntCoord(aPoint.y);

Already getting into bad habits of casting from one pixel type to another without explanation! :p

::: gfx/layers/apz/src/Axis.h
@@ +54,5 @@
>    /**
>     * Notify this Axis that a new touch has been received, including a timestamp
>     * for when the touch was received. This triggers a recalculation of velocity.
>     */
> +  void UpdateWithTouchAtDevicePoint(ScreenIntCoord aPos, uint32_t aTimestampMs);

We should probably change the ScreenIntCoords in this file to ScreenCoords. No sense rounding/truncating unless needed. That should be deferred to a follow-up bug though.

::: gfx/layers/composite/ContainerLayerComposite.cpp
@@ +271,2 @@
>    const gfx::Point layerTransform = GetScrollData(aLayer);
> +  const gfx::Point layerScroll = scrollOffset.ToUnknownPoint() - layerTransform;

Check with BenWa to make sure the rounding that was here before isn't intentional. It looks kind of suspicious.
Attachment #8465556 - Flags: review?(bugmail.mozilla) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #17)
> ::: gfx/layers/apz/src/APZCTreeManager.cpp
> @@ +614,5 @@
> >      gfx3DMatrix transformToGecko;
> >      GetInputTransforms(apzc, transformToApzc, transformToGecko);
> >      gfx3DMatrix outTransform = transformToApzc * transformToGecko;
> > +    aOutTransformedPoint->x = LayoutDeviceIntCoord(aPoint.x);
> > +    aOutTransformedPoint->y = LayoutDeviceIntCoord(aPoint.y);
> 
> Already getting into bad habits of casting from one pixel type to another
> without explanation! :p

I believe the code is correct, since we are applying a transform to the point to get it into LayoutDevice space.

One day, I hope to introduce strongly-typed matrix classes so that we can encode into the type of 'outTransform' that it converts from Screen space to LayoutDevice space.

In the meantime, I rewrote the code to use TransformTo<>(), which makes it look a bit better. This involved introducing an overload of TransformTo<>() that applies to IntPoints. Re-requesting review for these bits.

> ::: gfx/layers/apz/src/Axis.h
> @@ +54,5 @@
> >    /**
> >     * Notify this Axis that a new touch has been received, including a timestamp
> >     * for when the touch was received. This triggers a recalculation of velocity.
> >     */
> > +  void UpdateWithTouchAtDevicePoint(ScreenIntCoord aPos, uint32_t aTimestampMs);
> 
> We should probably change the ScreenIntCoords in this file to ScreenCoords.
> No sense rounding/truncating unless needed. That should be deferred to a
> follow-up bug though.

Filed bug 1046993 for this.

> ::: gfx/layers/composite/ContainerLayerComposite.cpp
> @@ +271,2 @@
> >    const gfx::Point layerTransform = GetScrollData(aLayer);
> > +  const gfx::Point layerScroll = scrollOffset.ToUnknownPoint() - layerTransform;
> 
> Check with BenWa to make sure the rounding that was here before isn't
> intentional. It looks kind of suspicious.

Checked with Mason and BenWa; the rounding wasn't intentional.
Attachment #8465556 - Attachment is obsolete: true
Attachment #8465556 - Flags: review?(bas)
Attachment #8465702 - Flags: review?(bugmail.mozilla)
Attachment #8465702 - Flags: review?(bas)
Attachment #8465702 - Flags: review?(bugmail.mozilla) → review+
I've already looked a little bit at the Moz2D parts and haven't seen anything particularly objectionable there, why are there a few places where we need to .value though?

Also, could you please split Moz2D changes up in a separate patch that goes in the queue before the rest? That makes everything a little cleaner.
(In reply to Bas Schouten (:bas.schouten) from comment #19)
> I've already looked a little bit at the Moz2D parts and haven't seen
> anything particularly objectionable there, why are there a few places where
> we need to .value though?

Sometimes we do things like 'sqrt(point.x * point.x + point.y * point.y)'. I don't want to define an operator*(Coord, Coord) until we have a meaningful strongly-typed result type that would make sense for it (like an Area or something).

> Also, could you please split Moz2D changes up in a separate patch that goes
> in the queue before the rest? That makes everything a little cleaner.

Will do.
Attached patch Part 1 - Moz2D changes (obsolete) — Splinter Review
Attachment #8465702 - Attachment is obsolete: true
Attachment #8465702 - Flags: review?(bas)
Attachment #8465754 - Flags: review?(bas)
Carrying r+ from Kats. Bas, if you'd like to review this part as well, please feel free.
Attachment #8465755 - Flags: review+
Comment on attachment 8465754 [details] [diff] [review]
Part 1 - Moz2D changes

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

I'm not 100% sure I agree with not allowing multiplication. But I'm also sensitive to your argument of it being nice to have a valid unit for those. For now let's just get this stuff in.
Attachment #8465754 - Flags: review?(bas) → review+
This additional bit became necessary during rebasing.
Attachment #8468546 - Flags: review?(bugmail.mozilla)
Attached patch Part 1 - Moz2D changes (obsolete) — Splinter Review
Rebased, carrying r+
Attachment #8465754 - Attachment is obsolete: true
Attachment #8468548 - Flags: review+
Rebased, carrying r+
Attachment #8465755 - Attachment is obsolete: true
Attachment #8468549 - Flags: review+
Another build-only Try push to make sure nothing has broken since last time:

https://tbpl.mozilla.org/?tree=Try&rev=982e5d1e578e
Attachment #8468546 - Flags: review?(bugmail.mozilla) → review+
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/d577815636c6 for mochitest-2 failures: https://tbpl.mozilla.org/php/getParsedLog.php?id=45371969&tree=Mozilla-Inbound


Reftest failed once on this push with https://tbpl.mozilla.org/php/getParsedLog.php?id=45369171&tree=Mozilla-Inbound but I don't know yet if it was just a one-off failure or is related to this patch.
Flags: needinfo?(botond)
The test failures caught a legitimate, and tricky, bug!

The problem was that the mixed strongly-typed/primitive operations were improperly returning the primitive type as their result type. This is wrong when e.g. the strongly-typed argument is a float-valued Coord, and the primitive argument is an integer, because then the result gets truncated to an integer, so that e.g. ScreenCoord(50) / 100 would return 0 instead of 0.5.

The fix is to return the "common type" of the Coord's underlying type and the primitive argument type, i.e. the same type that C++ would return for the same operation performed on primitive types.

The only modification is to the return types in CoordOperationsHelper, and the addition of CommonType to help compute them. Asking for re-review for these changes.

I'd Try, but Try is closed.
Attachment #8468548 - Attachment is obsolete: true
Attachment #8469600 - Flags: review?(bugmail.mozilla)
Flags: needinfo?(botond)
Attachment #8469600 - Flags: review?(bugmail.mozilla) → review?(bas)
(In reply to Botond Ballo [:botond] from comment #30)
> I'd Try, but Try is closed.

https://tbpl.mozilla.org/?tree=Try&rev=6017ad4b667e
Comment on attachment 8469600 [details] [diff] [review]
Part 1 - Moz2D changes

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

Stealing review back since Bas is on PTO or something. The changes look ok to me.
Attachment #8469600 - Flags: review?(bas) → review+
Doing another build-only Try push since some time has elapsed since the last one:

https://tbpl.mozilla.org/?tree=Try&rev=67e9c0e7ee35
https://hg.mozilla.org/mozilla-central/rev/4df5daa7ca34
https://hg.mozilla.org/mozilla-central/rev/6a9f66a511d2
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Blocks: 1057528
Depends on: 1057642
Depends on: 1060421
You need to log in before you can comment on or make changes to this bug.