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

Jump to bottom

Implement drawing CGPaths through CGContext #1307

Merged
merged 5 commits into from
Nov 5, 2016
Merged

Implement drawing CGPaths through CGContext #1307

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d43b075
Implement drawing CGPaths through CGContext
MSFTFox Oct 26, 2016
5f79b50
Fix calls to return an HRESULT.
MSFTFox Nov 3, 2016
5eab616
Address CR Feedback.
MSFTFox Nov 4, 2016
24f5707
Fix poor comptr usage and leaks. Return to static initializer model.
MSFTFox Nov 4, 2016
34bd6d5
More macros.
MSFTFox Nov 4, 2016
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
4 changes: 3 additions & 1 deletion Frameworks/CoreGraphics/CGBitmapImage.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@

#import "LoggingNative.h"

#import "D2DWrapper.h"

#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Wdeprecated-register"

Expand Down Expand Up @@ -452,7 +454,7 @@
BYTE* imageData = static_cast<BYTE*>(LockImageData());
ComPtr<IWICBitmap> wicBitmap = Make<CGIWICBitmap>(this, SurfaceFormat());
ComPtr<ID2D1Factory> d2dFactory;
THROW_IF_FAILED(D2D1CreateFactory(D2D1_FACTORY_TYPE_SINGLE_THREADED, __uuidof(ID2D1Factory), &d2dFactory));
FAIL_FAST_IF_FAILED(_CGGetD2DFactory(&d2dFactory));
Copy link
Contributor

Choose a reason for hiding this comment

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

Please no, THROW_IF_FAILED() is marginally better. FAIL_FAST is essentially abort, and as framework we should seldom be using it. And in other places too, per our offline discussion.

ComPtr<ID2D1RenderTarget> renderTarget;
THROW_IF_FAILED(d2dFactory->CreateWicBitmapRenderTarget(wicBitmap.Get(), D2D1::RenderTargetProperties(), &renderTarget));
Copy link
Member

@bbowman bbowman Nov 4, 2016 •
edited by MSFTFox
Loading

Choose a reason for hiding this comment

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

