Skip to content
This repository was archived by the owner on May 28, 2025. It is now read-only.

Fix CoreText Drawing#1297

Merged
aballway merged 1 commit into
microsoft:developfrom
aballway:ct
Nov 2, 2016
Merged

Fix CoreText Drawing#1297
aballway merged 1 commit into
microsoft:developfrom
aballway:ct

Conversation

@aballway

@aballway aballway commented Nov 2, 2016

Copy link
Copy Markdown
Contributor

It seems our drawing implementation was overcomplicated, doing unnecessary transformations which made assumptions about the state of the context which were not always true. Removes now unnecessary private drawing functions and removes ascent as well as simplifying CGContextDrawGlyphRun.

This should be the last change we need for drawing

Fixes #1290


This change is Reviewable

@rajsesh rajsesh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Seems straightforward enough. I'd like to be able to test these using automated tests using bitmap contexts. Given that this is blocking the release, I am approving this with manual validation, please open an issue to implement the said automated tests.


void CGContextDrawGlyphRun(CGContextRef ctx, const DWRITE_GLYPH_RUN* glyphRun, float lineAscent) {
ctx->Backing()->CGContextDrawGlyphRun(glyphRun, lineAscent);
void CGContextDrawGlyphRun(CGContextRef ctx, const DWRITE_GLYPH_RUN* glyphRun) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: This really should be _CGContextDrawGlyphRun as this isn't a public API. There are a few others that fall into this bucket, so if you want to open an issue and fix those separately (maybe @DHowett-MSFT, @MSFTFox or @msft-Jeyaram could fix it with the D2D work?)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

transform = CGAffineTransformConcat(transform, userTransform);
CGAffineTransformConcat(curState->curTransform, CGAffineTransformMake(1.0f / _scale, 0, 0, -1.0f / _scale, 0, height / _scale));

// Perform anti-clockwise rotation required to match the reference platform.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

are we still respecting this? (do we rotate in the wrong orientation now?)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Interestingly enough we rotate in the correct orientation without doing the conversion.

// Technically there should be a horizontal translation to the center as well,
// but it's to the center of _each individual glyph_, as the reference platform applies the text matrix to each glyph individually
// Uncertain whether it's ever going to be worth it to support this using DWrite, so just ignore it for now
CGAffineTransform transform = CGAffineTransformMake(1, 0, 0, -1, 0, -lineAscent / 2.0f);

@ms-jihua ms-jihua Nov 2, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

CGAffineTransform transform = CGAffineTransformMake(1, 0, 0, -1, 0, -lineAscent / 2.0f); [](start = 4, length = 88)

I'm sorry, I failed you :( #Resolved

@ms-jihua

ms-jihua commented Nov 2, 2016

Copy link
Copy Markdown
Contributor

:shipit:

1 similar comment
@bbowman

bbowman commented Nov 2, 2016

Copy link
Copy Markdown
Member

:shipit:

@aballway aballway merged commit 5eed895 into microsoft:develop Nov 2, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants