inject moveTo after close (when needed)

[reed1, 2012-01-11 22:55:01]

draft for discussion

[Stephen Chenney, 2012-01-11 23:13:25]

With this change I think you can also remove the kAfterClose_SegmentState in
SkPath::Iter, setting the initial value for fSegmentState =
kAfterPrimitive_SegmentState;

http://codereview.appspot.com/5529074/diff/1/src/core/SkPath.cpp
File src/core/SkPath.cpp (right):

http://codereview.appspot.com/5529074/diff/1/src/core/SkPath.cpp#newcode545
src/core/SkPath.cpp:545: fLastMoveToIndex ^= fLastMoveToIndex >> (8 *
sizeof(fLastMoveToIndex) - 1);
Is this just a non-branching way of doing what you want? Very cute, and I hope
you ran it through all the tests.

[Stephen Chenney, 2012-01-11 23:14:46]

And I would suggest getting http://codereview.appspot.com/5533074/ in first as
it has a bit more testing.

(I wish these patches didn't both end in 074).

[reed1, 2012-01-12 16:33:04]

This seems to pass everything, but fails in PathTests.cpp : test_raw_iter. I am
wondering if the test just needs to be modified? I find that test rather dense,
perhaps it can be restructured to make clearer its assumptions/organization? Of
course, perhaps I still have a bug and the test is correct, I just can't tell
yet.

[Stephen Chenney, 2012-01-12 16:38:25]

I patched this in and got all the kinks sorted out (there were several).
Unfortunately, now it's all mingled in with my fixes for SkPathMeasure and not
easy to upload a patch. I think I may need to commit it along with the fix for
SkPathMeasure.

http://codereview.appspot.com/5529074/diff/1/src/core/SkPath.cpp
File src/core/SkPath.cpp (right):

http://codereview.appspot.com/5529074/diff/1/src/core/SkPath.cpp#newcode545
src/core/SkPath.cpp:545: fLastMoveToIndex ^= fLastMoveToIndex >> (8 *
sizeof(fLastMoveToIndex) - 1);
On 2012/01/11 23:13:25, Stephen Chenney wrote:
> Is this just a non-branching way of doing what you want? Very cute, and I hope
> you ran it through all the tests.

This in fact fails for the case when the fLastMoveToIndex is 0.

[Stephen Chenney, 2012-01-12 16:39:38]

On 2012/01/12 16:33:04, reed1 wrote:
> This seems to pass everything, but fails in PathTests.cpp : test_raw_iter. I
am
> wondering if the test just needs to be modified? I find that test rather
dense,
> perhaps it can be restructured to make clearer its assumptions/organization?
Of
> course, perhaps I still have a bug and the test is correct, I just can't tell
> yet.

The test needed updating - if the new code adds in additional moves then the raw
iterator will return those. The test was not accounting for it.

[reed1, 2012-01-12 16:47:30]

This revision has an updated PathTest that passes.

I'll file a separate bug to address test_raw_iter, and make it cleaner (not so
many magic literals everywhere, etc.)

[reed1, 2012-01-12 16:51:55]

this test (with tolerance added) now passes as well.


static void test_meas() {
    SkPath path;

    path.lineTo(SK_Scalar1, 0);
    path.lineTo(SK_Scalar1, SK_Scalar1);
    path.lineTo(0, SK_Scalar1);
    path.close();
    path.lineTo(-SK_Scalar1, 0);

    SkPathMeasure meas(path, false);
    SkScalar length = meas.getLength();
    SkASSERT(length == SK_Scalar1 * 4);
    meas.nextContour();
    length = meas.getLength();
    SkASSERT(length == SK_Scalar1);

    SkPoint position;
    SkVector tangent;
    SkASSERT(meas.getPosTan(SK_ScalarHalf, &position, &tangent));
    
    // this needs a tiny tolerance to pass
    SkASSERT(SkScalarNearlyEqual(position.fX, -SK_ScalarHalf));
    SkASSERT(SkScalarNearlyZero(position.fY));

    // this fails (I think we're confused by verb/pt mismatches)
    SkASSERT(SkScalarNearlyEqual(tangent.fX, -SK_Scalar1));
    SkASSERT(SkScalarNearlyZero(tangent.fY));
}

[Stephen Chenney, 2012-01-12 17:00:42]

Some clean up changes below. Otherwise LGTM.

http://codereview.appspot.com/5529074/diff/5001/include/core/SkPath.h
File include/core/SkPath.h (right):

http://codereview.appspot.com/5529074/diff/5001/include/core/SkPath.h#newcode755
include/core/SkPath.h:755: friend class Iter;
This can be removed.

http://codereview.appspot.com/5529074/diff/5001/include/core/SkPath.h#newcode756
include/core/SkPath.h:756: void cons_moveto();
As can this.

http://codereview.appspot.com/5529074/diff/5001/src/core/SkPath.cpp
File src/core/SkPath.cpp (right):

http://codereview.appspot.com/5529074/diff/5001/src/core/SkPath.cpp#newcode169
src/core/SkPath.cpp:169: SkTSwap<uint8_t>(fFillType, other.fFillType);
Need to swap here too.

http://codereview.appspot.com/5529074/diff/5001/src/core/SkPath.cpp#newcode1232
src/core/SkPath.cpp:1232: kAfterClose_SegmentState,     // We will need a move
next, but we have a
This is more precisely named "kEmptyContour_SegmentState"

http://codereview.appspot.com/5529074/diff/5001/src/core/SkPath.cpp#newcode1244
src/core/SkPath.cpp:1244: fSegmentState = kAfterPrimitive_SegmentState;
This should be kAfterClose_SegmentState to match setPath below.

http://codereview.appspot.com/5529074/diff/5001/src/core/SkPath.cpp#newcode1318
src/core/SkPath.cpp:1318: if (fSegmentState == kAfterClose_SegmentState) {
This if block can be removed - it can never be hit.

[reed1, 2012-01-12 18:18:07]

fixed the missing swap.

for clarity, I'll make those cleanups a separate change.

[reed1, 2012-01-12 18:22:18]

actually, I don't understand the other comments per-se. How can I remove
const_moveto from the header but not remove it from the .cpp? Can you propose a
CL with those cleanups?

[Stephen Chenney, 2012-01-12 18:25:02]

On 2012/01/12 18:22:18, reed1 wrote:
> actually, I don't understand the other comments per-se. How can I remove
> const_moveto from the header but not remove it from the .cpp? Can you propose
a
> CL with those cleanups?

Sure. Will do once this is in.

[Stephen Chenney, 2012-01-18 18:36:10]

I think we can close this now. Cleanup code is in
http://codereview.appspot.com/5532091/