Rect::CenterPoint() returning a truncated Point for odd-size Rects. |
|||||||
Issue descriptionThe center point of a Rect can only be represented with a Point when sizes are even. This is a fairly large foot shooter observed with InkDrop related classes using gfx::PointF(Rect::CenterPoint()) with odd View::size()s. This causes subpixel misalignment (see screenshot). We should consider having Rect return a PointF instead of requiring upcasting a Rect to RectF to get the correct center point. If any usages require downcasting we should introduce a Point PointF::ToTruncatedPoint() or gfx::Point(PointF) constructor, to make sure any truncation is intentional. sky@, pkasting@: Any hunches here / reasons not to have Rect::CenterPoint() return a PointF? Examples below: Incorrect (foot-shooter), Rect::CenterPoint() truncates before upcast: gfx::PointF(GetMirroredRect(GetLocalBounds()).CenterPoint()) Correct: gfx::RectF(GetMirroredRect(GetLocalBounds())).CenterPoint() Return PointF: GetMirroredRect(GetLocalBounds()).CenterPoint() If we need truncation support here we add a gfx::Point constructor: gfx::Point(rect.CenterPoint()) or a truncation method which is even more explicit: rect.CenterPoint().ToTruncatedPoint()
,
Oct 10
The InkDrop doesn't use integer values, it takes a PointF as a center point and a SizeF to construct it. If the SizeF is odd-sized and PointF is truncated, then the bounds of the SizeF (when centered around the PointF). You could also argue that this code should take a Rect or RectF as actual bounds. Though any code that relies on .CenterPoint() being the center is going to be incorrect half of the time. We could also make it clearer by renaming Rect::CenterPoint -> Rect::TruncatedCenterPoint. Do you think the truncation problem is uninteresting and we should instead avoid placing anything around a center point?
,
Oct 10
sorry, "then the bounds of the generated bounds from the SizeF + PointF are subpixel misaligned" should've been part of the first paragraph.
,
Oct 10
My feeling is that if code is using integers, then it can't readily use floating point values, otherwise it would already be using floating values. From this perspective, converting CenterPoint to return PointF would just make the code more cumbersome to use. For example, in views layout code it's all integers, so anything returning floating point would need to be converted. Renaming the function to clearly indicate what it does, SGTM.
,
Oct 10
I don't think "if code is using integers, then it can't readily use floating point values, otherwise it would already be using floating values" is true. We use ints by default in many places where we wouldn't necessarily mind using floats. I started looking through some of the results of codesearching for CenterPoint() calls. It looks like there's a mixture of "clearly wants an int", "clearly wants a float", and "not clear but it would probably be better-served with a float", with the first category being the majority of the time but the second and third nontrivial. PointF(x.CenterPoint()) is a pretty clear sign that the code is broken, for example. My suggestion is that CenterPoint() return a PointF and TruncatedCenterPoint() return a Point (so, basically do both).
,
Oct 10
I think that suggestion is reasonable, supporting both is not a problem it's more the foot-shooter of implicit truncation.
,
Oct 11
,
Oct 11
,
Oct 11
,
Oct 11
,
Nov 16
**UI mass Triage** Adding more appropriate label.
,
Dec 11
|
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by sky@chromium.org
, Oct 10