Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2388833002:
Fix SkPath::arcTo when sweepAngle is tiny and radius is big (Closed)
DescriptionFix SkPath::arcTo when sweepAngle is tiny and radius is big
In this function, it first check whether this arc is a lone point
or not. If not, it converts angles to unit vectors. The problem
here is that when the radius is huge and the sweepAngle is small,
the function angles_to_unit_vectors() could return a startV ==stopV.
When that happens, it will draw a dot at the point that corresponding
to the startAngle. This CL adds a special branch for this case, and
draw a connecting line between the points at startAngle and endAngle.
BUG=640031, skia:5807
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2388833002
Committed: https://skia.googlesource.com/skia/+/6069ddabd8385ff838236dc25d7354e71649c9f3
Patch Set 1 #Patch Set 2 : real fix #Patch Set 3 : radiusX != radiusY #Patch Set 4 : remove stdio #
Total comments: 8
Patch Set 5 : address comments #Patch Set 6 : add a comment in code #Patch Set 7 : fix compile error on win #Messages
Total messages: 37 (21 generated)
Description was changed from ========== Fix a small error in SkPath::build_arc_conics() Currently in this function, when we cannot build a arc conic from the input parameters, we let the caller line-to the singlePt, while singlePt is calculated from the start point. This CL changes the singlePt to be calculated from the dst point. BUG=640031 ========== to ========== Fix a small error in SkPath::build_arc_conics() Currently in this function, when we cannot build a arc conic from the input parameters, we let the caller line-to the singlePt, while singlePt is calculated from the start point. This CL changes the singlePt to be calculated from the dst point. BUG=640031 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2388833002 ==========
Description was changed from ========== Fix a small error in SkPath::build_arc_conics() Currently in this function, when we cannot build a arc conic from the input parameters, we let the caller line-to the singlePt, while singlePt is calculated from the start point. This CL changes the singlePt to be calculated from the dst point. BUG=640031 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2388833002 ========== to ========== Fix a small error in SkPath::build_arc_conics() Currently in this function, when we cannot build a arc conic from the input parameters (e.g., when the sweepAngle is very very small), we let the caller line-to the singlePt, while singlePt is calculated from the start point. This CL changes the singlePt to be calculated from the dst point. BUG=640031 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2388833002 ==========
xidachen@chromium.org changed reviewers: + fmalita@chromium.org, reed@google.com, robertphillips@google.com
The CQ bit was checked by xidachen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL. Does this make sense?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
reed@google.com changed reviewers: + caryclark@google.com
can you create a gm or unittest that shows this CL fixing the behavior?
On 2016/10/03 15:22:47, reed1 wrote: > can you create a gm or unittest that shows this CL fixing the behavior? Should I just add a new case in gm/addarc.cpp, or is there any suggested place?
On 2016/10/03 15:24:47, xidachen wrote: > On 2016/10/03 15:22:47, reed1 wrote: > > can you create a gm or unittest that shows this CL fixing the behavior? > > Should I just add a new case in gm/addarc.cpp, or is there any suggested place? That would be fine.
Description was changed from ========== Fix a small error in SkPath::build_arc_conics() Currently in this function, when we cannot build a arc conic from the input parameters (e.g., when the sweepAngle is very very small), we let the caller line-to the singlePt, while singlePt is calculated from the start point. This CL changes the singlePt to be calculated from the dst point. BUG=640031 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2388833002 ========== to ========== Fix SkPath::arcTo when sweepAngle is tiny and radius is big In this function, it first check whether this arc is a lone point or not. If not, it converts angles to unit vectors. The problem here is that when the radius is huge and the sweepAngle is small, the function angles_to_unit_vectors() could return a startV ==stopV. When that happens, it will draw a dot at the point that corresponding to the startAngle. This CL adds a special branch for this case, and draw a connecting line between the points at startAngle and endAngle. BUG=640031 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2388833002 ==========
xidachen@chromium.org changed reviewers: + junov@chromium.org
New patch is ready, the title and description for CL are changed as well.
gentle ping
https://codereview.chromium.org/2388833002/diff/60001/src/core/SkPath.cpp File src/core/SkPath.cpp (right): https://codereview.chromium.org/2388833002/diff/60001/src/core/SkPath.cpp#new... src/core/SkPath.cpp:1302: oval.centerY() + radiusY * sinf(endAngle)); SkScalarSinCos() is preferable https://codereview.chromium.org/2388833002/diff/60001/src/core/SkPath.cpp#new... src/core/SkPath.cpp:1303: forceMoveTo ? this->moveTo(singlePt) : this->lineTo(singlePt); Does your example GM trigger both sides of this expression?
https://codereview.chromium.org/2388833002/diff/60001/src/core/SkPath.cpp File src/core/SkPath.cpp (right): https://codereview.chromium.org/2388833002/diff/60001/src/core/SkPath.cpp#new... src/core/SkPath.cpp:1302: oval.centerY() + radiusY * sinf(endAngle)); On 2016/10/05 14:08:11, caryclark wrote: > SkScalarSinCos() is preferable While I do agree that SkScalarSinCos() is preferable, it doesn't work here. The fundamental problem of the bug is that sinf(sweepAngle) is very very small, it is smaller than *SkScalarNearlyZero*, so when we call SkScalarSinCos() we end up having sin = 0 and cos = 1, that causes the bug. Because when the radius is huge, e.g., 100000, even if sin(sweepAngle) = 0.0001, we should still draw a line. Unfortunately 0.0001 is smaller than *SkScalarNearlyZero*. The *SkScalarNearlyZero* threshould is actually the reason why we got startV == stopV in the function angles_to_unit_vectors(). https://codereview.chromium.org/2388833002/diff/60001/src/core/SkPath.cpp#new... src/core/SkPath.cpp:1303: forceMoveTo ? this->moveTo(singlePt) : this->lineTo(singlePt); On 2016/10/05 14:08:11, caryclark wrote: > Does your example GM trigger both sides of this expression? No, it only triggers lineTo. This line is basically copied from line 1317 below.
https://codereview.chromium.org/2388833002/diff/60001/src/core/SkPath.cpp File src/core/SkPath.cpp (right): https://codereview.chromium.org/2388833002/diff/60001/src/core/SkPath.cpp#new... src/core/SkPath.cpp:1302: oval.centerY() + radiusY * sinf(endAngle)); A comment in the code to this effect would be helpful here. How about sk_float_sin and sk_float_cos? https://codereview.chromium.org/2388833002/diff/60001/src/core/SkPath.cpp#new... src/core/SkPath.cpp:1303: forceMoveTo ? this->moveTo(singlePt) : this->lineTo(singlePt); It would be great if was possible to verify that the new code path works
https://codereview.chromium.org/2388833002/diff/60001/src/core/SkPath.cpp File src/core/SkPath.cpp (right): https://codereview.chromium.org/2388833002/diff/60001/src/core/SkPath.cpp#new... src/core/SkPath.cpp:1302: oval.centerY() + radiusY * sinf(endAngle)); On 2016/10/05 14:47:03, caryclark wrote: > A comment in the code to this effect would be helpful here. > > How about sk_float_sin and sk_float_cos? comment added. sk_float_sin and sk_float_cos works fine. I have changed to those two. https://codereview.chromium.org/2388833002/diff/60001/src/core/SkPath.cpp#new... src/core/SkPath.cpp:1303: forceMoveTo ? this->moveTo(singlePt) : this->lineTo(singlePt); On 2016/10/05 14:47:03, caryclark wrote: > It would be great if was possible to verify that the new code path works I believe SkPath::addArc calls arcTo with forceMoveTo == true. So I added a call to addArc in the gm/addarc, and we can see that when forceMoveTo is true, it doesn't draw a line. I hope that does correspond to what you asked.
Description was changed from ========== Fix SkPath::arcTo when sweepAngle is tiny and radius is big In this function, it first check whether this arc is a lone point or not. If not, it converts angles to unit vectors. The problem here is that when the radius is huge and the sweepAngle is small, the function angles_to_unit_vectors() could return a startV ==stopV. When that happens, it will draw a dot at the point that corresponding to the startAngle. This CL adds a special branch for this case, and draw a connecting line between the points at startAngle and endAngle. BUG=640031 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2388833002 ========== to ========== Fix SkPath::arcTo when sweepAngle is tiny and radius is big In this function, it first check whether this arc is a lone point or not. If not, it converts angles to unit vectors. The problem here is that when the radius is huge and the sweepAngle is small, the function angles_to_unit_vectors() could return a startV ==stopV. When that happens, it will draw a dot at the point that corresponding to the startAngle. This CL adds a special branch for this case, and draw a connecting line between the points at startAngle and endAngle. BUG=640031, skia:5807 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2388833002 ==========
lgtm
On 2016/10/05 16:43:21, caryclark wrote: > lgtm Thanks Cary. After this CL, there will be some Blink layout tests failure (every one of them is about 4-5 pixels difference). Please give me some time to label them as NeedsManualRebaseline so that the bots won't turn red when skia rolls in.
On 2016/10/05 17:31:52, xidachen wrote: > On 2016/10/05 16:43:21, caryclark wrote: > > lgtm > > Thanks Cary. After this CL, there will be some Blink layout tests failure (every > one of them is about 4-5 pixels difference). Please give me some time to label > them as NeedsManualRebaseline so that the bots won't turn red when skia rolls > in. I think you should be able to pre-flight the layout test diffs if you run the linux_precise_blink_rel trybot on this CL.
On 2016/10/05 17:36:34, f(malita) wrote: > On 2016/10/05 17:31:52, xidachen wrote: > > On 2016/10/05 16:43:21, caryclark wrote: > > > lgtm > > > > Thanks Cary. After this CL, there will be some Blink layout tests failure > (every > > one of them is about 4-5 pixels difference). Please give me some time to label > > them as NeedsManualRebaseline so that the bots won't turn red when skia rolls > > in. > > I think you should be able to pre-flight the layout test diffs if you run the > linux_precise_blink_rel trybot on this CL. Thanks, I started a CL: https://codereview.chromium.org/2396753003/ to prepare for this CL to land.
The CQ bit was checked by xidachen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Win-MSVC-x86-Debug-Trybot on master.client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86-D...)
The CQ bit was checked by xidachen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by xidachen@chromium.org
The CQ bit was checked by xidachen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from caryclark@google.com Link to the patchset: https://codereview.chromium.org/2388833002/#ps120001 (title: "fix compile error on win")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Fix SkPath::arcTo when sweepAngle is tiny and radius is big In this function, it first check whether this arc is a lone point or not. If not, it converts angles to unit vectors. The problem here is that when the radius is huge and the sweepAngle is small, the function angles_to_unit_vectors() could return a startV ==stopV. When that happens, it will draw a dot at the point that corresponding to the startAngle. This CL adds a special branch for this case, and draw a connecting line between the points at startAngle and endAngle. BUG=640031, skia:5807 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2388833002 ========== to ========== Fix SkPath::arcTo when sweepAngle is tiny and radius is big In this function, it first check whether this arc is a lone point or not. If not, it converts angles to unit vectors. The problem here is that when the radius is huge and the sweepAngle is small, the function angles_to_unit_vectors() could return a startV ==stopV. When that happens, it will draw a dot at the point that corresponding to the startAngle. This CL adds a special branch for this case, and draw a connecting line between the points at startAngle and endAngle. BUG=640031, skia:5807 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2388833002 Committed: https://skia.googlesource.com/skia/+/6069ddabd8385ff838236dc25d7354e71649c9f3 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://skia.googlesource.com/skia/+/6069ddabd8385ff838236dc25d7354e71649c9f3 |
