Closed Bug 880676 Opened 7 years ago Closed 7 years ago

Add a struct/class to strongly type conversions between coordinate systems

Categories

(Core :: Graphics, defect)

23 Branch
x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: kats, Assigned: kats)

References

Details

Attachments

(10 files, 9 obsolete files)

18.06 KB, patch
ajones
: review+
Details | Diff | Splinter Review
12.11 KB, patch
BenWa
: review+
Details | Diff | Splinter Review
8.16 KB, patch
ajones
: review+
Details | Diff | Splinter Review
8.12 KB, patch
BenWa
: review+
Details | Diff | Splinter Review
12.10 KB, patch
kats
: review+
Details | Diff | Splinter Review
7.94 KB, patch
kats
: review+
Details | Diff | Splinter Review
15.69 KB, patch
BenWa
: review+
ajones
: review+
Details | Diff | Splinter Review
15.57 KB, patch
BenWa
: review+
Details | Diff | Splinter Review
6.39 KB, patch
kats
: review+
Details | Diff | Splinter Review
26.78 KB, patch
ajones
: review+
Details | Diff | Splinter Review
I've been slowly converting code to use the new templated gfx types introduced in bug 865735. While doing so it has become apparent that we could benefit from typing the conversion scale factors too. As Anthony pointed out at https://bugzilla.mozilla.org/show_bug.cgi?id=879004#c20, the resolution parameters passed to the conversion functions in LayerPixel et al would benefit from being incompatible with arbitrary floats or doubles. I'm not entirely sure of the best way to make this happen but I'm filing this bug to track this change and to think about it.
I think I made a mistake when I added type info to mCompositionBounds. It's actually in ScreenPixel units, not LayerPixel units.
This is my WIP for adding stronger type conversion between pixel units. I think using this ends up being pretty clunky, ideally we should be able to overload operators on some of these structs so that we can use * and / as one would expect to get back the right things.

Also note that this patch doesn't currently compile; still working on it but there's enough there to get an idea for feedback.
Attachment #761104 - Flags: feedback?(bgirard)
Attachment #761104 - Flags: feedback?(ajones)
Comment on attachment 761104 [details] [diff] [review]
Part 2 (WIP) - Add a ScaleFactor class to convert between pixel units

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

::: gfx/2d/ScaleFactor.h
@@ +48,5 @@
> +  }
> +
> +  bool operator==(const ScaleFactor& aScale) const {
> +    return x == aScale.x && y == aScale.y;
> +  }

Seems odd to have equality but no copy constructor.

