Add BoxF::ExpandTo

Add BoxF::ExpandTo

This will be useful when you need to compute a bounding box for a collection of
3D points.

R=ajuma@chromium.org
BUG=None

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=226359

[ajuma, 2013-10-01 13:21:02]

https://codereview.chromium.org/25518002/diff/1/ui/gfx/box_f.cc
File ui/gfx/box_f.cc (right):

https://codereview.chromium.org/25518002/diff/1/ui/gfx/box_f.cc#newcode68
ui/gfx/box_f.cc:68: depth_ = max.z() - origin_.z();
How about putting the logic that ExpandTo and Union have in common into a
separate helper function? I think we could share everything from Union except
the emptiness check.

[Ian Vollick, 2013-10-01 13:37:29]

On 2013/10/01 13:21:02, ajuma wrote:
> https://codereview.chromium.org/25518002/diff/1/ui/gfx/box_f.cc
> File ui/gfx/box_f.cc (right):
> 
> https://codereview.chromium.org/25518002/diff/1/ui/gfx/box_f.cc#newcode68
> ui/gfx/box_f.cc:68: depth_ = max.z() - origin_.z();
> How about putting the logic that ExpandTo and Union have in common into a
> separate helper function? I think we could share everything from Union except
> the emptiness check.

Whoops, I totally reinvented the wheel there. Done.

[ajuma, 2013-10-01 13:51:13]

On 2013/10/01 13:37:29, Ian Vollick wrote:
> On 2013/10/01 13:21:02, ajuma wrote:
> > https://codereview.chromium.org/25518002/diff/1/ui/gfx/box_f.cc
> > File ui/gfx/box_f.cc (right):
> > 
> > https://codereview.chromium.org/25518002/diff/1/ui/gfx/box_f.cc#newcode68
> > ui/gfx/box_f.cc:68: depth_ = max.z() - origin_.z();
> > How about putting the logic that ExpandTo and Union have in common into a
> > separate helper function? I think we could share everything from Union
except
> > the emptiness check.
> 
> Whoops, I totally reinvented the wheel there. Done.

Thanks, lgtm.

[Ian Vollick, 2013-10-01 13:53:04]

On 2013/10/01 13:51:13, ajuma wrote:
> On 2013/10/01 13:37:29, Ian Vollick wrote:
> > On 2013/10/01 13:21:02, ajuma wrote:
> > > https://codereview.chromium.org/25518002/diff/1/ui/gfx/box_f.cc
> > > File ui/gfx/box_f.cc (right):
> > > 
> > > https://codereview.chromium.org/25518002/diff/1/ui/gfx/box_f.cc#newcode68
> > > ui/gfx/box_f.cc:68: depth_ = max.z() - origin_.z();
> > > How about putting the logic that ExpandTo and Union have in common into a
> > > separate helper function? I think we could share everything from Union
> except
> > > the emptiness check.
> > 
> > Whoops, I totally reinvented the wheel there. Done.
> 
> Thanks, lgtm.

+danakj for OWNERS

[danakj, 2013-10-01 14:23:43]

https://codereview.chromium.org/25518002/diff/13001/ui/gfx/box_f.cc
File ui/gfx/box_f.cc (right):

https://codereview.chromium.org/25518002/diff/13001/ui/gfx/box_f.cc#newcode28
ui/gfx/box_f.cc:28: float min_x = std::min(x(), min.x());
DCHECK() that min <= max please.

https://codereview.chromium.org/25518002/diff/13001/ui/gfx/box_f.cc#newcode49
ui/gfx/box_f.cc:49: ExpandTo(box.origin(), gfx::Point3F(box.right(),
box.bottom(), box.front()));
What happens if |box| is entirely left/above |this|? That violates the
requirement for |min| and |max| on ExpandTo(min,max) doesn't it?

For instances, box = 0,0,0x5x5x5 and this = 20,20,20x5x5x5. You'll do
ExpandTo([20,20,20], [5,5,5])?

This case should be covered by the Union unit test, does it pass somehow?

https://codereview.chromium.org/25518002/diff/13001/ui/gfx/box_f.h
File ui/gfx/box_f.h (right):

https://codereview.chromium.org/25518002/diff/13001/ui/gfx/box_f.h#newcode97
ui/gfx/box_f.h:97: // Expands the box to contain the given point, if necessary.
Can you also comment on what this function does if the current box is empty?

https://codereview.chromium.org/25518002/diff/13001/ui/gfx/box_f.h#newcode101
ui/gfx/box_f.h:101: // Expands the box to contain the two given points. It is
guaranteed that each
s/guaranteed/required/

https://codereview.chromium.org/25518002/diff/13001/ui/gfx/box_unittest.cc
File ui/gfx/box_unittest.cc (right):

https://codereview.chromium.org/25518002/diff/13001/ui/gfx/box_unittest.cc#ne...
ui/gfx/box_unittest.cc:99: EXPECT_EQ(box1.ToString(), expected1_1.ToString());
expected values come first

[Ian Vollick, 2013-10-01 14:54:41]

