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

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 1 commit
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
5 changes: 3 additions & 2 deletions 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 @@ -451,8 +453,7 @@
if (_renderTarget == nullptr) {
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));
ComPtr<ID2D1Factory> d2dFactory = _GetD2DFactoryInstance();
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
4 changes: 3 additions & 1 deletion 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,7 +1647,8 @@ void CGContextFillRects(CGContextRef context, const CGRect* rects, size_t count)
*/
void CGContextDrawPath(CGContextRef context, CGPathDrawingMode mode) {
NOISY_RETURN_IF_NULL(context);
UNIMPLEMENTED();

__CGContextDrawGeometry(context, _getAndClosePathGeometry(context->Path()), mode);
Copy link

@DHowett-MSFT DHowett-MSFT Nov 2, 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.

_getAndClosePathGeometry [](start = 37, length = 24)

if(!HasPath)
// we can abort

Otherwise, Path() will create an empty path just to draw it #Resolved

Copy link

@DHowett-MSFT DHowett-MSFT Nov 2, 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.

__CGContextDrawGeometry [](start = 4, length = 23)

We may need to invert the user transform on this draw, because otherwise we could apply a double-transformed path. #WontFix

Copy link
Member Author

Choose a reason for hiding this comment

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

I made a note of this. We'll explore this more with the affine transform work item.

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

context->ClearPath();
}

Expand Down
5 changes: 3 additions & 2 deletions 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 @@ -155,8 +157,7 @@
if (_renderTarget == nullptr) {
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));
ComPtr<ID2D1Factory> d2dFactory = _GetD2DFactoryInstance();
Copy link

@DHowett-MSFT DHowett-MSFT Nov 2, 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.

_GetD2DFactoryInstance [](start = 42, length = 22)

This might be going cross-module soon; perhaps it should be named _CGGetD2DFactory(), and have the signature

HRESULT _CGGetD2DFactory(ID2D1Factory** pFactory);
``` #Resolved

ComPtr<ID2D1RenderTarget> renderTarget;
THROW_IF_FAILED(d2dFactory->CreateWicBitmapRenderTarget(wicBitmap.Get(), D2D1::RenderTargetProperties(), &renderTarget));
_renderTarget = renderTarget.Detach();
Expand Down
11 changes: 10 additions & 1 deletion 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 @@ -144,6 +145,14 @@ HRESULT InitializeGeometries() {
return S_OK;
}
};
ID2D1Geometry* _getAndClosePathGeometry(CGPathRef path) {
if (path) {
path->ClosePath();
return path->GetPathGeometry().Get();
}

return nullptr;
}

CFTypeID CGPathGetTypeID() {
return __CGPath::GetTypeID();
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;

ID2D1Geometry* _getAndClosePathGeometry(CGPathRef path);
Copy link
Contributor

@msft-Jeyaram msft-Jeyaram Nov 2, 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.

_getAndClosePathGeometry [](start = 15, length = 24)

extra nit: _CGPathGetAndClosePathGeometry(...) ? #Resolved

Copy link

Choose a reason for hiding this comment

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

please something more like:

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

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


#endif