Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix CGPathEqualToPath Implementation #1813

Merged
merged 4 commits into from
Jan 27, 2017
Merged

Fix CGPathEqualToPath Implementation #1813

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
69bf98b
Fix CGPathEqualToPath
MSFTFox Jan 25, 2017
d7f3a95
Address naming feedback and coding conventions.
MSFTFox Jan 25, 2017
3dccce2
Address CR Feedback.
MSFTFox Jan 26, 2017
74d12bd
Fix overuse of m_ and provide more proper names for parameters
MSFTFox Jan 26, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
93 changes: 80 additions & 13 deletions Frameworks/CoreGraphics/CGPath.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,11 +282,76 @@ HRESULT _CGPathGetGeometry(CGPathRef path, ID2D1Geometry** pGeometry) {
CFTypeID CGPathGetTypeID() {
return __CGPath::GetTypeID();
}
namespace {
// A helper for determining the number of points per path element type
static inline size_t __CGPathGetExpectedPointCountForType(CGPathElementType type) {
switch (type) {
case kCGPathElementMoveToPoint:
case kCGPathElementAddLineToPoint:
return 1;
case kCGPathElementAddQuadCurveToPoint:
return 2;
case kCGPathElementAddCurveToPoint:
return 3;
case kCGPathElementCloseSubpath:
Copy link
Contributor

@aballway aballway Jan 25, 2017 •
edited by MSFTFox
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: also default here #Resolved

return 0;
default:
TraceError(TAG, L"Invalid CGPathElementType encountered.");
return 0;
}
}

// Create a mimic of CGPathElement that holds the points in a vector with a convenient copy constructor.
struct __CGPathElementVector {
CGPathElementType type;
std::vector<CGPoint> points;
__CGPathElementVector(const CGPathElement& el)
: type(el.type), points(el.points, el.points + __CGPathGetExpectedPointCountForType(el.type)) {
}
};

// A struct to pass to the equality matching CGPathApply since only a single void* may be passed.
struct __CGPathElementMatch {
std::vector<__CGPathElementVector> elements;
bool equal = true;
int positionToMatch = 0;
};
}

// A function to pass to CGPathApply to determine whether two paths are equal.
static void __CGPathApplyCheckEquality(void* pathMatchContext, const CGPathElement* element1) {
__CGPathElementMatch* matchingContext = (__CGPathElementMatch*)pathMatchContext;

// If the matching has already failed, simply return asap. There's no way to stop a CGPathApply early.
if (!matchingContext->equal) {
return;
}
int i = matchingContext->positionToMatch;
__CGPathElementVector element2 = matchingContext->elements[i];
if (element2.type != element1->type) {
matchingContext->equal = false;
} else if (element2.type != kCGPathElementCloseSubpath) {
for (int i = 0; i < element2.points.size(); i++) {
if (element1->points[i] != element2.points[i]) {
matchingContext->equal = false;
break;
}
}
}

matchingContext->positionToMatch++;
}

// A function to pass to CGPathApply to retrieve the individual path elements to check equality against.
static void _CGPathApplyGetElements(void* pathElements, const CGPathElement* element) {
((std::vector<__CGPathElementVector>*)pathElements)->emplace_back(*element);
}

static Boolean __CGPathEqual(CFTypeRef cf1, CFTypeRef cf2) {
if (!cf1 && !cf2) {
if (cf1 == cf2) {
Copy link

@DHowett-MSFT DHowett-MSFT Jan 25, 2017 •
edited by MSFTFox
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The above check is redundant with this one. Remove the above one in favour of this one #Resolved

return true;
}

RETURN_FALSE_IF(!cf1);
RETURN_FALSE_IF(!cf2);

Expand All @@ -296,20 +361,22 @@ static Boolean __CGPathEqual(CFTypeRef cf1, CFTypeRef cf2) {
RETURN_FALSE_IF_FAILED(path1->ClosePath());
RETURN_FALSE_IF_FAILED(path2->ClosePath());

// ID2D1 Geometries have no isEquals method. However, for two geometries to be equal they are both reported to contain the other.
// Thus we must do two comparisons.
D2D1_GEOMETRY_RELATION relation = D2D1_GEOMETRY_RELATION_UNKNOWN;
RETURN_FALSE_IF_FAILED(path1->GetPathGeometry()->CompareWithGeometry(path2->GetPathGeometry(), D2D1::IdentityMatrix(), &relation));

// Does path 1 contain path 2?
if (relation == D2D1_GEOMETRY_RELATION_IS_CONTAINED) {
RETURN_FALSE_IF_FAILED(path2->GetPathGeometry()->CompareWithGeometry(path1->GetPathGeometry(), D2D1::IdentityMatrix(), &relation));

// Return true if path 2 also contains path 1
return (relation == D2D1_GEOMETRY_RELATION_IS_CONTAINED ? true : false);
// Check the segment count of the path as they must be equal.
UINT32 count1;
UINT32 count2;
RETURN_FALSE_IF_FAILED(path1->GetPathGeometry()->GetSegmentCount(&count1));
RETURN_FALSE_IF_FAILED(path2->GetPathGeometry()->GetSegmentCount(&count2));
if (count1 != count2) {
return false;
}

return false;
std::vector<__CGPathElementVector> path1Elements;
CGPathApply(path1, &path1Elements, _CGPathApplyGetElements);
__CGPathElementMatch match;
match.elements = path1Elements;
CGPathApply(path2, &match, __CGPathApplyCheckEquality);

return match.equal;
}

/**
Expand Down
44 changes: 36 additions & 8 deletions tests/unittests/CoreGraphics/CGPathTests.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -759,7 +759,7 @@ static bool testSymmetricEquivalence(CGPathRef path1, CGPathRef path2) {
return (CGPathEqualToPath(path1, path2) && CGPathEqualToPath(path2, path1));
}

DISABLED_TEST(CGPath, CGPathEqualsTest) {
TEST(CGPath, CGPathEqualsTest) {
CGMutablePathRef path1 = CGPathCreateMutable();
CGMutablePathRef path2 = CGPathCreateMutable();

Expand All @@ -784,12 +784,12 @@ static bool testSymmetricEquivalence(CGPathRef path1, CGPathRef path2) {
CGPathMoveToPoint(path1, nullptr, 50, 50);
CGPathAddLineToPoint(path1, nullptr, 100, 100);

// Two segments should be the same as one longer segment.
// Two segments are not the same as one longer segment even if they may appear the same.
CGPathMoveToPoint(path2, nullptr, 50, 50);
CGPathAddLineToPoint(path2, nullptr, 75, 75);
CGPathAddLineToPoint(path2, nullptr, 100, 100);

EXPECT_TRUE(testSymmetricEquivalence(path1, path2));
EXPECT_FALSE(testSymmetricEquivalence(path1, path2));

// Rectangles
CGRect theRectangle = CGRectMake(50, 50, 100, 100);
Expand Down Expand Up @@ -835,21 +835,21 @@ static bool testSymmetricEquivalence(CGPathRef path1, CGPathRef path2) {
CGPathRelease(rectPath1);
CGPathRelease(rectPath2);

// Two complicated paths that should be equal.
// Two paths with multiple segments that should be equal.
theRectangle = CGRectMake(50, 50, 100, 100);
rectPath1 = CGPathCreateWithRect(theRectangle, nullptr);
rectPath2 = CGPathCreateWithRect(theRectangle, nullptr);

CGPathAddLineToPoint(rectPath1, nullptr, 200, 200);

// The multiple line segments should not affect equality since it's still a straight line to the same point.
// The multiple line segments affect equality since individual segments much match.
CGPathAddLineToPoint(rectPath2, nullptr, 97, 97);
CGPathAddLineToPoint(rectPath2, nullptr, 124, 124);
CGPathAddLineToPoint(rectPath2, nullptr, 139, 139);
CGPathAddLineToPoint(rectPath2, nullptr, 187, 187);
CGPathAddLineToPoint(rectPath2, nullptr, 200, 200);

// Direction shoudl not matter either
// Direction matters
CGPathAddLineToPoint(rectPath1, nullptr, 230, 200);
CGPathAddLineToPoint(rectPath1, nullptr, 230, 230);
CGPathAddLineToPoint(rectPath1, nullptr, 200, 200);
Expand All @@ -858,7 +858,7 @@ static bool testSymmetricEquivalence(CGPathRef path1, CGPathRef path2) {
CGPathAddLineToPoint(rectPath2, nullptr, 230, 200);
CGPathAddLineToPoint(rectPath2, nullptr, 200, 200);

EXPECT_TRUE(testSymmetricEquivalence(rectPath1, rectPath2));
EXPECT_FALSE(testSymmetricEquivalence(rectPath1, rectPath2));

CGPathRelease(path1);
CGPathRelease(path2);
Expand All @@ -871,7 +871,35 @@ static bool testSymmetricEquivalence(CGPathRef path1, CGPathRef path2) {
CGPathRelease(containedRectangle);
}

DISABLED_TEST(CGPath, SubShapesEqualityTest) {
TEST(CGPath, CGPathEqualsCopyTest) {
CGMutablePathRef thePath = CGPathCreateMutable();
CGFloat width = 500.0f;
CGFloat height = 500.0f;
CGFloat xstart = 50.0f;
CGFloat ystart = 50.0f;

CGPathMoveToPoint(thePath, NULL, xstart + .75 * width, ystart + .5 * height);
CGPathAddArc(thePath, NULL, xstart + .5 * width, ystart + .5 * height, .5 * height, 0, M_PI / 2, true);
CGPathAddArc(thePath, NULL, xstart + .5 * width, ystart + .5 * height, .5 * height, M_PI / 2, 0, true);
CGPathMoveToPoint(thePath, NULL, xstart + .25 * width, ystart + .5 * height);
CGPathAddArc(thePath, NULL, xstart + .375 * width, ystart + .5 * height, .25 * height, M_PI, 0, false);
CGPathMoveToPoint(thePath, NULL, xstart + .5 * width, ystart + .5 * height);
CGPathAddArc(thePath, NULL, xstart + .625 * width, ystart + .5 * height, .25 * height, M_PI, 0, true);
CGPathMoveToPoint(thePath, NULL, xstart + .4375 * width, ystart + .5 * height);
CGPathAddArc(thePath, NULL, xstart + .375 * width, ystart + .5 * height, .125 * height, 0, M_PI / 2, true);
CGPathAddArc(thePath, NULL, xstart + .375 * width, ystart + .5 * height, .125 * height, M_PI / 2, 0, true);
CGPathMoveToPoint(thePath, NULL, xstart + .6875 * width, ystart + .5 * height);
CGPathAddArc(thePath, NULL, xstart + .625 * width, ystart + .5 * height, .125 * height, 0, M_PI / 2, true);
CGPathAddArc(thePath, NULL, xstart + .625 * width, ystart + .5 * height, .125 * height, M_PI / 2, 0, true);

CGMutablePathRef thePathCopy = CGPathCreateCopy(thePath);
EXPECT_TRUE(testSymmetricEquivalence(thePath, thePathCopy));

CGPathRelease(thePath);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: could use AutoCF here, but w/e

CGPathRelease(thePathCopy);
}

TEST(CGPath, SubShapesEqualityTest) {
CGMutablePathRef path1 = CGPathCreateMutable();
CGMutablePathRef path2 = CGPathCreateMutable();

Expand Down