Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1580363002:
Implement FloatRect::squaredDistanceTo function (Closed)
|
Created:
4 years, 11 months ago by hyunjunekim2 Modified:
4 years, 11 months ago CC:
blink-reviews, blink-reviews-platform-graphics_chromium.org, Rik, chromium-reviews, danakj+watch_chromium.org, dshwang, drott+blinkwatch_chromium.org, krit, f(malita), jbroman, Justin Novosad, kinuko+watch, pdr+graphicswatchlist_chromium.org, rwlbuis, Stephen Chennney, vmpstr+blinkwatch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement FloatRect::squaredDistanceTo function
This function is that calculate distance squared between
Rectangle and Point.
BUG=445329
Committed: https://crrev.com/ac7237a24b28daa6f1c5292f32e940cc63fba39d
Cr-Commit-Position: refs/heads/master@{#369334}
Patch Set 1 #Patch Set 2 : #
Total comments: 5
Patch Set 3 : #Patch Set 4 : #
Total comments: 2
Patch Set 5 : #Patch Set 6 : #
Messages
Total messages: 37 (20 generated)
Description was changed from ========== WIP BUG= ========== to ========== WIP [1] http://wiki.unity3d.com/index.php/Distance_from_a_point_to_a_rectangle ==========
Description was changed from ========== WIP [1] http://wiki.unity3d.com/index.php/Distance_from_a_point_to_a_rectangle ========== to ========== This function is that calculate distance between Rectangle and Point. [1] http://wiki.unity3d.com/index.php /Distance_from_a_point_to_a_rectangle ==========
Description was changed from ========== This function is that calculate distance between Rectangle and Point. [1] http://wiki.unity3d.com/index.php /Distance_from_a_point_to_a_rectangle ========== to ========== This function is distance calculation Algorithms[1] between Rectangle and Point. [1] http://wiki.unity3d.com/index.php /Distance_from_a_point_to_a_rectangle ==========
Description was changed from ========== This function is distance calculation Algorithms[1] between Rectangle and Point. [1] http://wiki.unity3d.com/index.php /Distance_from_a_point_to_a_rectangle ========== to ========== This function is distance calculation Algorithms[1] between Rectangle and Point. [1] http://wiki.unity3d.com/index.php/Distance_from_a_point_to_a_rectangle ==========
Description was changed from ========== This function is distance calculation Algorithms[1] between Rectangle and Point. [1] http://wiki.unity3d.com/index.php/Distance_from_a_point_to_a_rectangle ========== to ========== This function is distance calculation Algorithms[1] between Rectangle and Point. [1]http://wiki.unity3d.com/index.php/Distance_from_a_point_to_a_rectangle ==========
Description was changed from ========== This function is distance calculation Algorithms[1] between Rectangle and Point. [1]http://wiki.unity3d.com/index.php/Distance_from_a_point_to_a_rectangle ========== to ========== This function is distance calculation Algorithms[1] between Rectangle and Point. [1]http://wiki.unity3d.com/index.php /Distance_from_a_point_to_a_rectangle ==========
Description was changed from ========== This function is distance calculation Algorithms[1] between Rectangle and Point. [1]http://wiki.unity3d.com/index.php /Distance_from_a_point_to_a_rectangle ========== to ========== This function is distance calculation Algorithms[1] between Rectangle and Point. [1]http://wiki.unity3d.com/index.php/Distance_from_a_point_to_a_rectangle ==========
Description was changed from ========== This function is distance calculation Algorithms[1] between Rectangle and Point. [1]http://wiki.unity3d.com/index.php/Distance_from_a_point_to_a_rectangle ========== to ========== This function is distance calculation Algorithm[1] between Rectangle and Point. [1]http://wiki.unity3d.com/index.php/Distance_from_a_point_to_a_rectangle ==========
hyunjune.kim@samsung.com changed reviewers: + fs@opera.com, pdr@chromium.org
pdr, Could you check this CL? Thank you.
Description was changed from ========== This function is distance calculation Algorithm[1] between Rectangle and Point. [1]http://wiki.unity3d.com/index.php/Distance_from_a_point_to_a_rectangle ========== to ========== Implement FloatRect::distanceTo function This function is distance calculation Algorithm[1] between Rectangle and Point. [1]http://wiki.unity3d.com/index.php/Distance_from_a_point_to_a_rectangle ==========
Touched up the description a bit. I kind of like the "clamp" version (which runs a beter chance of ending up being branch-less), but I guess this works too - just need to work out what value to return =). (I think we could also make this "closestPoint" and leave it to the caller to decide what norm to use... Defeats a bit of the "optimizations" in this version though.) https://codereview.chromium.org/1580363002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/geometry/FloatRect.cpp (right): https://codereview.chromium.org/1580363002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/geometry/FloatRect.cpp:177: if (point.x() < this->x()) { I don't see the need for explicit this - here and elsewhere. https://codereview.chromium.org/1580363002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/geometry/FloatRect.cpp:179: distance = (point - FloatPoint(this->x(), this->y())).diagonalLengthSquared(); FloatRect has minXMinYCorner (etc.) which would make these slightly easier to read IMHO. https://codereview.chromium.org/1580363002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/geometry/FloatRect.cpp:181: distance = (point - FloatPoint(this->x(), this->maxY())).diagonalLengthSquared(); The 'corner' areas compute the squared distance while the other areas compute "proper" distances. Squared distances often makes sense, but regardless all cases should use the same convention. Calling the method just "distanceTo" I'd expect the Euclidian norm though (else "squaredDistanceTo" if a squared distance was returned.) https://codereview.chromium.org/1580363002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/geometry/FloatRect.h (right): https://codereview.chromium.org/1580363002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/geometry/FloatRect.h:172: float distanceTo(const FloatPoint&); const https://codereview.chromium.org/1580363002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/geometry/FloatRectTest.cpp (right): https://codereview.chromium.org/1580363002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/geometry/FloatRectTest.cpp:1: // Copyright 2015 The Chromium Authors. All rights reserved. 2016
On 2016/01/13 10:22:03, fs wrote:
> Touched up the description a bit.
>
> I kind of like the "clamp" version (which runs a beter chance of ending up
being
> branch-less), but I guess this works too - just need to work out what value to
> return =). (I think we could also make this "closestPoint" and leave it to the
> caller to decide what norm to use... Defeats a bit of the "optimizations" in
> this version though.)
8 months age, we made same function on LayoutSVGInlineText.cpp.
Like this,
static float squaredDistanceToClosestPoint(const FloatRect& rect, const
FloatPoint& point)
{
FloatPoint closestPoint;
closestPoint.setX(std::max(std::min(point.x(), rect.maxX()), rect.x()));
closestPoint.setY(std::max(std::min(point.y(), rect.maxY()), rect.y()));
return (point - closestPoint).diagonalLengthSquared();
}
If it's possible, i will change like this.
On 2016/01/13 10:44:23, hyunjunekim2 wrote:
> On 2016/01/13 10:22:03, fs wrote:
> > Touched up the description a bit.
> >
> > I kind of like the "clamp" version (which runs a beter chance of ending up
> being
> > branch-less), but I guess this works too - just need to work out what value
to
> > return =). (I think we could also make this "closestPoint" and leave it to
the
> > caller to decide what norm to use... Defeats a bit of the "optimizations" in
> > this version though.)
>
> 8 months age, we made same function on LayoutSVGInlineText.cpp.
> Like this,
> static float squaredDistanceToClosestPoint(const FloatRect& rect, const
> FloatPoint& point)
> {
> FloatPoint closestPoint;
> closestPoint.setX(std::max(std::min(point.x(), rect.maxX()), rect.x()));
> closestPoint.setY(std::max(std::min(point.y(), rect.maxY()), rect.y()));
> return (point - closestPoint).diagonalLengthSquared();
> }
>
> If it's possible, i will change like this.
And you gave me a advice which use clampTo on patch 1541083002.
Description was changed from ========== Implement FloatRect::distanceTo function This function is distance calculation Algorithm[1] between Rectangle and Point. [1]http://wiki.unity3d.com/index.php/Distance_from_a_point_to_a_rectangle ========== to ========== Implement FloatRect::distanceTo function This function is distance calculation between Rectangle and Point. ==========
Description was changed from ========== Implement FloatRect::distanceTo function This function is distance calculation between Rectangle and Point. ========== to ========== Implement FloatRect::distanceTo function This function is distance calculation between Rectangle and Point. ==========
fs, Could you check PS3? Thank you.
On 2016/01/13 13:55:41, hyunjunekim2 wrote: > fs, Could you check PS3? Thank you. PS4, replace clampTo with clampTo<float>.
https://codereview.chromium.org/1580363002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/geometry/FloatRect.cpp (right): https://codereview.chromium.org/1580363002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/geometry/FloatRect.cpp:174: float FloatRect::distanceTo(const FloatPoint& point) const squaredDistanceTo https://codereview.chromium.org/1580363002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/geometry/FloatRect.cpp:177: closestPoint.setX(clampTo<float>(point.x(), this->minXMinYCorner().x(), this->maxX())); With this formulation minXMinYCorner() doesn't help in anyway - just use x() (and ditto y()). Also, drop "this->". The (explicit) template argument should be needed either I think.
On 2016/01/13 14:36:17, fs wrote: > https://codereview.chromium.org/1580363002/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/platform/geometry/FloatRect.cpp (right): > > https://codereview.chromium.org/1580363002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/geometry/FloatRect.cpp:174: float > FloatRect::distanceTo(const FloatPoint& point) const > squaredDistanceTo > > https://codereview.chromium.org/1580363002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/geometry/FloatRect.cpp:177: > closestPoint.setX(clampTo<float>(point.x(), this->minXMinYCorner().x(), > this->maxX())); > With this formulation minXMinYCorner() doesn't help in anyway - just use x() > (and ditto y()). Also, drop "this->". The (explicit) template argument should be > needed either I think. fs, uploaded PS6. Thank you.
Description was changed from ========== Implement FloatRect::distanceTo function This function is distance calculation between Rectangle and Point. ========== to ========== Implement FloatRect::squaredDistanceTo function This function is squared distance calculation between Rectangle and Point. ==========
Description was changed from ========== Implement FloatRect::squaredDistanceTo function This function is squared distance calculation between Rectangle and Point. ========== to ========== Implement FloatRect::squaredDistanceTo function This function is that calculate distance squared between Rectangle and Point. ==========
On 2016/01/13 at 15:01:59, hyunjune.kim wrote: > On 2016/01/13 14:36:17, fs wrote: > > https://codereview.chromium.org/1580363002/diff/60001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/platform/geometry/FloatRect.cpp (right): > > > > https://codereview.chromium.org/1580363002/diff/60001/third_party/WebKit/Sour... > > third_party/WebKit/Source/platform/geometry/FloatRect.cpp:174: float > > FloatRect::distanceTo(const FloatPoint& point) const > > squaredDistanceTo > > > > https://codereview.chromium.org/1580363002/diff/60001/third_party/WebKit/Sour... > > third_party/WebKit/Source/platform/geometry/FloatRect.cpp:177: > > closestPoint.setX(clampTo<float>(point.x(), this->minXMinYCorner().x(), > > this->maxX())); > > With this formulation minXMinYCorner() doesn't help in anyway - just use x() > > (and ditto y()). Also, drop "this->". The (explicit) template argument should be > > needed either I think. > > > fs, uploaded PS6. Thank you. Non-owner LGTM - I guess it's still possible that pdr want the version he referenced though =)
On 2016/01/13 at 15:17:33, fs wrote: > On 2016/01/13 at 15:01:59, hyunjune.kim wrote: > > On 2016/01/13 14:36:17, fs wrote: > > > https://codereview.chromium.org/1580363002/diff/60001/third_party/WebKit/Sour... > > > File third_party/WebKit/Source/platform/geometry/FloatRect.cpp (right): > > > > > > https://codereview.chromium.org/1580363002/diff/60001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/platform/geometry/FloatRect.cpp:174: float > > > FloatRect::distanceTo(const FloatPoint& point) const > > > squaredDistanceTo > > > > > > https://codereview.chromium.org/1580363002/diff/60001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/platform/geometry/FloatRect.cpp:177: > > > closestPoint.setX(clampTo<float>(point.x(), this->minXMinYCorner().x(), > > > this->maxX())); > > > With this formulation minXMinYCorner() doesn't help in anyway - just use x() > > > (and ditto y()). Also, drop "this->". The (explicit) template argument should be > > > needed either I think. > > > > > > fs, uploaded PS6. Thank you. > > Non-owner LGTM - I guess it's still possible that pdr want the version he referenced though =) BTW, I think you should add a BUG= reference too (445329).
Description was changed from ========== Implement FloatRect::squaredDistanceTo function This function is that calculate distance squared between Rectangle and Point. ========== to ========== Implement FloatRect::squaredDistanceTo function This function is that calculate distance squared between Rectangle and Point. BUG=445329 ==========
pdr, Could you check this CL?
On 2016/01/13 at 23:15:56, hyunjune.kim wrote: > pdr, Could you check this CL? Nice! LGTM
The CQ bit was checked by hyunjune.kim@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1580363002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1580363002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_dbg_recipe on tryserver.chromium.android (JOB_TIMED_OUT, no build URL) android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_TIMED_OUT, no build URL) android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_TIMED_OUT, no build URL) android_clang_dbg_recipe on tryserver.chromium.android (JOB_TIMED_OUT, no build URL) android_compile_dbg on tryserver.chromium.android (JOB_TIMED_OUT, no build URL) cast_shell_android on tryserver.chromium.android (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by hyunjune.kim@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1580363002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1580363002/100001
Message was sent while issue was closed.
Description was changed from ========== Implement FloatRect::squaredDistanceTo function This function is that calculate distance squared between Rectangle and Point. BUG=445329 ========== to ========== Implement FloatRect::squaredDistanceTo function This function is that calculate distance squared between Rectangle and Point. BUG=445329 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Implement FloatRect::squaredDistanceTo function This function is that calculate distance squared between Rectangle and Point. BUG=445329 ========== to ========== Implement FloatRect::squaredDistanceTo function This function is that calculate distance squared between Rectangle and Point. BUG=445329 Committed: https://crrev.com/ac7237a24b28daa6f1c5292f32e940cc63fba39d Cr-Commit-Position: refs/heads/master@{#369334} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/ac7237a24b28daa6f1c5292f32e940cc63fba39d Cr-Commit-Position: refs/heads/master@{#369334} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
