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

Add support for colored and non-colored Patterns #1952

Merged
merged 8 commits into from
Feb 16, 2017
Merged

Add support for colored and non-colored Patterns #1952

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
2a06c45
Add support for colored and non-colored Patterns
msft-Jeyaram Feb 13, 2017
5cbe655
updating tests and making old tests parm
msft-Jeyaram Feb 15, 2017
adcef78
disabling some tests
msft-Jeyaram Feb 15, 2017
58dbe68
adding the rotational test
msft-Jeyaram Feb 15, 2017
95821be
removal of failure reference images
msft-Jeyaram Feb 15, 2017
0d94658
cr feedback
msft-Jeyaram Feb 15, 2017
1d2120b
s
msft-Jeyaram Feb 15, 2017
20dad65
updating bitmap format type
msft-Jeyaram Feb 15, 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
41 changes: 31 additions & 10 deletions Frameworks/CoreGraphics/CGContext.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1770,9 +1770,10 @@ static CGImageRef __CGContextCreateRenderableImage(CGImageRef image) {
#pragma region Drawing Parameters - Stroke / Fill Patterns

template <typename ContextStageLambda> // Takes the form HRESULT(*)(CGContextRef)
Copy link
Contributor

@aballway aballway Feb 15, 2017 •
edited by msft-Jeyaram
Loading

Choose a reason for hiding this comment

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

nit: update the comment w/ new params #Resolved

static HRESULT _CreatePatternBrush(
CGContextRef context, CGPatternRef pattern, const CGFloat* components, ID2D1BitmapBrush1** brush, ContextStageLambda&& contextStage) {
// TODO #1592: change to support grayscale (masks) after dustins change.
static HRESULT _CreatePatternBrush(CGContextRef context,
CGPatternRef pattern,
ID2D1BitmapBrush1** brush,
ContextStageLambda&& contextStage) {
Copy link
Contributor

@aballway aballway Feb 15, 2017 •
edited by msft-Jeyaram
Loading

Choose a reason for hiding this comment

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

nit: make the out param the last one #WontFix

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's much nicer to have the lambda last in my opinion.

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

woc::unique_cf<CGColorSpaceRef> colorspace{ CGColorSpaceCreateDeviceRGB() };

// We need to generate the pattern as an image (then tile it)
Expand All @@ -1781,20 +1782,23 @@ static HRESULT _CreatePatternBrush(

size_t bitsPerComponent = 8;
size_t bytesPerRow = 4 * tileSize.size.width;
CGBitmapInfo bitmapInfo = kCGImageAlphaPremultipliedFirst | kCGBitmapByteOrder32Big;
CGBitmapInfo bitmapInfo = kCGImageAlphaPremultipliedFirst | kCGBitmapByteOrderDefault;
Copy link
Contributor

Choose a reason for hiding this comment

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

How does this work? We should not be using alphafirst formats since that is not natively supported by d2d.

Copy link

Choose a reason for hiding this comment

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

@rajsesh-msft alpha first + default = BGRA 😸

Copy link

Choose a reason for hiding this comment

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

However, @msft-Jeyaram: If you want to avoid unnecessary bitmap copies, please use AlphaLast|Big: That is RGBA, and won't have the overhead of a format-swizzling copy when we give it to D2D.

Copy link

Choose a reason for hiding this comment

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

Sorry: premultiplied alpha last | big

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@DHowett-MSFT updating 😄


woc::unique_cf<CGContextRef> patternContext{ CGBitmapContextCreate(
nullptr, tileSize.size.width, tileSize.size.height, bitsPerComponent, bytesPerRow, colorspace.get(), bitmapInfo) };
RETURN_HR_IF_NULL(E_UNEXPECTED, patternContext);

// Determine if this pattern is a colored pattern (the coloring is specified in the pattern callback) or if it is stencil pattern (the
// color is set outside and not within the pattern callback)
bool isColored = _CGPatternIsColored(pattern);
Copy link
Contributor

@rajsesh rajsesh Feb 14, 2017 •
edited by msft-Jeyaram
Loading

Choose a reason for hiding this comment

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

Nit: can you just do this in the lambda? will simplify the code. #ByDesign

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it'll be duplicated in both stoke and fill.
I'll leave it here as the comments will also get duplicated.

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


// Stage the drawing context
RETURN_IF_FAILED(std::forward<ContextStageLambda>(contextStage)(patternContext.get()));
RETURN_IF_FAILED(std::forward<ContextStageLambda>(contextStage)(patternContext.get(), isColored));
Copy link
Contributor

@aballway aballway Feb 15, 2017 •
edited by msft-Jeyaram
Loading

Choose a reason for hiding this comment

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

On more thought, the lambda is really only saving us from having to check if it's stroke vs fill. For this case I think it'd be better to pass in a flag (enum or just bool since this'll be the only difference) and inline the lambda stuff here. #ByDesign

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes but this helps make this more general.
Leaving it as it is.

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


// Now we ask the user to draw
_CGPatternIssueCallBack(patternContext.get(), pattern);

// Get the image out of it

woc::unique_cf<CGImageRef> bitmapTiledimage{ CGBitmapContextCreateImage(patternContext.get()) };
woc::unique_cf<CGImageRef> tileImage{ __CGContextCreateRenderableImage(bitmapTiledimage.get()) };
RETURN_HR_IF_NULL(E_UNEXPECTED, tileImage);
Expand Down Expand Up @@ -1848,9 +1852,18 @@ void CGContextSetFillPattern(CGContextRef context, CGPatternRef pattern, const C
}

ComPtr<ID2D1BitmapBrush1> bitmapBrush;
FAIL_FAST_IF_FAILED(_CreatePatternBrush(context, pattern, components, &bitmapBrush, [&](CGContextRef drawingContext) {
FAIL_FAST_IF_FAILED(_CreatePatternBrush(context, pattern, &bitmapBrush, [&](CGContextRef drawingContext, bool isColored) {
Copy link
Contributor

@aballway aballway Feb 15, 2017 •
edited by msft-Jeyaram
Loading

Choose a reason for hiding this comment

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

is it possible that we'll have other pattern variables to address in the future? Even if not, I think it would make more sense for the function to take (CGContextRef, CGPatternRef) rather than a bool flag, especially since the lambdas are almost identical already. #WontFix

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no and we don't need to keep on passing it.
I just did it at one place, to add the comments on what's happening with the colored/non-colored indication.

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

CGContextSetFillColorSpace(drawingContext, context->FillColorSpace());
CGContextSetFillColor(drawingContext, components);
if (isColored) {
// It's a colored pattern, the color is specified by the pattern callback.
// The 'components' are alpha value.
CGContextSetAlpha(drawingContext, components[0]);
} else {
// It is a stencil pattern, we should stage the context's color
// The 'components' are the color for the stencil.
CGContextSetFillColor(drawingContext, components);
}

return S_OK;
}));
// set the fill brush
Expand Down Expand Up @@ -1879,9 +1892,17 @@ void CGContextSetStrokePattern(CGContextRef context, CGPatternRef pattern, const
}

ComPtr<ID2D1BitmapBrush1> bitmapBrush;
FAIL_FAST_IF_FAILED(_CreatePatternBrush(context, pattern, components, &bitmapBrush, [&](CGContextRef drawingContext) {
FAIL_FAST_IF_FAILED(_CreatePatternBrush(context, pattern, &bitmapBrush, [&](CGContextRef drawingContext, bool isColored) {
CGContextSetStrokeColorSpace(drawingContext, context->StrokeColorSpace());
CGContextSetStrokeColor(drawingContext, components);
if (isColored) {
// It's a colored pattern, the color is specified by the pattern callback.
// The 'components' are alpha value.
CGContextSetAlpha(drawingContext, components[0]);
} else {
// It is a stencil pattern, we should stage the context's color
// The 'components' are the color for the stencil.
CGContextSetStrokeColor(drawingContext, components);
}
return S_OK;
}));
// set the stroke brush
Expand Down
4 changes: 4 additions & 0 deletions Frameworks/CoreGraphics/CGPattern.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,3 +233,7 @@ CGRect _CGPatternGetFinalPatternSize(CGPatternRef pattern) {
RETURN_RESULT_IF_NULL(pattern, CGRectNull);
return { CGPointZero, { ((CGPattern*)pattern)->xStep, ((CGPattern*)pattern)->yStep } };
}

bool _CGPatternIsColored(CGPatternRef pattern) {
return ((CGPattern*)pattern)->isColored;
}
7 changes: 7 additions & 0 deletions Frameworks/include/CGPatternInternal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,3 +50,10 @@ CGAffineTransform _CGPatternGetTransformation(CGPatternRef pattern);
* Get the final size of the pattern tile (after xStep and xStep has been applied).
*/
CGRect _CGPatternGetFinalPatternSize(CGPatternRef pattern);

/*
* Get the pattern colored value.
* If it is colored, then we have a colored pattern has inherent color,
* if it's false then we have a stencil pattern does not have inherent color.
*/
bool _CGPatternIsColored(CGPatternRef pattern);
1 change: 1 addition & 0 deletions build/Tests/UnitTests/CoreGraphics.Drawing/CoreGraphics.Drawing.UnitTests.vcxproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,7 @@
<ClangCompile Include="$(StarboardBasePath)\tests\unittests\CoreGraphics.Drawing\CGContextDrawing_ImageMaskTests.cpp" />
<ClangCompile Include="$(StarboardBasePath)\tests\unittests\CoreGraphics.Drawing\CGContextDrawing_GradientTests.cpp" />
<ClangCompile Include="$(StarboardBasePath)\tests\unittests\CoreGraphics.Drawing\CGContextDrawing_ImageDrawingTests.cpp" />
<ClangCompile Include="$(StarboardBasePath)\tests\unittests\CoreGraphics.Drawing\CGContextDrawing_PatternTests.cpp" />
<ClangCompile Include="$(StarboardBasePath)\tests\unittests\CoreGraphics.Drawing\CGPathDrawingTests.cpp" />
<ClangCompile Include="$(StarboardBasePath)\tests\unittests\CoreGraphics.Drawing\DrawingTest.cpp" />
<ClangCompile Include="$(StarboardBasePath)\tests\unittests\CoreGraphics.drawing\ImageComparison.cpp" />
Expand Down
204 changes: 0 additions & 204 deletions tests/UnitTests/CoreGraphics.drawing/CGContextDrawingTests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,163 +19,6 @@
#include "ImageHelpers.h"
#include <windows.h>

static void drawPatternWindowsLogo(void* info, CGContextRef context) {
CGContextFillRect(context, CGRectMake(0, 0, 50, 50));

CGContextSetRGBFillColor(context, 0, 0.63, 0.94, 1);
CGContextFillRect(context, CGRectMake(0, 50, 50, 50));

CGContextSetRGBFillColor(context, 0.48, 0.73, 0, 1);
CGContextFillRect(context, CGRectMake(50, 50, 50, 50));

CGContextSetRGBFillColor(context, 1, 0.73, 0, 1);
CGContextFillRect(context, CGRectMake(50, 0, 50, 50));
}

static void drawPatternSliced(void* info, CGContextRef context) {
CGFloat red[] = { 1, 0, 0, 1 };
CGContextSetStrokeColor(context, red);

CGPoint points[] = { { 0.0, 0.0 }, { 100, 100 }, { 100, 0.0 }, { 0.0, 100 } };
CGContextStrokeLineSegments(context, points, 4);

CGFloat green[] = { 0, 1, 0, 1 };
CGContextSetFillColor(context, green);
CGRect middleSpot = CGRectMake(100 / 8 * 3, 100 / 8 * 3, 2 * 100 / 8, 2 * 100 / 8);
CGContextFillRect(context, middleSpot);
}

static void _SetPatternForStroke(CGContextRef context, CGRect rect, float xStep, float yStep, CGPatternDrawPatternCallback drawpattern) {
CGPatternCallbacks coloredPatternCallbacks = { 0, drawpattern, NULL };

CGPatternRef pattern =
CGPatternCreate(NULL, rect, CGAffineTransformIdentity, xStep, yStep, kCGPatternTilingNoDistortion, false, &coloredPatternCallbacks);

CFAutorelease(pattern);
woc::unique_cf<CGColorSpaceRef> rgbColorSpace(CGColorSpaceCreateDeviceRGB());
woc::unique_cf<CGColorSpaceRef> patternColorSpace{ CGColorSpaceCreatePattern(rgbColorSpace.get()) };

CGContextSetStrokeColorSpace(context, patternColorSpace.get());

CGFloat color[] = { 0.96, 0.32, 0.07, 1 };
CGContextSetStrokePattern(context, pattern, color);
}

static void _SetPatternForFill(CGContextRef context, CGRect rect, float xStep, float yStep, CGPatternDrawPatternCallback drawpattern) {
CGPatternCallbacks coloredPatternCallbacks = { 0, drawpattern, NULL };

CGPatternRef pattern =
CGPatternCreate(NULL, rect, CGAffineTransformIdentity, xStep, yStep, kCGPatternTilingNoDistortion, false, &coloredPatternCallbacks);

CFAutorelease(pattern);
woc::unique_cf<CGColorSpaceRef> rgbColorSpace(CGColorSpaceCreateDeviceRGB());
woc::unique_cf<CGColorSpaceRef> patternColorSpace{ CGColorSpaceCreatePattern(rgbColorSpace.get()) };

CGFloat color[] = { 0.96, 0.32, 0.07, 1 };

CGContextSetFillColorSpace(context, patternColorSpace.get());

CGContextSetFillPattern(context, pattern, color);
}

DISABLED_DRAW_TEST_F(CGContext, PatternStroke, UIKitMimicTest<>) {
CGContextRef context = GetDrawingContext();
CGRect bounds = GetDrawingBounds();

_SetPatternForStroke(context, CGRectMake(0, 0, 100, 100), 100, 100, drawPatternWindowsLogo);
CGRect borderRect = CGRectInset(bounds, 30, 50);
CGContextSetLineWidth(context, 45);
CGContextStrokeRect(context, borderRect);
}

DISABLED_DRAW_TEST_F(CGContext, PatternStrokeSliced, UIKitMimicTest<>) {
CGContextRef context = GetDrawingContext();
CGRect bounds = GetDrawingBounds();

_SetPatternForStroke(context, CGRectMake(0, 0, 100, 100), 100, 100, drawPatternSliced);
CGRect borderRect = CGRectInset(bounds, 30, 50);
CGContextSetLineWidth(context, 45);
CGContextStrokeRect(context, borderRect);
}

DISABLED_DRAW_TEST_F(CGContext, PatternDrawPath, UIKitMimicTest<>) {
CGContextRef context = GetDrawingContext();
CGRect bounds = GetDrawingBounds();

CGMutablePathRef theFirstPath = CGPathCreateMutable();
CGMutablePathRef theSecondPath = CGPathCreateMutable();

CGPathMoveToPoint(theFirstPath, NULL, 200, 35);
CGPathAddLineToPoint(theFirstPath, NULL, 165, 100);
CGPathAddLineToPoint(theFirstPath, NULL, 100, 100);
CGPathAddLineToPoint(theFirstPath, NULL, 150, 150);
CGPathAddLineToPoint(theFirstPath, NULL, 135, 225);
CGPathAddLineToPoint(theFirstPath, NULL, 200, 170);
CGPathAddLineToPoint(theFirstPath, NULL, 265, 225);

CGPathMoveToPoint(theSecondPath, NULL, 265, 225);

CGPathAddLineToPoint(theSecondPath, NULL, 350, 225);
CGPathAddLineToPoint(theSecondPath, NULL, 350, 35);
CGPathAddLineToPoint(theSecondPath, NULL, 200, 35);

CGPathAddPath(theFirstPath, NULL, theSecondPath);
CGContextAddPath(context, theFirstPath);

CGContextClosePath(context);

CGContextSetLineWidth(context, 15);
_SetPatternForFill(context, CGRectMake(0, 0, 100, 100), 100, 100, drawPatternWindowsLogo);
_SetPatternForStroke(context, CGRectMake(0, 0, 100, 100), 100, 100, drawPatternSliced);

CGContextDrawPath(context, kCGPathEOFillStroke);
CGPathRelease(theFirstPath);
CGPathRelease(theSecondPath);
}

DISABLED_DRAW_TEST_F(CGContext, PatternFill, UIKitMimicTest<>) {
CGContextRef context = GetDrawingContext();
CGRect bounds = GetDrawingBounds();

_SetPatternForFill(context, CGRectMake(0, 0, 100, 100), 100, 100, drawPatternWindowsLogo);
CGContextFillRect(context, bounds);
}

DRAW_TEST_F(CGContext, PatternFillNULLRect, UIKitMimicTest<>) {
CGContextRef context = GetDrawingContext();
CGRect bounds = GetDrawingBounds();

CGContextSetRGBFillColor(context, 0.5, 0.5, 0.5, 1.0);
CGContextFillRect(context, bounds);

_SetPatternForFill(context, CGRectNull, 100, 100, drawPatternWindowsLogo);

CGRect borderRect = CGRectInset(bounds, 30, 50);

CGContextFillRect(context, borderRect);
}

DRAW_TEST_F(CGContext, PatternStrokeNULLRect, UIKitMimicTest<>) {
CGContextRef context = GetDrawingContext();
CGRect bounds = GetDrawingBounds();

CGContextSetRGBFillColor(context, 0.5, 0.5, 0.5, 1.0);
CGContextFillRect(context, bounds);

_SetPatternForStroke(context, CGRectNull, 100, 100, drawPatternWindowsLogo);
CGRect borderRect = CGRectInset(bounds, 30, 50);
CGContextSetLineWidth(context, 45);
CGContextStrokeRect(context, borderRect);
}

DISABLED_DRAW_TEST_F(CGContext, PatternFillSliced, UIKitMimicTest<>) {
CGContextRef context = GetDrawingContext();
CGRect bounds = GetDrawingBounds();

_SetPatternForFill(context, CGRectMake(0, 0, 100, 100), 100, 100, drawPatternSliced);
CGContextFillRect(context, bounds);
}

DISABLED_DRAW_TEST_F(CGContext, Canva, UIKitMimicTest<>) {
CGContextRef context = GetDrawingContext();
CGRect bounds = GetDrawingBounds();
Expand All @@ -192,53 +35,6 @@ DISABLED_DRAW_TEST_F(CGContext, Canva, UIKitMimicTest<>) {
CGContextFillRect(context, middleSpot);
}

DISABLED_DRAW_TEST_F(CGContext, PatternFillWindowsLogoWithAlpha, UIKitMimicTest<>) {
CGContextRef context = GetDrawingContext();
CGRect bounds = GetDrawingBounds();

CGContextSetAlpha(context, 0.8);
_SetPatternForFill(context, CGRectMake(0, 0, 100, 100), 150, 150, drawPatternWindowsLogo);
CGContextFillRect(context, bounds);
}

DISABLED_DRAW_TEST_F(CGContext, PatternFillWindowsLogoRotate, UIKitMimicTest<>) {
CGContextRef context = GetDrawingContext();
CGRect bounds = GetDrawingBounds();

_SetPatternForFill(context, CGRectMake(0, 0, 100, 100), 100, 100, drawPatternWindowsLogo);
CGContextRotateCTM(context, 0.4);
CGContextFillRect(context, bounds);
}

DISABLED_DRAW_TEST_F(CGContext, PatternFillWindowsLogoRegion, UIKitMimicTest<>) {
CGContextRef context = GetDrawingContext();
CGRect bounds = GetDrawingBounds();

_SetPatternForFill(context, CGRectMake(0, 0, 100, 100), 100, 100, drawPatternWindowsLogo);
CGRect borderRect = CGRectInset(bounds, 30, 50);
CGContextFillRect(context, borderRect);
}

DISABLED_DRAW_TEST_F(CGContext, PatternFillWindowsLogoPath, UIKitMimicTest<>) {
CGContextRef context = GetDrawingContext();
CGRect bounds = GetDrawingBounds();

_SetPatternForFill(context, CGRectMake(0, 0, 100, 100), 100, 100, drawPatternWindowsLogo);

CGMutablePathRef thepath = CGPathCreateMutable();
CGPathMoveToPoint(thepath, NULL, 30, 100);
CGPathAddCurveToPoint(thepath, NULL, 47.0f, 67.0f, 50.0f, 55.0f, 45.0f, 50.0f);
CGPathAddCurveToPoint(thepath, NULL, 42.0f, 47.0f, 37.0f, 46.0f, 30.0f, 55.0f);

CGPathAddCurveToPoint(thepath, NULL, 23.0f, 46.0f, 18.0f, 47.0f, 15.0f, 50.0f);
CGPathAddCurveToPoint(thepath, NULL, 10.0f, 55.0f, 13.0f, 67.0f, 30.0f, 100.0f);

CGPathCloseSubpath(thepath);
CGContextAddPath(context, thepath);
CGContextFillPath(context);
CGPathRelease(thepath);
}

DRAW_TEST_F(CGContext, RedBox, UIKitMimicTest<>) {
CGContextRef context = GetDrawingContext();
CGRect bounds = GetDrawingBounds();
Expand Down
Loading