Fix CGPathAddArc and add additional tests. #2136

MSFTFox · 2017-03-02T01:00:13Z

Correctly translates the CoreGraphics arc parameters into D2D1_ARC_SEGMENT parameters. Added additional testing to cover previously known failure situations.

Fix #2062

DHowett-MSFT · 2017-03-02T01:02:09Z

Frameworks/CoreGraphics/CGPath.mm

+
+    void SetIsFirstCall(bool firstCall) {
+        isFirstCall = firstCall;
+    }


I very much do not like this mechanism.

According to the docs - it is not the "first call", rather the existence of an open sub path - that should be the mechanism used here.

DHowett-MSFT

also, no test images?

DHowett-MSFT · 2017-03-02T01:03:55Z

Also, does this allow you to enable @msft-Jeyaram's arc clearing test?

msft-Jeyaram · 2017-03-02T01:06:47Z

tests/unittests/CoreGraphics.Drawing/CGPathDrawingTests.cpp

+            break;
+    }
+    if (moveToPoint == 0) {
+    }


msft-Jeyaram · 2017-03-02T01:10:02Z

@MSFTFox where are the generated base images for the tests?
I think it got missed in your addition

msft-Jeyaram · 2017-03-02T01:17:25Z

@MSFTFox I've also added some FillMode tests for Arcs.
These were parameterized tests that I've added to ensure arcs was working.
Please enable them when #2133 is in.
There is a TODO with issue id #2062

Also depending on who gets the changes in first, arc clearing tests need to be enabled.
The disabled tests are tagged with the Arc issue ids.

rajsesh · 2017-03-02T23:51:03Z

Frameworks/CoreGraphics/CGPath.mm

+
+    void SetIsFirstCall(bool firstCall) {
+        isFirstCall = firstCall;
+    }


According to the docs - it is not the "first call", rather the existence of an open sub path - that should be the mechanism used here.

DHowett-MSFT · 2017-03-07T01:21:56Z

