Implemented CGPathApply. by adein · Pull Request #559 · microsoft/WinObjC

adein · 2016-06-14T22:09:09Z

Replaced pathComponent with CGPathElement. Rewrote CGPathAddEllipseInRect,
CGPathAddArc, CGPathAddArcToPoint to support CGPathElement.
Implemented CGPathGetCurrentPoint for use in CGPathAddArcToPoint.
Removed caveats for CGPathAddArc, CGPathAddEllipseInRect.
Caveat for CGPathAddArcToPoint remains.

Fix #525 : CGPathApply.

msftclas · 2016-06-14T22:09:12Z

Hi @adein, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

rajsesh · 2016-06-14T23:39:30Z

@bdrlamb-ms is added to the review. #Closed

rajsesh · 2016-06-15T16:39:57Z

Are there tests for these apis that need to be part of this pull request? #Closed

adein · 2016-06-16T15:38:53Z

@rajsesh-msft Are you requesting that unit tests be added to the pull request, or for details on how we tested the changes? We tested the changes using a sample project; we have not created unit tests in the bridge.

rajsesh · 2016-06-16T17:31:25Z

@adein, yes please. If you added a sample to woccatalog, please include that as well. the idea is to make sure we have test collateral to guard against any future regressions.

bbowman · 2016-06-20T17:39:43Z


 __CGPath::~__CGPath() {
    if (_components) {
+		for (unsigned i = 0; i < _count; i++) {


nit: please use spaces instead of tabs to match the rest of the codebase. To that end, we have setup clang-format for the project and ask that all PRs have it run prior to submission so that we can avoid silly formatting comments like this. :-)

rajsesh · 2016-06-24T22:11:33Z

@jeffMeador could you please rebase the branch and push again?

jeffMeador · 2016-06-24T22:31:48Z

@rajsesh-msft This branch now contains the test project & unit tests for CGPathApply.

The unit tests are in tests/unittests/CoreGraphics/CGPathTests.mm
The sample code is in samples/CGCatalog

rajsesh · 2016-06-24T22:36:30Z

Could you pull latest changes from github/master your branch, do a git -rebase and resubmit the pull request? The way it stands now, the PR shows 188 changed files and is hard to go through.

rajsesh · 2016-06-24T22:43:30Z

+void cgPathModifyingApplierFunction(void* info, const CGPathElement* element) {
+    int pointCount = cgPathPointCountForElementType(element->type);
+
+    NSMutableArray* points = [NSMutableArray array];


element->points? otherwise this seems unused...

Are you referring to the points NSMutableArray not being used? It's not used, I can remove it. cgPathModifyingApplierFunction just exists to show that paths cannot be manipulated in an applier function.

jeffMeador · 2016-06-27T19:50:33Z

@rajsesh-msft This branch has been properly rebased and now only shows the 5 files that have been edited.

rajsesh · 2016-06-27T19:51:18Z

Thanks a bunch, this is a lot easier to look at.

In reply to: 228855101 [](ancestors = 228855101)

rajsesh · 2016-06-27T21:51:32Z

+    if (path == NULL) {
+        return;
+    }
+


Nit: it looks like CGPathApply is the only one that is doing the null check. fail fast (by not checking NULL) will make it consistent.

rajsesh · 2016-06-28T00:37:59Z

It would help to add some comments about the math in all the new methods.

In reply to: 228855305 [](ancestors = 228855305,228855101)

rajsesh · 2016-06-28T00:38:04Z

🕐

bbowman · 2016-06-28T01:06:43Z

+
+    dx0 = x0 - x1;
+    dy0 = y0 - y1;
+    xl0 = sqrt(dx0 * dx0 + dy0 * dy0);


Would be nice to use more descriptive names across the board here. dx0 could be deltaX0 and x10 could be distance10. I know its common to be extra terse in general math but more descriptive names aids the casual reader in remembering what the components all are when the code is longer than a single screen etc.

Actually would be nice to make dx0 deltaX10. I was confused why there was no dx1 / dy1 but that is because a single point was chosen for the deltas.

In reply to: 68683240 [](ancestors = 68683240)

Replaced pathComponent with CGPathElement. Rewrote CGPathAddEllipseInRect, CGPathAddArc, CGPathAddArcToPoint to support CGPathElement. Implemented CGPathGetCurrentPoint for use in CGPathAddArcToPoint. Removed caveats for CGPathAddArc, CGPathAddEllipseInRect. Caveat for CGPathAddArcToPoint remains.

Used Visual Studio ClangFormat plugin to format CGPath.mm, CGPath.h, and CGPathInternal.h.

Switched text editor setting to use spaces instead of tabs. Re-ran ClangFormat on CGPath.mm CGPathInternal.h and CGPath.h.

Ran untabify and re-ran ClangFormat on CGPath.mm, CGPathInternal.h and CGPath.h.

Added CGPathApply unit tests. Added a test to ensure paths cannot be manipulated in the applier function and multiple tests for creating different types of paths and checking the resulting elements. Multiple bug fixes in CGPath. Re-ran formatter on all files.

Removed unused array in cgPathModifyingApplierFunction.

CGPathApply no longer prevents the maniuplation of points. Added _CGPathAddElement utility function to handle increasing size of elements array and creating point arrays. Point arrays are all now the max size, 3 points. Point arrays are copied in CGPathCreateMutableCopy to new path. CGPathEqualToPath implemented so path copies could be tested. Comments have been added for new math. CGPathAddArcToPoint marked as a todo as it needs to be refactored here, CGContextImpl::CGContextAddArcToPoint and CGContextCairo::CGContextAddArcToPoint. Fixed implementation of CGPathCreateWithEllipseInRect. Added points copy in CGPathAddPath and added unit test for CGPathAddPath.

jeffMeador · 2016-06-28T21:46:41Z

@rajsesh-msft everything we discussed should be addressed in 184e31f . CGPathAddArcToPoint was left untouched with a note that it needs to be refactored along with the copies in the other spots.

rajsesh · 2016-06-28T21:48:02Z

+        path->_elements = (CGPathElement*)IwRealloc(path->_elements, path->_max * sizeof(CGPathElement));
+    }
+    CGPathElement* element = &path->_elements[path->_count];
+    element->points = (CGPoint*)IwCalloc(kCGPathMaxPointCount, sizeof(CGPoint));


This is not what i meant. We are still allocating memory every time a path element is added. perhaps i misunderstood your earlier question. Do you want to chat over the phone so we can get on the same page here? I really want to avoid duplicate allocations for points every time an item is added to the path. Allocating a fixed number of points without doing it once isn't really buying us anything. #Closed

CGPath now uses CGPathElementInternal when creating an array of elements. CGPathElementInternal holds a fixed sized array big enough for the maximum number of points in an element. CGPathElementInternal inherits from CGPathElement and adjusts the points pointer to point at its internal points array as much as possible. Calls to updatePointsArrayPointer() need to be done manually when allocating CGPathElementInternal arrays dynamically or using memcpy.

Added call to base struct constructor in CGPathElementInternal

jeffMeador · 2016-06-29T13:15:54Z

@rajsesh-msft CGPath has been updated to use CGPathElementInternal and just have 1 allocation for an array of elements instead of an allocation for an array of elements & for each array of points.

rajsesh · 2016-06-29T16:29:29Z

+    CGPathElementInternal* element = &path->_elements[path->_count];
+    // The new elements needs its points array pointer updated
+    // because it was created with alloc
+    element->updatePointsArrayPointer();


nit: from a OO standpoint, i would call this function init() (caller doesn't need to know internal details of what this function does). There is alternately, no need for this function, you can call placement new on the memory location like this: CGPathElementInternal* element = new (&path->_elements[path->_count]) CGPathElementInternal(); clearly, this doesnt work for the mutable copy scenario below. so it is okay to leave as is.

rajsesh · 2016-06-29T16:44:22Z

Added a few minor comments to the new changes.

In reply to: 229352074 [](ancestors = 229352074)

Switched to point references in CGPathEqualToPath and element references in CGPathAddPath to avoid copying. Renamed CGPathElementInternal.updatePointsArrayPointer() to CGPathElementInternal.init()

jeffMeador · 2016-06-29T18:35:30Z

The changes requested have been committed.

rajsesh · 2016-06-29T19:37:14Z

In CI build. validated internal apps manually and they look good so far.

rajsesh · 2016-06-29T20:42:31Z

CI build passed.

msftclas added the cla-already-signed label Jun 14, 2016

bbowman reviewed Jun 20, 2016
View reviewed changes

rajsesh reviewed Jun 24, 2016
View reviewed changes

jeffMeador force-pushed the feature/CGPathApply branch from df76d05 to a1e40a3 Compare June 27, 2016 18:43

rajsesh reviewed Jun 27, 2016
View reviewed changes

bbowman reviewed Jun 28, 2016
View reviewed changes

jeffMeador added 2 commits June 28, 2016 17:32

Ran ClangFormat on files in branch.

b1bf4f4

Used Visual Studio ClangFormat plugin to format CGPath.mm, CGPath.h, and CGPathInternal.h.

jeffMeador added 5 commits June 28, 2016 17:32

Converted tabs to spaces in branch.

586c64b

Switched text editor setting to use spaces instead of tabs. Re-ran ClangFormat on CGPath.mm CGPathInternal.h and CGPath.h.

Ran untabify on files in branch.

c2590c3

Ran untabify and re-ran ClangFormat on CGPath.mm, CGPathInternal.h and CGPath.h.

Removing unused code in CGPathTests.mm

93fd110

Removed unused array in cgPathModifyingApplierFunction.

jeffMeador force-pushed the feature/CGPathApply branch from 0745299 to 184e31f Compare June 28, 2016 21:33

rajsesh reviewed Jun 28, 2016
View reviewed changes

jeffMeador added 2 commits June 29, 2016 08:40

Fixed constructor in CGPathElementInternal

13d52a8

Added call to base struct constructor in CGPathElementInternal

rajsesh reviewed Jun 29, 2016
View reviewed changes

Reduced copies in CGPath

52c5948

Switched to point references in CGPathEqualToPath and element references in CGPathAddPath to avoid copying. Renamed CGPathElementInternal.updatePointsArrayPointer() to CGPathElementInternal.init()

rajsesh merged commit e44b7b5 into microsoft:master Jun 29, 2016

adein deleted the feature/CGPathApply branch June 30, 2016 13:40

Conversation

adein commented Jun 14, 2016

Uh oh!

msftclas commented Jun 14, 2016

Uh oh!

rajsesh commented Jun 14, 2016

Uh oh!

rajsesh commented Jun 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adein commented Jun 16, 2016

Uh oh!

rajsesh commented Jun 16, 2016

Uh oh!

bbowman Jun 20, 2016

Choose a reason for hiding this comment

Uh oh!

rajsesh commented Jun 24, 2016

Uh oh!

jeffMeador commented Jun 24, 2016

Uh oh!

rajsesh commented Jun 24, 2016

Uh oh!

rajsesh Jun 24, 2016

Choose a reason for hiding this comment

Uh oh!

jeffMeador Jun 27, 2016

Choose a reason for hiding this comment

Uh oh!

jeffMeador commented Jun 27, 2016

Uh oh!

rajsesh commented Jun 27, 2016

Uh oh!

rajsesh Jun 27, 2016

Choose a reason for hiding this comment

Uh oh!

rajsesh commented Jun 28, 2016

Uh oh!

rajsesh commented Jun 28, 2016

Uh oh!

bbowman Jun 28, 2016

Choose a reason for hiding this comment

Uh oh!

bbowman Jun 28, 2016

Choose a reason for hiding this comment

Uh oh!

jeffMeador commented Jun 28, 2016

Uh oh!

rajsesh Jun 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeffMeador commented Jun 29, 2016

Uh oh!

rajsesh Jun 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rajsesh commented Jun 29, 2016

Uh oh!

jeffMeador commented Jun 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rajsesh commented Jun 29, 2016

Uh oh!

rajsesh commented Jun 29, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

rajsesh commented Jun 15, 2016 •

edited

Loading

rajsesh Jun 28, 2016 •

edited

Loading

rajsesh Jun 29, 2016 •

edited

Loading

jeffMeador commented Jun 29, 2016 •

edited

Loading