-
Notifications
You must be signed in to change notification settings - Fork 806
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.
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
Changes from 1 commit
2a06c45
5cbe655
adcef78
58dbe68
95821be
0d94658
1d2120b
20dad65
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1770,8 +1770,10 @@ static CGImageRef __CGContextCreateRenderableImage(CGImageRef image) { | |
#pragma region Drawing Parameters - Stroke / Fill Patterns | ||
|
||
template <typename ContextStageLambda> // Takes the form HRESULT(*)(CGContextRef) | ||
static HRESULT _CreatePatternBrush( | ||
CGContextRef context, CGPatternRef pattern, const CGFloat* components, ID2D1BitmapBrush1** brush, ContextStageLambda&& contextStage) { | ||
static HRESULT _CreatePatternBrush(CGContextRef context, | ||
CGPatternRef pattern, | ||
ID2D1BitmapBrush1** brush, | ||
ContextStageLambda&& contextStage) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: make the out param the last one #WontFix There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) |
||
// TODO #1592: change to support grayscale (masks) after dustins change. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TODO is obsolete now. #Resolved |
||
woc::unique_cf<CGColorSpaceRef> colorspace{ CGColorSpaceCreateDeviceRGB() }; | ||
|
||
|
@@ -1781,20 +1783,23 @@ static HRESULT _CreatePatternBrush( | |
|
||
size_t bitsPerComponent = 8; | ||
size_t bytesPerRow = 4 * tileSize.size.width; | ||
CGBitmapInfo bitmapInfo = kCGImageAlphaPremultipliedFirst | kCGBitmapByteOrder32Big; | ||
CGBitmapInfo bitmapInfo = kCGImageAlphaPremultipliedFirst | kCGBitmapByteOrderDefault; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @rajsesh-msft alpha first + default = There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry: premultiplied alpha last | big There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it'll be duplicated in both stoke and fill. 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)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes but this helps make this more general. 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); | ||
|
@@ -1848,9 +1853,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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no and we don't need to keep on passing it. 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 | ||
|
@@ -1879,9 +1893,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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -233,3 +233,8 @@ CGRect _CGPatternGetFinalPatternSize(CGPatternRef pattern) { | |
RETURN_RESULT_IF_NULL(pattern, CGRectNull); | ||
return { CGPointZero, { ((CGPattern*)pattern)->xStep, ((CGPattern*)pattern)->yStep } }; | ||
} | ||
|
||
bool _CGPatternIsColored(CGPatternRef pattern) { | ||
FAIL_FAST_IF_NULL(pattern); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: we shouldn't fail fast in private functions. Our pattern has been to return an HRESULT and fail in the base CG API function. That said, I don't think you need the null check in particular here. Since it's a private method, move the fail_fast_if_null to CGContext. If you plan on using this method in more places though I would recommend keeping the fail fast here and returning a struct for {HRESULT, returnvalue} which has been our preferred pattern or passing a return bool by reference. #Resolved There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. also, it null cannot happen here, just the code below should fail_fast it for you 😄 #Resolved |
||
return ((CGPattern*)pattern)->isColored; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,178 @@ | ||
//****************************************************************************** | ||
// | ||
// Copyright (c) Microsoft. All rights reserved. | ||
// | ||
// This code is licensed under the MIT License (MIT). | ||
// | ||
// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR | ||
// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, | ||
// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE | ||
// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER | ||
// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, | ||
// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN | ||
// THE SOFTWARE. | ||
// | ||
//****************************************************************************** | ||
|
||
#include "DrawingTest.h" | ||
|
||
#pragma region Stencil Pattern | ||
|
||
static void drawStencilStar(void* info, CGContextRef context) { | ||
double r = 0.8 * 16 / 2; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: why all of the 16 / 2? make this a constant? #WontFix There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is just a test, doesn't make sense to make everything in tests as consts. In reply to: 101328691 [](ancestors = 101328691) |
||
double theta = 2 * M_PI * (2.0 / 5.0); // 144 degrees | ||
|
||
CGContextTranslateCTM(context, 16 / 2, 16 / 2); | ||
|
||
CGContextMoveToPoint(context, 0, r); | ||
for (int i = 1; i < 5; i++) { | ||
CGContextAddLineToPoint(context, r * sin(i * theta), r * cos(i * theta)); | ||
} | ||
CGContextClosePath(context); | ||
CGContextFillPath(context); | ||
} | ||
|
||
static void drawStencilBox(void* info, CGContextRef context) { | ||
CGContextFillRect(context, CGRectMake(0, 0, 8, 8)); | ||
} | ||
|
||
struct PatternCallback { | ||
const char* name; | ||
CGPatternDrawPatternCallback callback; | ||
}; | ||
|
||
struct StencilColor { | ||
const char* name; | ||
float color[4]; | ||
}; | ||
|
||
PatternCallback stencilPatternCallback[] = { { "drawStencilStar", &drawStencilStar }, { "drawStencilBox", &drawStencilBox } }; | ||
|
||
StencilColor stencilColors[] = { { "orange", { 1, 0.64, 0, 1 } }, { "gold", { 1, 0.84, 0, 1 } }, { "red", { 1, 0, 0, 1 } }, | ||
{ "green", { 0, 1, 0, 1 } }, { "blue", { 0, 0, 1, 1 } }, { "cyan", { 0, 1, 1, 1 } }, | ||
{ "teal", { 0, 0.5, 0.5, 1 } }, { "purple", { 0.5, 0, 0.5, 1 } }, { "Maroon", { 0.5, 0, 0, 1 } } }; | ||
|
||
int patternsizes[] = { 1, 2, 4, 8, 13, 10, 16, 20, 32, 128, 241 }; | ||
|
||
//<PatternCallback,StencilColor,pattern size> | ||
class CGPatternStencil : public WhiteBackgroundTest<>, | ||
public ::testing::WithParamInterface<::testing::tuple<PatternCallback, StencilColor, int>> { | ||
CFStringRef CreateOutputFilename() { | ||
PatternCallback stencil = ::testing::get<0>(GetParam()); | ||
StencilColor stencilColor = ::testing::get<1>(GetParam()); | ||
int size = ::testing::get<2>(GetParam()); | ||
|
||
return CFStringCreateWithFormat(nullptr, | ||
nullptr, | ||
CFSTR("TestImage.CGContext.CGPatternStencil.%s.%s.%d.png"), | ||
stencil.name, | ||
stencilColor.name, | ||
size); | ||
} | ||
}; | ||
|
||
TEST_P(CGPatternStencil, DrawStencils) { | ||
CGPatternDrawPatternCallback callback = ::testing::get<0>(GetParam()).callback; | ||
const CGFloat* stencilColor = ::testing::get<1>(GetParam()).color; | ||
int size = ::testing::get<2>(GetParam()); | ||
|
||
CGContextRef context = GetDrawingContext(); | ||
CGRect bounds = GetDrawingBounds(); | ||
|
||
static const CGPatternCallbacks callbacks = { 0, callback, NULL }; | ||
|
||
CGColorSpaceRef baseSpace = CGColorSpaceCreateDeviceRGB(); | ||
CGColorSpaceRef patternSpace = CGColorSpaceCreatePattern(baseSpace); | ||
CGContextSetFillColorSpace(context, patternSpace); | ||
CGColorSpaceRelease(patternSpace); | ||
CGColorSpaceRelease(baseSpace); | ||
|
||
CGPatternRef pattern = CGPatternCreate( | ||
NULL, CGRectMake(0, 0, size, size), CGAffineTransformIdentity, size, size, kCGPatternTilingConstantSpacing, false, &callbacks); | ||
|
||
CGContextSetFillPattern(context, pattern, stencilColor); | ||
CGPatternRelease(pattern); | ||
CGContextFillRect(context, bounds); | ||
} | ||
|
||
INSTANTIATE_TEST_CASE_P(CGPatternTests, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The stars with the lowest size don't appear. I don't think it's worth having a test for those sizes especially when the images are blank. #Resolved There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it's because the drawing gets clipped due to the size. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need to verify so many times that they are getting clipped (with different colors? ) 😸 #Resolved |
||
CGPatternStencil, | ||
::testing::Combine(::testing::ValuesIn(stencilPatternCallback), | ||
::testing::ValuesIn(stencilColors), | ||
::testing::ValuesIn(patternsizes))); | ||
|
||
#pragma endregion Stencil Pattern | ||
|
||
#pragma region Colored Pattern | ||
|
||
void drawColoredPatternMultiSquare(void* info, CGContextRef context) { | ||
CGFloat subunit = 5; // the pattern cell itself is 16 by 18 | ||
|
||
CGRect myRect1 = { { 0, 0 }, { subunit, subunit } }, myRect2 = { { subunit, subunit }, { subunit, subunit } }, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: this is not one of those cases where multiple variables on the same line makes sense 🌮 #Resolved |
||
myRect3 = { { 0, subunit }, { subunit, subunit } }, myRect4 = { { subunit, 0 }, { subunit, subunit } }; | ||
|
||
CGContextSetRGBFillColor(context, 0, 0, 1, 0.5); | ||
CGContextFillRect(context, myRect1); | ||
CGContextSetRGBFillColor(context, 1, 0, 0, 0.5); | ||
CGContextFillRect(context, myRect2); | ||
CGContextSetRGBFillColor(context, 0, 1, 0, 0.5); | ||
CGContextFillRect(context, myRect3); | ||
CGContextSetRGBFillColor(context, .5, 0, .5, 0.5); | ||
CGContextFillRect(context, myRect4); | ||
} | ||
|
||
PatternCallback ColoredPatternCallback[] = { { "drawColoredPatternMultiSquare", &drawColoredPatternMultiSquare } }; | ||
|
||
//<PatternCallback,alpha,pattern size> | ||
class CGPatternColored : public WhiteBackgroundTest<>, | ||
public ::testing::WithParamInterface<::testing::tuple<PatternCallback, CGFloat, int>> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. super duper nit: should use CGFloat rather than int for CGSize width, height values #Pending |
||
CFStringRef CreateOutputFilename() { | ||
PatternCallback pattern = ::testing::get<0>(GetParam()); | ||
CGFloat alpha = ::testing::get<1>(GetParam()); | ||
int size = ::testing::get<2>(GetParam()); | ||
|
||
return CFStringCreateWithFormat(nullptr, | ||
nullptr, | ||
CFSTR("TestImage.CGContext.CGPatternColored.%s.size.%d.A%1.01f.png"), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
need light naming fix. #Resolved |
||
pattern.name, | ||
size, | ||
alpha); | ||
} | ||
}; | ||
|
||
TEST_P(CGPatternColored, ColoredSquare) { | ||
CGPatternDrawPatternCallback callback = ::testing::get<0>(GetParam()).callback; | ||
CGFloat alpha = ::testing::get<1>(GetParam()); | ||
int size = ::testing::get<2>(GetParam()); | ||
|
||
CGContextRef context = GetDrawingContext(); | ||
CGRect bounds = GetDrawingBounds(); | ||
static const CGPatternCallbacks callbacks = { 0, callback, NULL }; | ||
|
||
CGColorSpaceRef patternSpace = CGColorSpaceCreatePattern(NULL); | ||
CGContextSetFillColorSpace(context, patternSpace); | ||
CGColorSpaceRelease(patternSpace); | ||
|
||
CGPatternRef pattern = CGPatternCreate(NULL, | ||
CGRectMake(0, 0, size / 2, size / 2), | ||
CGAffineTransformMake(1, 0, 0, 1, 0, 0), | ||
size, | ||
size, | ||
kCGPatternTilingConstantSpacing, | ||
true, | ||
&callbacks); | ||
|
||
CGContextSetFillPattern(context, pattern, &alpha); | ||
CGPatternRelease(pattern); | ||
CGContextFillRect(context, bounds); | ||
} | ||
|
||
static CGFloat alphas[] = { 0.0f, 0.25f, 0.35f, 0.42f, 0.5f, 0.75f, 0.85f, 0.95f, 1.f }; | ||
|
||
INSTANTIATE_TEST_CASE_P(CGPatternTests, | ||
CGPatternColored, | ||
::testing::Combine(::testing::ValuesIn(ColoredPatternCallback), | ||
::testing::ValuesIn(alphas), | ||
::testing::ValuesIn(patternsizes))); | ||
|
||
#pragma endregion Colored Pattern |
There was a problem hiding this comment.
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