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)
Core
Graphics
Tracking
()
NEW
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.
Reporter | ||
Updated•7 years ago
|
Keywords: leave-open
Whiteboard: [gfx-noted]
Comment hidden (mozreview-request) |
Comment 2•7 years 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) |
Reporter | ||
Comment 4•7 years ago
|
||
(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•7 years 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+
Reporter | ||
Updated•7 years ago
|
Priority: -- → P3
Comment hidden (mozreview-request) |
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
Backed out for build failures like https://treeherder.mozilla.org/logviewer.html#?job_id=120785772&repo=autoland
https://hg.mozilla.org/integration/autoland/rev/9c4e1d455e0dae135e857f2fd7b0572e5e65620b
Flags: needinfo?(milan)
Reporter | ||
Comment 9•7 years ago
|
||
Missing std:: was a problem for linux. Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c62e049a3c507ea00c983b70b59bc2251b49641c
Flags: needinfo?(milan)
Comment hidden (mozreview-request) |
Reporter | ||
Comment 11•7 years ago
|
||
Assignee: nobody → milan
Reporter | ||
Comment 12•7 years ago
|
||
Reporter | ||
Updated•7 years ago
|
Attachment #8893538 -
Attachment description: more includes → WIP: more includes
Reporter | ||
Comment 13•7 years ago
|
||
Reporter | ||
Comment 14•7 years ago
|
||
Reporter | ||
Updated•7 years ago
|
Attachment #8893537 -
Attachment is obsolete: true
Reporter | ||
Updated•7 years ago
|
Attachment #8893538 -
Attachment is obsolete: true
Reporter | ||
Updated•7 years ago
|
Attachment #8893539 -
Attachment is obsolete: true
Reporter | ||
Updated•7 years ago
|
Comment 15•7 years 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
Comment 16•7 years ago
|
||
bugherder |
Comment 17•7 years ago
|
||
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.
Reporter | ||
Comment 18•7 years ago
|
||
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 :)
Reporter | ||
Comment 19•7 years ago
|
||
I'm not sure we will want width/height to be unsigned. More on the topic later, but wanted to record the thought.
Reporter | ||
Comment 20•7 years ago
|
||
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.
Reporter | ||
Updated•7 years ago
|
status-firefox57:
--- → unaffected
Reporter | ||
Updated•7 years ago
|
Reporter | ||
Updated•7 years ago
|
Updated•7 years ago
|
Comment 21•7 years ago
|
||
(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.)
Reporter | ||
Comment 22•7 years ago
|
||
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)
Reporter | ||
Updated•6 years ago
|
Assignee: milaninbugzilla → nobody
Comment 23•6 years ago
|
||
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)
Comment 24•6 years ago
|
||
Matt what should we do with this bug?
Flags: needinfo?(dbolter) → needinfo?(matt.woodrow)
Comment 25•6 years ago
|
||
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)
Comment 26•6 years ago
|
||
(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
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•