Change BaseRect to internally store extents instead of width/height

NEW
Unassigned

Status

()

Core
Graphics
P3
normal
a year ago
2 months ago

People

(Reporter: milan, Unassigned)

Tracking

(Depends on: 4 bugs, {leave-open})

unspecified
leave-open
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 unaffected)

Details

(Whiteboard: [gfx-noted])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 3 obsolete attachments)

"I" in this e-mail refers to :botond

We currently represent a rectangle by storing the coordinates of its
top-left corner, its width, and its height. I'll refer to this
representation as "x/y/w/h".

I would like to propose storing instead the coordinates of the
top-left corner, and the coordinates of the bottom-right corner. I'll
refer to this representation as "x1/y1/x2/y2".

The x1/y1/x2/y2 representation has several advantages over x/y/w/h:

   - Several operations are more efficient with x1/y1/x2/y2, including
intersection,
     union, and point-in-rect.
   - The representation is more symmetric, since it stores two
quantities
of the
     same kind (two points) rather than a point and a dimension
(width/height).
   - The representation is less susceptible to overflow. With x/y/w/h,
computation
     of x2/y2 can overflow for a large range of values of x/y and w/h.
However,
     with x1/y1/x2/y2, computation of w/h cannot overflow if the
coordinates are
     signed and the resulting w/h is unsigned.

A known disadvantage of x1/y1/x2/y2 is that translating the rectangle
requires translating both points, whereas translating x/y/w/h only
requires translating one point. I think this disadvantage is minor in
comparison to the above advantages.

The proposed change would affect the class mozilla::gfx::BaseRect, and
classes that derive from it (such as CSSRect, LayoutRect, etc., and,
notably, nsRect and nsIntRect), but NOT other rectangle classes like
DOMRect.
Keywords: leave-open
Whiteboard: [gfx-noted]
Comment hidden (mozreview-request)

Comment 2

a year ago
mozreview-review
Comment on attachment 8892462 [details]
Bug 1386277: Add set methods for width and height that change nothing else, as well as the Swap method

https://reviewboard.mozilla.org/r/163436/#review168800

::: gfx/2d/BaseRect.h:372
(Diff revision 1)
> -  T YMost() const { return y + height; }
> +  MOZ_ALWAYS_INLINE T YMost() const { return y + height; }
> +
> +  // Set width and height
> +  MOZ_ALWAYS_INLINE void SetWidth(T aWidth) { width = aWidth; }
> +  MOZ_ALWAYS_INLINE void SetHeight(T aHeight) { height = aHeight; }
> +  MOZ_ALWAYS_INLINE void SetWidthHeight(T aWidth, T aHeight) { width = aWidth; height = aHeight; }

This one already exists, under the name SizeTo
Comment hidden (mozreview-request)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #2)
> ...
> > +  MOZ_ALWAYS_INLINE void SetWidthHeight(T aWidth, T aHeight) { width = aWidth; height = aHeight; }
> 
> This one already exists, under the name SizeTo

Thanks!  Removed it from the updated patch.

Comment 5

a year ago
mozreview-review
Comment on attachment 8892462 [details]
Bug 1386277: Add set methods for width and height that change nothing else, as well as the Swap method

https://reviewboard.mozilla.org/r/163436/#review169176
Attachment #8892462 - Flags: review?(bas) → review+
Priority: -- → P3
Comment hidden (mozreview-request)

Comment 7

a year ago
Pushed by msreckovic@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f56d71bbd7eb
Add set methods for width and height that change nothing else, as well as the Swap method r=bas
Comment hidden (mozreview-request)
Created attachment 8893537 [details] [diff] [review]
WIP: gfx/ includes
Assignee: nobody → milan
Attachment #8893538 - Attachment description: more includes → WIP: more includes
Attachment #8893537 - Attachment is obsolete: true
Attachment #8893538 - Attachment is obsolete: true
Attachment #8893539 - Attachment is obsolete: true
No longer blocks: 1387514
Depends on: 1387514

Comment 15

a year ago
Pushed by msreckovic@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/65f903ed1b1b
Add set methods for width and height that change nothing else, as well as the Swap method r=bas
As noted in bug 1387514 comment 17, we'll want to be on the lookout for code like this:
>  for (int32_t x = 0; x < aFillRect.Width(); x++) {

In this bug, we will be changing Width() from an trivial accessor into a function that performs some (minimal) arithmetic.  We'll likely want to adjust loops like this so that they only call |Width()| once, to avoid unnecessarily performing that arithmetic on every loop iteration.
Yes, I expect a few more steps before we get to the actual switch.
.x/.y should be audited, perhaps even replaced.  We may want to get rid of the direct access and replace with "change just origin" vs. "translate rectangle" - currently changing .x/.y does the translation.

Then we'll want to audit the calls to Width()/Height() and fix the places where the optimization like above should be done.  And a few others.  This is a bit of a long haul bug :)
Depends on: 1390110
Depends on: 1390959
I'm not sure we will want width/height to be unsigned.  More on the topic later, but wanted to record the thought.
I'm going to do another pass (in spin-off bugs) and remove direct access to x and y as well.  Right now, it was only done for width/height.  This will be less mechanical, as we want to make sure that the intent is captured - modifying x and y is implicitly doing "MoveTo", so it's important to understand if that is the intended behaviour, and modify the places that are setting values to use another method.  Or MoveTo/MoveBy.
status-firefox57: --- → unaffected
No longer depends on: 1423541
No longer depends on: 1423548
No longer depends on: 1423551
No longer depends on: 1423552
No longer depends on: 1423556
No longer depends on: 1423558
No longer depends on: 1423559
No longer depends on: 1423567
No longer depends on: 1423570
Depends on: 1423570
No longer depends on: 1223570
(In reply to Milan Sreckovic [:milan] from comment #18)
> Then we'll want to audit the calls to Width()/Height() and fix the places
> where the optimization like above should be done.  And a few others.

(One specific case for optimization, after we're storing extents: calls to foo.SetWidth(foo.Width() + $OTHERSTUFF);  These should be reasonably easy to find, and they're worth finding because they involve two avoidable arithmetic operations (in the getter as well as the setter).  All such calls could be represented more efficiently as foo.SetRightEdge(foo.XMost() + $OTHERSTUFF), which drops the unnecessary arithmetic.)
It may be worth adding "Change" methods for all of those, although that explodes the number of methods in BaseRect and may have impact on code size.  Both for width/height, but also for all four edges.  We already have them for X and Y (MoveTo/MoveBy)
Assignee: milaninbugzilla → nobody
You need to log in before you can comment on or make changes to this bug.