On 2013/10/01 14:23:43, danakj wrote:
> https://codereview.chromium.org/25518002/diff/13001/ui/gfx/box_f.cc
> File ui/gfx/box_f.cc (right):
> 
> https://codereview.chromium.org/25518002/diff/13001/ui/gfx/box_f.cc#newcode28
> ui/gfx/box_f.cc:28: float min_x = std::min(x(), min.x());
> DCHECK() that min <= max please.
> 
> https://codereview.chromium.org/25518002/diff/13001/ui/gfx/box_f.cc#newcode49
> ui/gfx/box_f.cc:49: ExpandTo(box.origin(), gfx::Point3F(box.right(),
> box.bottom(), box.front()));
> What happens if |box| is entirely left/above |this|? That violates the
> requirement for |min| and |max| on ExpandTo(min,max) doesn't it?

Nope, it doesn't. Here's another attempt at describing the requirement. If |min|
and |max| are two corners of |box|, |min| must be the corner with the smallest
components (i.e., box.origin()) and |max| is the corner with the larger
components (i.e., box.origin() + (box.width(), box.height(), box.depth())).

This is just a workaround to deal with the fact that the internal representation
of box is a little clumsy. If a box was represented in terms of these two
points, it would make make operations like union and expand-to much more
natural. Union already had to do a dance to put the box into this two point
representation temporarily and then transform it back into origin + dimensions.
The new ExpandTo(point, point) function just refactors that dance so it can be
reused by ExpandTo(point).

> 
> For instances, box = 0,0,0x5x5x5 and this = 20,20,20x5x5x5. You'll do
> ExpandTo([20,20,20], [5,5,5])?
It'll be ExpandTo([0,0,0], [5,5,5]) <-- the two point representation of |box|.
The two point representation of |this| is [20,20,20] [25,25,25]. The resulting
expanded box in two-point representation will be [0,0,0] [25,25,25] which will
be converted back to origin + dimensions as usual (which happens to look the
same).
> 
> This case should be covered by the Union unit test, does it pass somehow?
Yep, it passes.
> 
> https://codereview.chromium.org/25518002/diff/13001/ui/gfx/box_f.h
> File ui/gfx/box_f.h (right):
> 
> https://codereview.chromium.org/25518002/diff/13001/ui/gfx/box_f.h#newcode97
> ui/gfx/box_f.h:97: // Expands the box to contain the given point, if
necessary.
> Can you also comment on what this function does if the current box is empty?
Done.
> 
> https://codereview.chromium.org/25518002/diff/13001/ui/gfx/box_f.h#newcode101
> ui/gfx/box_f.h:101: // Expands the box to contain the two given points. It is
> guaranteed that each
> s/guaranteed/required/
Done.
> 
> https://codereview.chromium.org/25518002/diff/13001/ui/gfx/box_unittest.cc
> File ui/gfx/box_unittest.cc (right):
> 
>
https://codereview.chromium.org/25518002/diff/13001/ui/gfx/box_unittest.cc#ne...
> ui/gfx/box_unittest.cc:99: EXPECT_EQ(box1.ToString(), expected1_1.ToString());
> expected values come first
Done.

[danakj, 2013-10-01 15:09:15]

LGTM with one comment request on the public ExpandTo

https://codereview.chromium.org/25518002/diff/26001/ui/gfx/box_f.cc
File ui/gfx/box_f.cc (right):

https://codereview.chromium.org/25518002/diff/26001/ui/gfx/box_f.cc#newcode54
ui/gfx/box_f.cc:54: ExpandTo(box.origin(), gfx::Point3F(box.right(),
box.bottom(), box.front()));
Ah I fail at reading, I see you're giving box's data for both arguments here.
Thanks for the explanation.

https://codereview.chromium.org/25518002/diff/26001/ui/gfx/box_f.h
File ui/gfx/box_f.h (right):

https://codereview.chromium.org/25518002/diff/26001/ui/gfx/box_f.h#newcode97
ui/gfx/box_f.h:97: // Expands the box to contain the given point, if necessary.
Comment on what happens if |this| is empty? I see you added some stuff on the
private ExpandTo() but I'm more concerned with documenting the public API.

[Ian Vollick, 2013-10-01 18:26:20]

On 2013/10/01 15:09:15, danakj wrote:
> LGTM with one comment request on the public ExpandTo
> 
> https://codereview.chromium.org/25518002/diff/26001/ui/gfx/box_f.cc
> File ui/gfx/box_f.cc (right):
> 
> https://codereview.chromium.org/25518002/diff/26001/ui/gfx/box_f.cc#newcode54
> ui/gfx/box_f.cc:54: ExpandTo(box.origin(), gfx::Point3F(box.right(),
> box.bottom(), box.front()));
> Ah I fail at reading, I see you're giving box's data for both arguments here.
> Thanks for the explanation.
> 
> https://codereview.chromium.org/25518002/diff/26001/ui/gfx/box_f.h
> File ui/gfx/box_f.h (right):
> 
> https://codereview.chromium.org/25518002/diff/26001/ui/gfx/box_f.h#newcode97
> ui/gfx/box_f.h:97: // Expands the box to contain the given point, if
necessary.
> Comment on what happens if |this| is empty? I see you added some stuff on the
> private ExpandTo() but I'm more concerned with documenting the public API.

SG. Done.

[commit-bot: I haz the power, 2013-10-01 18:26:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vollick@chromium.org/25518002/29001

[commit-bot: I haz the power, 2013-10-01 21:31:27]

Retried try job too often on win_rel for step(s) telemetry_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-10-01 22:06:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vollick@chromium.org/25518002/29001

[commit-bot: I haz the power, 2013-10-02 00:09:18]

Change committed as 226359