Frameworks/CoreGraphics/CGPath.mm

    void BeginFigure() {
-        if (!geometrySink->IsFigureOpen()) {
+        if (geometrySink && !geometrySink->IsFigureOpen()) {


should this logic be in the above function?

DHowett-MSFT · 2017-03-07T01:22:10Z

Frameworks/CoreGraphics/CGPath.mm

@@ -482,6 +486,21 @@ static HRESULT _createPathReadyForFigure(CGPathRef previousPath,
    return S_OK;
 }

+static CGFloat __normalizeAngle(CGFloat originalAngle) {


A comment explaining what this one is doing would be exceptionally helpful.

rajsesh

Would be useful to add the manual tests to CGCatalog for the primary missing scenario (full circle).

DHowett-MSFT

You changed some constant values in the path drawing tests, but didn't update any test images. That worries me.

Additionally, I'm concerned as to why the ArcCircles_NoMove test changed so much...

DHowett-MSFT · 2017-03-07T18:46:57Z

Frameworks/CoreGraphics/CGPath.mm

@@ -567,6 +570,9 @@ void CGPathAddArcToPoint(

 /**
 @Status Interoperable
+ @Notes For the scenario of drawing a full circle, any distance of 2pi will result in a circle being drawn
+        regardless of the direction being drawn. The reference platform however, will not draw a circle


nit: , however, (missing preceding comma)

this is too wordy, and could be trimmed down to a succinct

All arcs from 0 to 2pi in either direction will result in a circle; the reference platform, by contrast, does not honor this behavior.

aballway · 2017-03-07T18:52:10Z

Frameworks/CoreGraphics/CGPath.mm

+            return 2 * M_PI;
+        }
+    }
+    if (returnAngle < 0) {


This case should never happen because you're setting it to 0 above if < .00001

aballway · 2017-03-07T18:52:39Z

Frameworks/CoreGraphics/CGPath.mm

+// the calculations for arcs since 0, 2pi, 4pi, etc... are all visually the same angle.
+static CGFloat __normalizeAngle(CGFloat originalAngle) {
+    CGFloat returnAngle = fmod(originalAngle, 2 * M_PI);
+    if (returnAngle < .00001) {


super nit: pull this out into some constant, c_zeroAngleThreshold or something

aballway · 2017-03-07T18:53:51Z

Frameworks/CoreGraphics/CGPath.mm

+        returnAngle = 0;
+    }
+    if (returnAngle == 0) {
+        if (abs(originalAngle) > 0) {


nit: this would be more clear as (originalAngle != 0) since you're taking the abs

aballway · 2017-03-07T18:56:18Z

Frameworks/CoreGraphics/CGPath.mm

-    newSink->AddArc(arcSegment);
+    // This will only happen when drawing a circle in the clockwise direction from 2pi to 0, a scenario
+    // supported on the reference platform.
+    if (abs(abs(rawDifference) - 2 * M_PI) < .00001) {


nit: rawDifference is already abs() from above

But could still be negative from a counterclockwise rotation. This negative is important later on but the strict value being close enough to zero here is the important factor for when to draw a full circle.

aballway · 2017-03-07T18:58:03Z

Frameworks/CoreGraphics/CGPath.mm

+    CGFloat normalizedStartAngle = __normalizeAngle(startAngle);
+    CGFloat normalizedEndAngle = __normalizeAngle(endAngle);
+
+    CGPoint startPoint = CGPointMake(x + radius * cos(normalizedStartAngle), y + radius * sin(normalizedStartAngle));


nit: you're calculating radius * sin, cos twice

They're on different angles.

😮 well by golly they are

ms-jihua · 2017-03-08T00:03:53Z

Frameworks/CoreGraphics/CGPath.mm

+        }
+    }
+    if (returnAngle < 0) {
+        returnAngle += M_PI * 2;


super super nit: consistent ordering with your (2*pi)s

ms-jihua · 2017-03-08T00:05:58Z

Frameworks/CoreGraphics/CGPath.mm

+    // This will only happen when drawing a circle in the clockwise direction from 2pi to 0, a scenario
+    // supported on the reference platform.
+    if (abs(abs(rawDifference) - 2 * M_PI) < .00001) {
+        CGFloat midPointAngle = normalizedStartAngle + rawDifference / 2;


nit: do we wanna just round the latter term to +/- pi here?

ms-jihua · 2017-03-08T00:06:38Z

Frameworks/CoreGraphics/CGPath.mm

+    CGFloat normalizedStartAngle = __normalizeAngle(startAngle);
+    CGFloat normalizedEndAngle = __normalizeAngle(endAngle);
+
+    CGPoint startPoint = CGPointMake(x + radius * cos(normalizedStartAngle), y + radius * sin(normalizedStartAngle));


nit: should this equation be an inline function?

DHowett-MSFT · 2017-03-08T01:19:42Z

Frameworks/CoreGraphics/CGPath.mm

+// This function will return a normalized angle in radians between 0 and 2pi. This is to standardize
+// the calculations for arcs since 0, 2pi, 4pi, etc... are all visually the same angle.
+static CGFloat __normalizeAngle(CGFloat originalAngle) {
+    CGFloat returnAngle = fmod(originalAngle, 2 * M_PI);


I'm not certain what this function is doing anymore.

Does it boil down to

return std::min(abs(angle), 2 * M_PI);

?

Not quite, since that would ignore any angle like 7pi/2 and instead return 2 *M_PI. We only want to return an angle of 2pi if the angle is a multiple of 2pi and not 0. That's what the extra checks are for.

msft-Jeyaram · 2017-03-08T19:50:08Z

tests/UnitTests/CoreGraphics.drawing/CGContextDrawing_ClearRectTests.cpp

@@ -141,10 +141,8 @@ DRAW_TEST_P(CGClearRectArc, ClearArc) {
 }

 static CGPoint sweep[] = { CGPointMake(0, M_PI), CGPointMake(M_PI, 0), CGPointMake(0.3 * M_PI, 0.6 * M_PI), CGPointMake(0, 1.9 * M_PI) };
-// TODO: enable when #2062 is fixed.
-INSTANTIATE_TEST_CASE_P(DISABLED_CGContextTests,


msftclas added the cla-not-required label Mar 2, 2017

MSFTFox requested a review from DHowett-MSFT March 2, 2017 01:00

DHowett-MSFT reviewed Mar 2, 2017

View reviewed changes

DHowett-MSFT suggested changes Mar 2, 2017

View reviewed changes

msft-Jeyaram reviewed Mar 2, 2017

View reviewed changes

tests/unittests/CoreGraphics.Drawing/CGPathDrawingTests.cpp Outdated

break;

}

if (moveToPoint == 0) {

}

Copy link

Contributor

msft-Jeyaram Mar 2, 2017

Choose a reason for hiding this comment

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

? needed?

rajsesh suggested changes Mar 2, 2017

View reviewed changes

DHowett-MSFT mentioned this pull request Mar 6, 2017

Release 45 regressions: #2161

Closed

MSFTFox added 4 commits March 6, 2017 16:48

Fix CGPathAddArc and add additional tests.

9ab4024

Add New Test Images

555f462

Updating arc drawing test's reference images.

64a8864

Use figure tracking for first arc call.

c097251

DHowett-MSFT approved these changes Mar 7, 2017

View reviewed changes

rajsesh approved these changes Mar 7, 2017

View reviewed changes

MSFTFox added 2 commits March 6, 2017 22:17

Fix default cases for Arcs.

c352974

AddLines Fix

8d209ab

MSFTFox force-pushed the henryfox/fixarc branch from 25d3349 to 8d209ab Compare March 7, 2017 06:57

DHowett-MSFT suggested changes Mar 7, 2017

View reviewed changes

aballway reviewed Mar 7, 2017

View reviewed changes

Fixing test failures introduced by test changes.

eecc5e2

ms-jihua reviewed Mar 8, 2017

View reviewed changes

Remove unreachable cases and streamline code a bit.

e31b035

ms-jihua reviewed Mar 8, 2017

View reviewed changes

Address CR Feedback.

8607169

DHowett-MSFT reviewed Mar 8, 2017

View reviewed changes

msft-Jeyaram reviewed Mar 8, 2017

View reviewed changes

Replace Arc Images

950cbc4

DHowett-MSFT approved these changes Mar 9, 2017

View reviewed changes

MSFTFox merged commit a974bc3 into microsoft:develop Mar 9, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix CGPathAddArc and add additional tests. #2136

Fix CGPathAddArc and add additional tests. #2136

MSFTFox commented Mar 2, 2017

DHowett-MSFT Mar 2, 2017

rajsesh Mar 2, 2017

DHowett-MSFT left a comment

DHowett-MSFT commented Mar 2, 2017

msft-Jeyaram Mar 2, 2017

msft-Jeyaram commented Mar 2, 2017 •

edited

Loading

msft-Jeyaram commented Mar 2, 2017

rajsesh Mar 2, 2017

DHowett-MSFT Mar 7, 2017

DHowett-MSFT Mar 7, 2017

rajsesh left a comment

DHowett-MSFT left a comment

DHowett-MSFT Mar 7, 2017

DHowett-MSFT Mar 7, 2017

aballway Mar 7, 2017

aballway Mar 7, 2017

aballway Mar 7, 2017

aballway Mar 7, 2017

MSFTFox Mar 7, 2017

aballway Mar 7, 2017

MSFTFox Mar 7, 2017

aballway Mar 7, 2017

ms-jihua Mar 8, 2017

ms-jihua Mar 8, 2017

ms-jihua Mar 8, 2017

DHowett-MSFT Mar 8, 2017

MSFTFox Mar 8, 2017

msft-Jeyaram Mar 8, 2017

MSFTFox Mar 8, 2017

Fix CGPathAddArc and add additional tests. #2136

Fix CGPathAddArc and add additional tests. #2136

Conversation

MSFTFox commented Mar 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett-MSFT left a comment

Choose a reason for hiding this comment

DHowett-MSFT commented Mar 2, 2017

Choose a reason for hiding this comment

msft-Jeyaram commented Mar 2, 2017 • edited Loading

msft-Jeyaram commented Mar 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajsesh left a comment

Choose a reason for hiding this comment

DHowett-MSFT left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msft-Jeyaram commented Mar 2, 2017 •

edited

Loading