Chromium Code Reviews
|
|
Chromium Code Reviews
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. |
Descriptionpathops: 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. #Messages
Total messages: 27 (10 generated)
Description was changed from ========== 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: ========== to ========== 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&is... ==========
Description was changed from ========== 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&is... ========== to ========== 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&is... ==========
deanm@chromium.org changed reviewers: + caryclark@google.com
I prefer not to add comments that repeat line for line what the code does, which is why I recommend using helper functions like between() and zero_or_one() to show the intent of the compares. I like block comments that explain what a complex block of code does, but the comment should be a signal to beware of dragons, look out below -- for two reasons: - I don't want to reread comments in the future that give me no more insight than a cursory glance at the code - Comments tend to rot over time, and worse than reading a comment that does not enhance is to be misled by a comment which is stale. https://codereview.chromium.org/1920663002/diff/1/src/pathops/SkPathOpsCubic.cpp File src/pathops/SkPathOpsCubic.cpp (right): https://codereview.chromium.org/1920663002/diff/1/src/pathops/SkPathOpsCubic.... src/pathops/SkPathOpsCubic.cpp:246: SkASSERT(ls >= 0 && ls <= 1 && ms >= 0 && ms <= 1); // Both in [0, 1]. use between, e.g.: SkASSERT(between(0, ls, 1) && between(0, ms, 1)); https://codereview.chromium.org/1920663002/diff/1/src/pathops/SkPathOpsCubic.... src/pathops/SkPathOpsCubic.cpp:248: SkASSERT(*t >= 0 && *t <= 1); // Average should also be in [0, 1]. use between https://codereview.chromium.org/1920663002/diff/1/src/pathops/SkPathOpsCubic.... src/pathops/SkPathOpsCubic.cpp:249: return *t > 0 && *t < 1; // Check that the cut isn't on an endpoint. use zero_or_one(t)
On 2016/04/25 14:46:31, caryclark wrote: > I prefer not to add comments that repeat line for line what the code does, which > is why I recommend using helper functions like between() and zero_or_one() to > show the intent of the compares. > > I like block comments that explain what a complex block of code does, but the > comment should be a signal to beware of dragons, look out below -- for two > reasons: > - I don't want to reread comments in the future that give me no more insight > than a cursory glance at the code > - Comments tend to rot over time, and worse than reading a comment that does not > enhance is to be misled by a comment which is stale. > Yes, I agree about the comments, but in this case it took me a few passes to understand that this code was in a projective space and therefor I added the comments to make it more clear that if between(0, ms, mt) actually means that t is [0, 1]. I guess it's pretty obvious after the fact but on the first reading for me it wasn't. > https://codereview.chromium.org/1920663002/diff/1/src/pathops/SkPathOpsCubic.cpp > File src/pathops/SkPathOpsCubic.cpp (right): > > https://codereview.chromium.org/1920663002/diff/1/src/pathops/SkPathOpsCubic.... > src/pathops/SkPathOpsCubic.cpp:246: SkASSERT(ls >= 0 && ls <= 1 && ms >= 0 && ms > <= 1); // Both in [0, 1]. > use between, e.g.: > SkASSERT(between(0, ls, 1) && between(0, ms, 1)); Sure > > https://codereview.chromium.org/1920663002/diff/1/src/pathops/SkPathOpsCubic.... > src/pathops/SkPathOpsCubic.cpp:248: SkASSERT(*t >= 0 && *t <= 1); // Average > should also be in [0, 1]. > use between Sure > > https://codereview.chromium.org/1920663002/diff/1/src/pathops/SkPathOpsCubic.... > src/pathops/SkPathOpsCubic.cpp:249: return *t > 0 && *t < 1; // Check that the > cut isn't on an endpoint. > use zero_or_one(t) This line I just left as is from the old code. I can change it to !zero_or_one(t). It doesn't seem like there is an exclusive version of between, only an inclusive version. It should never happen (that is what the assert is for) that the range is outside of 0 .. 1, but I thought it was a bit more "defensive" to leave the check as it was instead of checking *t != 0 && *t != 1.
Updated to be clearer by using between(). I left the return statement but can change it to !zero_or_one(*t) if you prefer.
On 2016/04/25 15:02:04, Dean McNamee wrote: > Updated to be clearer by using between(). > > I left the return statement but can change it to !zero_or_one(*t) if you prefer. Also just a note as an aside, between(), zero_or_one(), etc operate on doubles, while the code here is using SkScalar (float). This just means that they might be promoted to doubles for the comparison which is not harmful but perhaps a bit wasteful. Anyway I still agree that it reads much clearer with your recommendations.
This fails with this change and succeeds without: ninja -C out/Release pathops_unittest && out/Release/pathops_unittest.exe -v -V -x -m PathOpsOpLoopsThreaded I haven't investigated why.
I think it was a problem for points very close but just outside of t [0, 1]. Perhaps also the point can be moved a little bit later during alignment? I always hate to add an epsilon test, but I guess that is the reality. All tests pass now. Finished 90 tests, 0 failures, 0 skipped. (140751312 internal tests)
lgtm Moving points is tricky; I'm still trying to get it right in my current CL. I'm not a fan of epsilon tests either but here they are a necessary evil.
By the way, I'm not a big fan of CLs that mix documentation and function. Please correct me if I'm missing reading this, but the change to SkOpEdgeBuilder.cpp is strictly syntactical, yes? In the future, if by chance a failure is isolated to this CL, I prefer not to spend time analyzing and reverting files that are syntactic-only when they're mixed in with functional changes. Sometimes this is inevitable, so it's not a hard and fast rule, just my personal preference.
The CQ bit was checked by deanm@chromium.org to run a CQ dry run
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
The CQ bit was unchecked by commit-bot@chromium.org
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...) Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...) Build-Ubuntu-Clang-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-Clang-x...) Build-Ubuntu-GCC-Arm64-Debug-Android-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...) Build-Ubuntu-GCC-Arm7-Debug-Android-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...) Build-Ubuntu-GCC-Mips-Debug-Android-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Mip...) Build-Ubuntu-GCC-x86_64-Release-CMake-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x86...) Build-Ubuntu-GCC-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x86...)
> Yes. Do you prefer that I split the CLs? > > The change to the edge builder is just to make it clearer that > ComplexBreak breaks more curve types than just self-intersecting > loops. > > Definitely have no problem splitting the patch, just thought it's a > little bit annoying to review a tiny patch to change the variable name > and comment, and since it is the only call site of the function I was > changing I thought it made sense to just change them together. I am > fine with whatever you prefer. It would be great if we had better tools that would show me 'this file/line diff does not affect code generation' but until we do I prefer not to mix style and substance. As it is I think I've read and re-read the CL each time I opened it to verify that the SkOpEdgeBuilder is a doc-only change. Yes, I prefer separate CLs but given that we've hashed this to death, it's OK with me to wait until next time.
Moved cosmetic changed to separate CL: https://codereview.chromium.org/1921173003
The CQ bit was checked by deanm@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/1920663002/#ps60001 (title: "Remove documentation change of complex break in edge builder.")
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
The CQ bit was unchecked by commit-bot@chromium.org
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...) Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...) Test-Ubuntu-GCC-ShuttleA-GPU-GTX660-x86_64-Release-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-ShuttleA-GPU...) Build-Ubuntu-Clang-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-Clang-x...) Build-Ubuntu-GCC-Arm64-Debug-Android-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...) Build-Ubuntu-GCC-Arm7-Debug-Android-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...) Build-Ubuntu-GCC-Mips-Debug-Android-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Mip...) Build-Ubuntu-GCC-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x86...)
The CQ bit was checked by deanm@chromium.org
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
Message was sent while issue was closed.
Description was changed from ========== 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&is... ========== to ========== 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&is... Committed: https://skia.googlesource.com/skia/+/83496425e3216a1d3d1613a8ddd4d3bb7d71638a ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://skia.googlesource.com/skia/+/83496425e3216a1d3d1613a8ddd4d3bb7d71638a |
