Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(185)

Issue 1920663002: pathops: Split loop type cubics only when there is a self-intersection. (Closed)

Created:
4 years, 8 months ago by Dean McNamee
Modified:
4 years, 8 months ago
Reviewers:
caryclark
CC:
reviews_skia.org
Base URL:
https://skia.googlesource.com/skia.git/@master
Target Ref:
refs/heads/master
Project:
skia
Visibility:
Public.

Description

pathops: Split loop type cubics only when there is a self-intersection. The ComplexBreak code comes from Loop and Blinn, which requires loops to be split if either double point is visible. However for intersection loops only need to be split when there is actually a self-intersection (when both double points are in [0, 1]). This patch splits cubics much less often so the output doesn't have extra segments unless the input had a self-intersecting loop. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1920663002 Committed: https://skia.googlesource.com/skia/+/83496425e3216a1d3d1613a8ddd4d3bb7d71638a

Patch Set 1 #

Total comments: 3

Patch Set 2 : Reduce comments in favor of using between() #

Patch Set 3 : Use roughly_between for points very close to endpoints. #

Patch Set 4 : Remove documentation change of complex break in edge builder. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+4 lines, -4 lines) Patch
M src/pathops/SkPathOpsCubic.cpp View 1 2 1 chunk +4 lines, -4 lines 0 comments Download

Messages

Total messages: 27 (10 generated)
Dean McNamee
4 years, 8 months ago (2016-04-25 14:23:55 UTC) #4
caryclark
I prefer not to add comments that repeat line for line what the code does, ...
4 years, 8 months ago (2016-04-25 14:46:31 UTC) #5
Dean McNamee
On 2016/04/25 14:46:31, caryclark wrote: > I prefer not to add comments that repeat line ...
4 years, 8 months ago (2016-04-25 14:53:35 UTC) #6
Dean McNamee
Updated to be clearer by using between(). I left the return statement but can change ...
4 years, 8 months ago (2016-04-25 15:02:04 UTC) #7
Dean McNamee
On 2016/04/25 15:02:04, Dean McNamee wrote: > Updated to be clearer by using between(). > ...
4 years, 8 months ago (2016-04-25 15:11:06 UTC) #8
caryclark
This fails with this change and succeeds without: ninja -C out/Release pathops_unittest && out/Release/pathops_unittest.exe -v ...
4 years, 8 months ago (2016-04-25 19:45:36 UTC) #9
Dean McNamee
I think it was a problem for points very close but just outside of t ...
4 years, 8 months ago (2016-04-26 14:04:05 UTC) #10
caryclark
lgtm Moving points is tricky; I'm still trying to get it right in my current ...
4 years, 8 months ago (2016-04-26 14:39:34 UTC) #11
caryclark
By the way, I'm not a big fan of CLs that mix documentation and function. ...
4 years, 8 months ago (2016-04-26 14:46:33 UTC) #12
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1920663002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1920663002/40001
4 years, 8 months ago (2016-04-26 15:45:20 UTC) #14
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-Trybot/builds/8107) Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on ...
4 years, 8 months ago (2016-04-26 15:46:13 UTC) #16
caryclark
> Yes. Do you prefer that I split the CLs? > > The change to ...
4 years, 8 months ago (2016-04-26 16:09:32 UTC) #17
Dean McNamee
Moved cosmetic changed to separate CL: https://codereview.chromium.org/1921173003
4 years, 8 months ago (2016-04-26 16:27:29 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1920663002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1920663002/60001
4 years, 8 months ago (2016-04-26 17:12:00 UTC) #21
commit-bot: I haz the power
Try jobs failed on following builders: Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-Trybot/builds/8110) Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on client.skia (JOB_FAILED, ...
4 years, 8 months ago (2016-04-26 17:12:53 UTC) #23
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1920663002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1920663002/60001
4 years, 8 months ago (2016-04-26 20:12:45 UTC) #25
commit-bot: I haz the power
4 years, 8 months ago (2016-04-26 21:15:24 UTC) #27
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as
https://skia.googlesource.com/skia/+/83496425e3216a1d3d1613a8ddd4d3bb7d71638a

Powered by Google App Engine
This is Rietveld 408576698