@@ +51,5 @@
> +    return x == aScale.x && y == aScale.y;
> +  }
> +
> +  bool operator!=(const ScaleFactor& aScale) const {
> +    return x != aScale.x || y != aScale.y;

return !(*this == aScale);

::: gfx/layers/client/ClientLayerManager.cpp
@@ +395,5 @@
>      // This is derived from the code in
>      // gfx/layers/ipc/CompositorParent.cpp::TransformShadowTree.
>      const gfx3DMatrix& rootTransform = GetRoot()->GetTransform();
> +    LayerToCSSScale paintScale = LayerToCSSScale(rootTransform.GetXScale(),
> +                                                 rootTransform.GetYScale());

LayerToCSSScale paintScale(rootTransform.GetXScale(), rootTransform.GetYScale());

::: layout/base/Units.h
@@ +44,5 @@
> +typedef gfx::ScaleFactor<LayerPixel, CSSPixel> LayerToCSSScale;
> +typedef gfx::ScaleFactor<CSSPixel, ScreenPixel> CSSToScreenScale;
> +typedef gfx::ScaleFactor<ScreenPixel, CSSPixel> ScreenToCSSScale;
> +typedef gfx::ScaleFactor<LayerPixel, ScreenPixel> LayerToScreenScale;
> +typedef gfx::ScaleFactor<ScreenPixel, LayerPixel> ScreenToLayerScale;

I'm not completely sold on these typedefs. For example if we just did s/Typed/T/ and s/Point/P/ then we could do away with them. ScaleFactor could just be called Scale.
Attachment #761104 - Flags: feedback?(ajones) → feedback+
Comment on attachment 761104 [details] [diff] [review]
Part 2 (WIP) - Add a ScaleFactor class to convert between pixel units

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

::: gfx/layers/client/ClientLayerManager.cpp
@@ +394,5 @@
>  
>      // This is derived from the code in
>      // gfx/layers/ipc/CompositorParent.cpp::TransformShadowTree.
>      const gfx3DMatrix& rootTransform = GetRoot()->GetTransform();
> +    LayerToCSSScale paintScale = LayerToCSSScale(rootTransform.GetXScale(),

Is this really typesafe? Here paintScale is LayerToCSSScale and FromCSSRect takes a CSSToLayerScale.
Attachment #761104 - Attachment is obsolete: true
Attachment #761104 - Flags: feedback?(bgirard)
Attachment #761686 - Flags: review?(bgirard)
This is just to make the next few patches easier to read. And to keep Units.h a bit cleaner.
Attachment #761694 - Flags: review?(bgirard)
Commit message needs updating on this, I forgot to do that.
Attachment #761695 - Flags: review?(bgirard)
Attached patch Part 7 - Introduce ScaleFactor (obsolete) — Splinter Review
Attachment #761697 - Flags: review?(bgirard)
Attachment #761697 - Flags: review?(ajones)
Attachment #761697 - Flags: feedback?(roc)
(In reply to Benoit Girard (:BenWa) from comment #4)
> 
> Is this really typesafe? Here paintScale is LayerToCSSScale and FromCSSRect
> takes a CSSToLayerScale.

I overloaded the () operator so that the compiler could call Invert() on it make it fit the requirements. I had hoped it would make the code simpler but I think it actually adds confusion. The new patches don't use this sneaky stuff (although I think I left in the overloaded operator in part 7, I can take that out).
Missed a couple of files in this patch.
Attachment #761703 - Attachment is obsolete: true
Attachment #761703 - Flags: review?(ajones)
Attachment #761723 - Flags: review?(ajones)
Try build at https://tbpl.mozilla.org/?tree=Try&rev=0e5914369c31 - hopefully I didn't screw up the merge with Ms2ger's changes to Units.h.
Attachment #761687 - Flags: review?(ajones) → review+
Attachment #761702 - Flags: review?(ajones) → review+
Comment on attachment 761723 [details] [diff] [review]
Part 10 - Remove ToCSSIntRectRoundIn and convert APZC::CalculateResolution

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

::: gfx/2d/Rect.h
@@ +109,5 @@
>                               NS_lround(aRect.height));
>  }
>  
> +template<class units>
> +IntRectTyped<units> RoundIn(const RectTyped<units>& aRect)

If we name the function RoundedIn then it gives a hint that it's not the dreaded state mutation variant.

::: gfx/2d/ScaleFactor.h
@@ +25,5 @@
>    ScaleFactor<dst, src> Invert() {
>      return ScaleFactor<dst, src>(1 / x, 1 / y);
>    }
>  
> +  ScaleFactor<src, dst> XScale() const {

Suggestion: XScale isn't an entirely obvious name but I can't think of a better one. It may be better not to have such a function and to explicitly use ScaleFactor(other.x, other.y). Having said that I don't know if this operation is actually necessary.

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +749,5 @@
>      MonitorAutoLock monitor(mMonitor);
>  
>      // We want to inversely scale it because when you're zoomed further in, a
>      // larger swipe should move you a shorter distance.
> +    ScreenToCSSScale inverseResolution = CalculateResolution(mFrameMetrics).Invert();

This function would be better named Inverse() because it is returning the inverse.
Attachment #761723 - Flags: review?(ajones) → review+
Comment on attachment 761686 [details] [diff] [review]
Part 2 - Convert another gfx::Point to a ScreenPoint

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

This being a ScreenPoint isn't going to work with sub-FrameMetrics and sub-APZC. It's really not meant to be a LayerPoint? I guess we will need to update this code.
Attachment #761686 - Flags: review?(bgirard) → review+
Attachment #761691 - Flags: review?(bgirard) → review+
Comment on attachment 761694 [details] [diff] [review]
Part 5 - Units.h cleanup (cosmetic)

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

::: layout/base/Units.h
@@ +12,5 @@
>  
>  namespace mozilla {
>  
> +// Forward declarations so we can declare the typedefs
> +// and use them in the actual implementations for readbility.

Opinion: I'm not sure this comment adds value.
Attachment #761694 - Flags: review?(bgirard) → review+
Attachment #761695 - Flags: review?(bgirard) → review+
Comment on attachment 761697 [details] [diff] [review]
Part 7 - Introduce ScaleFactor

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

::: gfx/2d/ScaleFactor.h
@@ +21,5 @@
> +  explicit ScaleFactor(float aScale) : x(aScale), y(aScale) {}
> +  explicit ScaleFactor(float aX, float aY) : x(aX), y(aY) {}
> +  explicit ScaleFactor(gfxSize aScale) : x(aScale.width), y(aScale.height) {}
> +
> +  ScaleFactor<dst, src> Invert() {

Prefer Inverse().

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +1191,2 @@
>    *aNewTransform = ViewTransform(-translation, localScale);
> +  aScrollOffset = (scrollOffset * localScale);

aScrollOffset = scrollOffset * localScale;
Attachment #761697 - Flags: review?(ajones) → review+
Attachment #761685 - Flags: review?(ajones) → review+
Comment on attachment 761697 [details] [diff] [review]
Part 7 - Introduce ScaleFactor

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

In a lot of places --- probably most places, overall --- x and y scales are always the same and we only want to store one of them. So I think I would rather have ScaleFactor be a single value and have ScaleFactorXY (or some better name) for the two-axis version.

::: gfx/2d/ScaleFactor.h
@@ +11,5 @@
> +namespace mozilla {
> +namespace gfx {
> +
> +template<class src, class dst>
> +struct ScaleFactor {

This needs documentation.

@@ +26,5 @@
> +    return ScaleFactor<dst, src>(1 / x, 1 / y);
> +  }
> +
> +  operator ScaleFactor<dst, src>() const {
> +    return Invert();

This is freaky. Why have it?
Comment on attachment 761687 [details] [diff] [review]
Part 3 - Convert some things in APZC::SampleContentTransformForFrame

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

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +1154,5 @@
>      localScale = CalculateResolution(mFrameMetrics);
>  
>      if (frame.IsScrollable()) {
> +      metricsScrollOffset = LayerPoint::FromUnknownPoint(
> +        frame.GetScrollOffsetInLayerPixels());

Why not make it return a LayerPoint now you're changing the return value anyway?
(In reply to Anthony Jones (:kentuckyfriedtakahe) from comment #18)
> > +template<class units>
> > +IntRectTyped<units> RoundIn(const RectTyped<units>& aRect)
> 
> If we name the function RoundedIn then it gives a hint that it's not the
> dreaded state mutation variant.

Good call. I'll do that.

> > +  ScaleFactor<src, dst> XScale() const {
> 
> Suggestion: XScale isn't an entirely obvious name but I can't think of a
> better one. It may be better not to have such a function and to explicitly
> use ScaleFactor(other.x, other.y). Having said that I don't know if this
> operation is actually necessary.

Yeah I'm not happy with XScale either. I'll try doing what roc suggested and having just one scale factor, which will get rid of XScale entirely. Right now the code is a mess with respect to using x and y or just x scale factors (and I don't know if any of the x-and-y cases are actually important) so it might break some stuff.

> ::: gfx/layers/ipc/AsyncPanZoomController.cpp
> > +    ScreenToCSSScale inverseResolution = CalculateResolution(mFrameMetrics).Invert();
> 
> This function would be better named Inverse() because it is returning the
> inverse.

Will do.

(In reply to Benoit Girard (:BenWa) from comment #19)
> This being a ScreenPoint isn't going to work with sub-FrameMetrics and
> sub-APZC. It's really not meant to be a LayerPoint? I guess we will need to
> update this code.

Do you know what specifically conflicts? I'll double-check to see if it's actually a LayerPoint and I screwed up somewhere. It might also be in some other coordinate space entirely. I'm not sure if there's another coordinate space that needs to be introduced because of the cross-process architecture on B2G.

(In reply to Benoit Girard (:BenWa) from comment #20)
> > +// Forward declarations so we can declare the typedefs
> > +// and use them in the actual implementations for readbility.
> 
> Opinion: I'm not sure this comment adds value.

I'll remove it.

(In reply to Anthony Jones (:kentuckyfriedtakahe) from comment #21)
> > +  ScaleFactor<dst, src> Invert() {
> 
> Prefer Inverse().

Yup, will do.

> ::: gfx/layers/ipc/AsyncPanZoomController.cpp
> > +  aScrollOffset = (scrollOffset * localScale);
> 
> aScrollOffset = scrollOffset * localScale;

Will do.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #22)
> 
> In a lot of places --- probably most places, overall --- x and y scales are
> always the same and we only want to store one of them. So I think I would
> rather have ScaleFactor be a single value and have ScaleFactorXY (or some
> better name) for the two-axis version.

Ok, I'll give that a shot.

> ::: gfx/2d/ScaleFactor.h
> > +template<class src, class dst>
> > +struct ScaleFactor {
> 
> This needs documentation.

I'll add some.

> @@ +26,5 @@
> > +  operator ScaleFactor<dst, src>() const {
> > +    return Invert();
> 
> This is freaky. Why have it?

Left over from my original WIP actually. I'll remove it.

(In reply to :Ms2ger from comment #23)
> > +      metricsScrollOffset = LayerPoint::FromUnknownPoint(
> > +        frame.GetScrollOffsetInLayerPixels());
> 
> Why not make it return a LayerPoint now you're changing the return value
> anyway?

I assume you're referring to GetScrollOffsetInLayerPixels return value? I'm not actually sure that it is a LayerPoint. The calculations it does internally looks wrong to me, and I need audit those pieces more carefully before I'm convinced that the "LayerPixels" in the name is the same as the "LayerPixels" we're using elsewhere.
Rebased, carrying r+
Attachment #761694 - Attachment is obsolete: true
Attachment #762203 - Flags: review+
Updated part 6 so that the function is called RoundedToInt rather than RoundToInt. This makes it consistent with the change Anthony requested. Carrying r+
Attachment #761695 - Attachment is obsolete: true
Attachment #762204 - Flags: review+
Updated so that ScaleFactor has only one float instead of two. After discussing with a few people no use cases came up where the scale factor would actually differ in the two axes, so I decided to remove that aspect entirely. It can be put back if it's needed later. Also on a suggestion from cpeterson I added MOZ_ASSERTs to ScaleFactor so that if it is constructed with differing scales on the two axes it will tell us.
Attachment #761697 - Attachment is obsolete: true
Attachment #761697 - Flags: review?(bgirard)
Attachment #761697 - Flags: feedback?(roc)
Attachment #762208 - Flags: review?(bgirard)
Attachment #762208 - Flags: review?(ajones)
Rebased, carrying r?BenWa
Attachment #761699 - Attachment is obsolete: true
Attachment #761699 - Flags: review?(bgirard)
Attachment #762209 - Flags: review?(bgirard)
This one is rebased, but because of the accumulated changes from the previous patches it looks quite different. Re-requesting review just to be safe.
Attachment #761723 - Attachment is obsolete: true
Attachment #762214 - Flags: review?(ajones)
Comment on attachment 762208 [details] [diff] [review]
Part 7 - Introduce ScaleFactor (v2)

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

::: gfx/2d/ScaleFactor.h
@@ +19,5 @@
> + * Note that some parts of the code that pre-date this class used separate
> + * scaling factors for the x and y axes. However, at runtime these values
> + * were always expected to be the same, so this class uses only one scale
> + * factor for both axes. The two constructors that take two-axis scaling
> + * factors check to ensure that this assertion holds.

Good idea taking both param and asserting.

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +549,5 @@
>  
>    {
>      MonitorAutoLock monitor(mMonitor);
>  
> +    CSSToScreenScale resolution(CalculateResolution(mFrameMetrics).width);

This would be an excellent place to assert that width/height scale factor is the same by calling the right constructor.

@@ +554,4 @@
>      gfxFloat userZoom = mFrameMetrics.mZoom.width;
>      ScreenPoint focusPoint = aEvent.mFocusPoint;
>  
> +    CSSPoint focusChange = (mLastZoomFocus - focusPoint) / resolution;

Nice use of operator overload
Attachment #762208 - Flags: review?(bgirard) → review+
Attachment #762208 - Flags: review?(ajones) → review+
Attachment #762214 - Flags: review?(ajones) → review+
Attachment #762209 - Flags: review?(bgirard) → review+
Blocks: 907901
You need to log in before you can comment on or make changes to this bug.