Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2226973004:
Refactor SkCurveMeasure to use existing eval code (Closed)
|
Created:
4 years, 4 months ago by Harry Stern Modified:
4 years, 4 months ago CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@curvemeasure_rework Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionRefactor SkCurveMeasure to use existing eval code
- Use quad, cubic, conic eval code from SkGeometry.h
- Implement evaluateDerivativeLength, evaluateDerivative and evaluate switch cases for lines along with the refactor
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2226973004
Committed: https://skia.googlesource.com/skia/+/4ab47e087ecfc82f070cbbaef4d9eb562d3fd163
Patch Set 1 #Patch Set 2 : Remove incomplete line fragment mistakenly included #
Total comments: 4
Patch Set 3 : Fix line interpolation and add UNIMPLEMENTED macro #Patch Set 4 : Fix minor stuff - comment and extra break; #Messages
Total messages: 22 (15 generated)
Description was changed from ========== Refactor SkCurveMeasure to use existing eval code - Use quad, cubic, conic eval code from SkGeometry.h - Implement ArcLengthIntegrator::evaluateDerivativeLength for lines - Implement evaluateDerivative and evaluate static functions for lines along with the refactor BUG=skia: ========== to ========== Refactor SkCurveMeasure to use existing eval code - Use quad, cubic, conic eval code from SkGeometry.h - Implement ArcLengthIntegrator::evaluateDerivativeLength for lines - Implement evaluateDerivative and evaluate static functions for lines along with the refactor BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2226973004 ==========
Description was changed from ========== Refactor SkCurveMeasure to use existing eval code - Use quad, cubic, conic eval code from SkGeometry.h - Implement ArcLengthIntegrator::evaluateDerivativeLength for lines - Implement evaluateDerivative and evaluate static functions for lines along with the refactor BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2226973004 ========== to ========== Refactor SkCurveMeasure to use existing eval code - Use quad, cubic, conic eval code from SkGeometry.h - Implement ArcLengthIntegrator::evaluateDerivativeLength for lines - Implement evaluateDerivative and evaluate static functions for lines along with the refactor BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2226973004 ==========
hstern@google.com changed reviewers: + caryclark@google.com, reed@google.com
The CQ bit was checked by hstern@google.com 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: This issue passed the CQ dry run.
Description was changed from ========== Refactor SkCurveMeasure to use existing eval code - Use quad, cubic, conic eval code from SkGeometry.h - Implement ArcLengthIntegrator::evaluateDerivativeLength for lines - Implement evaluateDerivative and evaluate static functions for lines along with the refactor BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2226973004 ========== to ========== Refactor SkCurveMeasure to use existing eval code - Use quad, cubic, conic eval code from SkGeometry.h - Implement evaluateDerivativeLength, evaluateDerivative and evaluate switch cases for lines along with the refactor BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2226973004 ==========
On 2016/08/09 21:00:44, Harry Stern wrote: I am unsure if I should actually have the implementations for lines in evaluate* functions and in the ArcLengthIntegrator constructor or not. I want to fast-path lines because they do not need their arclength approximated, but it may be useful to someone if they are implemented, and it is nice to have all of the cases in the switch statement. On the other hand, it may hurt readability/intelligibility if there are codepaths that are never taken and might be misleading.
lgtm https://codereview.chromium.org/2226973004/diff/20001/src/utils/SkCurveMeasur... File src/utils/SkCurveMeasure.cpp (right): https://codereview.chromium.org/2226973004/diff/20001/src/utils/SkCurveMeasur... src/utils/SkCurveMeasure.cpp:82: break; nit: don't need the break; https://codereview.chromium.org/2226973004/diff/20001/src/utils/SkCurveMeasure.h File src/utils/SkCurveMeasure.h (right): https://codereview.chromium.org/2226973004/diff/20001/src/utils/SkCurveMeasur... src/utils/SkCurveMeasure.h:51: // kCubic_SegType -> 3 points: start control1 control2 end cubic -> 4 ?
The CQ bit was checked by hstern@google.com 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: This issue passed the CQ dry run.
The CQ bit was checked by hstern@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from reed@google.com Link to the patchset: https://codereview.chromium.org/2226973004/#ps60001 (title: "Fix minor stuff - comment and extra break;")
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 ========== Refactor SkCurveMeasure to use existing eval code - Use quad, cubic, conic eval code from SkGeometry.h - Implement evaluateDerivativeLength, evaluateDerivative and evaluate switch cases for lines along with the refactor BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2226973004 ========== to ========== Refactor SkCurveMeasure to use existing eval code - Use quad, cubic, conic eval code from SkGeometry.h - Implement evaluateDerivativeLength, evaluateDerivative and evaluate switch cases for lines along with the refactor BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2226973004 Committed: https://skia.googlesource.com/skia/+/4ab47e087ecfc82f070cbbaef4d9eb562d3fd163 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://skia.googlesource.com/skia/+/4ab47e087ecfc82f070cbbaef4d9eb562d3fd163
Message was sent while issue was closed.
forgot to send out these comments, I guess it doesn't really matter. https://codereview.chromium.org/2226973004/diff/20001/src/utils/SkCurveMeasur... File src/utils/SkCurveMeasure.cpp (right): https://codereview.chromium.org/2226973004/diff/20001/src/utils/SkCurveMeasur... src/utils/SkCurveMeasure.cpp:82: break; On 2016/08/10 12:08:07, reed1 wrote: > nit: don't need the break; Done. https://codereview.chromium.org/2226973004/diff/20001/src/utils/SkCurveMeasure.h File src/utils/SkCurveMeasure.h (right): https://codereview.chromium.org/2226973004/diff/20001/src/utils/SkCurveMeasur... src/utils/SkCurveMeasure.h:51: // kCubic_SegType -> 3 points: start control1 control2 end On 2016/08/10 12:08:07, reed1 wrote: > cubic -> 4 ? Done.
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/2237503003/ by halcanary@google.com. The reason for reverting is: perf debug assert.. |