:-( super sad at the inconsistencies here. I thought the decision was to be error returning out to the public layer at which point a decision would be made on the correct course of action. This still seems internal as it returns a D2D object.

Additionally, returning a Com object with a raw * usually speaks to a problematic design pattern. This is because it requires the "unjacketing" of the ComObject to return it to the caller without a clear communication on transfer/sharing of ownership which is by definition occuring by handing it out. #Resolved

Copy link

Choose a reason for hiding this comment

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

The render target generators in CGImage and friends are going away (and are only being updated as necessary as part of this PR); ideally, yes, we'd fix everything all at once. There's little point when it's gonna be turfed. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

This code has gone away, i'll be sending out the PR soon.
Actually this whole file is gone.

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

Copy link
Member Author

Choose a reason for hiding this comment

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

We should sync up.

In reply to: 86614591 [](ancestors = 86614591,86612430)

_renderTarget = renderTarget.Detach();
Expand Down
9 changes: 7 additions & 2 deletions Frameworks/CoreGraphics/CGContext.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
#import <CoreGraphics/CGGradient.h>
#import "CGColorSpaceInternal.h"
#import "CGContextInternal.h"
#import "CGPathInternal.h"

#import <CFCppBase.h>

Expand Down Expand Up @@ -1646,8 +1647,12 @@ void CGContextFillRects(CGContextRef context, const CGRect* rects, size_t count)
*/
void CGContextDrawPath(CGContextRef context, CGPathDrawingMode mode) {
NOISY_RETURN_IF_NULL(context);
UNIMPLEMENTED();
context->ClearPath();
if (context->HasPath()) {
ComPtr<ID2D1Geometry> pGeometry;
FAIL_FAST_IF_FAILED(_CGPathGetGeometry(context->Path(), &pGeometry));
FAIL_FAST_IF_FAILED(__CGContextDrawGeometry(context, pGeometry.Get(), mode));
context->ClearPath();
Copy link
Contributor

@aballway aballway Nov 4, 2016 •
edited by MSFTFox
Loading

Choose a reason for hiding this comment

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

should do something with the HRESULT here #Resolved

}
}

/**
Expand Down
4 changes: 3 additions & 1 deletion Frameworks/CoreGraphics/CGGraphicBufferImage.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@
#import "LoggingNative.h"
#import <CGGraphicBufferImage.h>

#import "D2DWrapper.h"

#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Wdeprecated-register"

Expand Down Expand Up @@ -156,7 +158,7 @@
BYTE* imageData = static_cast<BYTE*>(LockImageData());
ComPtr<IWICBitmap> wicBitmap = Make<CGIWICBitmap>(this, SurfaceFormat());
ComPtr<ID2D1Factory> d2dFactory;
THROW_IF_FAILED(D2D1CreateFactory(D2D1_FACTORY_TYPE_SINGLE_THREADED, __uuidof(ID2D1Factory), &d2dFactory));
FAIL_FAST_IF_FAILED(_CGGetD2DFactory(&d2dFactory));
ComPtr<ID2D1RenderTarget> renderTarget;
THROW_IF_FAILED(d2dFactory->CreateWicBitmapRenderTarget(wicBitmap.Get(), D2D1::RenderTargetProperties(), &renderTarget));
_renderTarget = renderTarget.Detach();
Expand Down
38 changes: 26 additions & 12 deletions Frameworks/CoreGraphics/CGPath.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,12 @@
#import <CoreFoundation/CoreFoundation.h>
#import <CFRuntime.h>
#import "D2DWrapper.h"
#import "CGPathInternal.h"

#include <COMIncludes.h>
#import <wrl/client.h>
#import <D2d1.h>
#include <COMIncludes_End.h>

#import <CFCPPBase.h>

static const wchar_t* TAG = L"CGPath";
Expand Down Expand Up @@ -91,7 +92,8 @@ void SetStartingPoint(CGPoint newPoint) {
HRESULT PreparePathForEditing() {
if (!_impl.geometrySink) {
// Re-open this geometry.
ComPtr<ID2D1Factory> factory = _GetD2DFactoryInstance();
ComPtr<ID2D1Factory> factory;
RETURN_IF_FAILED(_CGGetD2DFactory(&factory));

// Create temp vars for new path/sink
ComPtr<ID2D1PathGeometry> newPath;
Expand All @@ -113,12 +115,13 @@ HRESULT PreparePathForEditing() {
return S_OK;
}

void ClosePath() {
HRESULT ClosePath() {
if (_impl.geometrySink) {
EndFigure(D2D1_FIGURE_END_OPEN);
Copy link

@DHowett-MSFT DHowett-MSFT Nov 4, 2016 •
edited by MSFTFox
Loading

Choose a reason for hiding this comment

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

Can this fail? #ByDesign

Copy link
Member Author

Choose a reason for hiding this comment

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

virtual void EndFigure(
D2D1_FIGURE_END figureEnd
) = 0;

It cannot. Calling it multiple times can result in a bad state though which makes this API slightly weird since it doesn't report that itself.

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

_impl.geometrySink->Close();
RETURN_IF_FAILED(_impl.geometrySink->Close());
_impl.geometrySink = nullptr;
}
return S_OK;
}

void BeginFigure() {
Expand All @@ -136,7 +139,8 @@ void EndFigure(D2D1_FIGURE_END figureStatus) {
}

HRESULT InitializeGeometries() {
ComPtr<ID2D1Factory> factory = _GetD2DFactoryInstance();
ComPtr<ID2D1Factory> factory;
RETURN_IF_FAILED(_CGGetD2DFactory(&factory));

RETURN_IF_FAILED(factory->CreatePathGeometry(&_impl.pathGeometry));
RETURN_IF_FAILED(_impl.pathGeometry->Open(&_impl.geometrySink));
Expand All @@ -145,6 +149,14 @@ HRESULT InitializeGeometries() {
}
};

HRESULT _CGPathGetGeometry(CGPathRef path, ID2D1Geometry** pGeometry) {
Copy link
Contributor

@msft-Jeyaram msft-Jeyaram Nov 4, 2016 •
edited by MSFTFox
Loading

Choose a reason for hiding this comment

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

pGeometry [](start = 59, length = 9)

if pGeometry is invalid, we end up returning S_OK.
adding a check for pGeoMetry and path should help.

RETURN_HR_IF_NULL(E_POINTER,pGeometry);
RETURN_HR_IF_NULL(E_POINTER,path); #Resolved

RETURN_HR_IF_NULL(E_POINTER, pGeometry);
RETURN_HR_IF_NULL(E_POINTER, path);
RETURN_IF_FAILED(path->ClosePath());
path->GetPathGeometry().CopyTo(pGeometry);
return S_OK;
}

CFTypeID CGPathGetTypeID() {
return __CGPath::GetTypeID();
}
Expand All @@ -159,8 +171,8 @@ static Boolean __CGPathEqual(CFTypeRef cf1, CFTypeRef cf2) {
__CGPath* path1 = (__CGPath*)cf1;
__CGPath* path2 = (__CGPath*)cf2;

path1->ClosePath();
path2->ClosePath();
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.
Expand Down Expand Up @@ -212,7 +224,7 @@ CGMutablePathRef CGPathCreateMutableCopy(CGPathRef path) {
// In order to call stream and copy the contents of the original path into the
// new copy we must close this path.
// Otherwise the D2D calls will return that a bad state has been entered.
path->ClosePath();
FAIL_FAST_IF_FAILED(path->ClosePath());

FAIL_FAST_IF_FAILED(path->GetPathGeometry()->Stream(mutableRet->GetGeometrySink().Get()));

Expand Down Expand Up @@ -451,7 +463,7 @@ void CGPathAddPath(CGMutablePathRef path, const CGAffineTransform* transform, CG
RETURN_IF(!path || !toAdd);

// Close the path we're adding and open the path we need to add to.
toAdd->ClosePath();
FAIL_FAST_IF_FAILED(toAdd->ClosePath());
FAIL_FAST_IF_FAILED(path->PreparePathForEditing());
FAIL_FAST_IF_FAILED(toAdd->GetPathGeometry()->Stream(path->GetGeometrySink().Get()));
}
Expand Down Expand Up @@ -510,7 +522,9 @@ CGRect CGPathGetBoundingBox(CGPathRef path) {

D2D1_RECT_F bounds;

path->ClosePath();
if (FAILED(path->ClosePath())) {
return CGRectNull;
}

if (FAILED(path->GetPathGeometry()->GetBounds(D2D1::IdentityMatrix(), &bounds))) {
return CGRectNull;
Expand All @@ -530,7 +544,7 @@ bool CGPathIsEmpty(CGPathRef path) {

UINT32 count;

path->ClosePath();
RETURN_FALSE_IF_FAILED(path->ClosePath());

RETURN_FALSE_IF_FAILED(path->GetPathGeometry()->GetFigureCount(&count));
return count == 0;
Expand Down Expand Up @@ -673,7 +687,7 @@ bool CGPathContainsPoint(CGPathRef path, const CGAffineTransform* transform, CGP

BOOL containsPoint = FALSE;

path->ClosePath();
RETURN_FALSE_IF_FAILED(path->ClosePath());
RETURN_FALSE_IF_FAILED(path->GetPathGeometry()->FillContainsPoint(_CGPointToD2D_F(point), D2D1::IdentityMatrix(), &containsPoint));

return (containsPoint ? true : false);
Expand Down
4 changes: 2 additions & 2 deletions Frameworks/CoreGraphics/D2DWrapper.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
#import <CoreGraphics/CGGeometry.h>
#import <CoreGraphics/CGBase.h>

Microsoft::WRL::ComPtr<ID2D1Factory> _GetD2DFactoryInstance();
HRESULT _CGGetD2DFactory(ID2D1Factory** factory);

Microsoft::WRL::ComPtr<IWICImagingFactory> _GetWICFactory();

Expand All @@ -40,4 +40,4 @@ inline CGRect _D2DRectToCGRect(D2D1_RECT_F rect) {
CGFloat height = rect.bottom - y;

return CGRectMake(x, y, width, height);
}
}
14 changes: 5 additions & 9 deletions Frameworks/CoreGraphics/D2DWrapper.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,16 +19,12 @@

using namespace Microsoft::WRL;

// Private helper for creating a D2DFactory
static ComPtr<ID2D1Factory> __createD2DFactory() {
ComPtr<ID2D1Factory> d2dFactory;
FAIL_FAST_IF_FAILED(D2D1CreateFactory(D2D1_FACTORY_TYPE_SINGLE_THREADED, __uuidof(ID2D1Factory), &d2dFactory));
return d2dFactory;
}
HRESULT _CGGetD2DFactory(ID2D1Factory** factory) {
static ComPtr<ID2D1Factory> sFactory;
static HRESULT sHr = D2D1CreateFactory(D2D1_FACTORY_TYPE_SINGLE_THREADED, __uuidof(ID2D1Factory), &sFactory);

Copy link

Choose a reason for hiding this comment

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

If one static referring to another as part of its initialization is safe, then this should be totally fine.

ComPtr<ID2D1Factory> _GetD2DFactoryInstance() {
static ComPtr<ID2D1Factory> factory = __createD2DFactory();
return factory;
sFactory.CopyTo(factory);
RETURN_HR(sHr);
Copy link

Choose a reason for hiding this comment

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

RETURN_HR [](start = 4, length = 9)

return sHR?

Copy link

Choose a reason for hiding this comment

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

just a nit.

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Figured this was better from result.h

"// Always returns a known result (HRESULT) - logs failures in pre-release
#define RETURN_HR(hr) __RETURN_HR(wil::verify_hresult(hr), #hr)"

In reply to: 86633680 [](ancestors = 86633680,86633664)

}

static ComPtr<IWICImagingFactory> __createWICFactory() {
Expand Down
6 changes: 6 additions & 0 deletions Frameworks/include/CGPathInternal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,10 @@
#include "CoreGraphics/CGContext.h"
#include "CoreGraphics/CGPath.h"

#include <COMIncludes.h>
#include <d2d1.h>
#include <COMIncludes_End.h>

const int kCGPathMaxPointCount = 3;

struct CGPathElementInternal : CGPathElement {
Expand Down Expand Up @@ -51,4 +55,6 @@ struct CGPathElementInternal : CGPathElement {
};
typedef struct CGPathElementInternal CGPathElementInternal;

HRESULT _CGPathGetGeometry(CGPathRef path, ID2D1Geometry** pGeometry);

#endif
4 changes: 2 additions & 2 deletions tests/unittests/CoreGraphics/CGPathTests.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -412,7 +412,7 @@ CGPathRef newPathForRoundRect(CGRect rect, CGFloat radius) {
return path;
}

TEST(CGPath, CGPathContainsPointOutsideRect) {
DISABLED_TEST(CGPath, CGPathContainsPointOutsideRect) {
Copy link
Member Author

Choose a reason for hiding this comment

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

DISABLED_TEST [](start = 0, length = 13)

These tests should have been disabled initially since they involve work with arcs/curves. These will be re-enabled then.

CGFloat originX = 10.0f;
CGFloat originY = 20.0f;
CGFloat pathWidth = 100.0f;
Expand All @@ -438,7 +438,7 @@ CGPathRef newPathForRoundRect(CGRect rect, CGFloat radius) {
EXPECT_FALSE(test);
}

TEST(CGPath, CGPathContainsPointInsideRectOutsidePath) {
DISABLED_TEST(CGPath, CGPathContainsPointInsideRectOutsidePath) {
CGFloat originX = 10.0f;
CGFloat originY = 20.0f;
CGFloat pathWidth = 100.0f;
Expand Down