Open Bug 1386277 Opened 7 years ago Updated 2 years ago

Change BaseRect to internally store extents instead of width/height

Categories

(Core :: Graphics, enhancement, P3)

enhancement

Tracking

()

Tracking Status
firefox57 --- unaffected

People

(Reporter: milan, Unassigned)

References

(Depends on 4 open bugs)

Details

(Whiteboard: [gfx-noted])

Attachments

(1 file, 3 obsolete files)

"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 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
(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 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
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
Attached patch WIP: gfx/ includes (obsolete) — Splinter Review
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
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 :)
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.
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
The leave-open keyword is there and there is no activity for 6 months.
:davidb, maybe it's time to close this bug?
Flags: needinfo?(dbolter)
Matt what should we do with this bug?
Flags: needinfo?(dbolter) → needinfo?(matt.woodrow)
We have RectAbsolute now, which is implemented using the extents, so the remaining work is to switch code over to using that implementation instead of the old Rect.

I think this is still valuable, we just need to find time to get it done.

Botond, is this something you're still interested in?
Flags: needinfo?(matt.woodrow) → needinfo?(botond)
(In reply to Matt Woodrow (:mattwoodrow) from comment #25)
> Botond, is this something you're still interested in?

Based on previous conversations with Jeff, it's something the Graphics team is still interested in getting fixed.

It's not the highest priority though, and given other priorities I don't see myself getting to it any time soon.

As a patch has landed here, normally I'd suggest, for bookkeeping purposes, closing this bug and filing a follow-up for remaining work. However, there's so much context here that I think that would be counterproductive, so I'm just going to remove the leave-open keyword.
Flags: needinfo?(botond)
Keywords: leave-open
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: