Implement CTFontGetVerticalTranslationsForGlyphs #2104

aballway · 2017-02-27T18:00:23Z

Fixes #2025

There is a magic number that the reference platform adds to every vertical glyph translation independent of font. With this constant added to all vertical translations we are within 1 design unit of reference platform values.

pradipd · 2017-02-27T18:04:11Z

Out of curiosity, what is unit of measurement is "1em"? #Closed

aballway · 2017-02-27T18:09:04Z

I mistyped, it should be "design units" but em is arbitrary I think? I still don't 100% grok the differences between the units,

ms-jihua · 2017-02-27T18:13:47Z

Frameworks/CoreText/CTFont.mm

@@ -96,6 +96,9 @@
 const CFStringRef kCTFontDefaultItalicFontName = CFSTR("Arial Italic");
 const CFStringRef kCTFontDefaultMonospacedFontName = CFSTR("Courier New");

+// Reference platform adds a constant amount of space (scaled by font size) to each glyph's vertical translation
+static const unsigned int sc_VerticalTranslationSpace = 102;


nit: camelCase instead of PascalCase?

ms-jihua · 2017-02-27T18:18:09Z

scripts/git/common.ps1

@@ -55,9 +55,9 @@ function Get-StagedFiles {

 # Formats the given file using ClangFormat
 function ClangFormatFile([string]$repoRoot, [string]$filename, [bool]$isDryRun){
-    $clangCommand = "$repoRoot/tools/scripts/clang-format.ps1"
+    $clangCommand = "$repoRoot/scripts/git/clang-format.ps1"


This branch changed the location of the scripts without updating the scripts, needed to fix this ro get my pre-commit to run

oh, i didn't see that this wasn't going into develop. how come that's the case?

Testing the packaging branch basically

what's the plan for getting this into develop then? are we cherry-picking it, or will develop not have it until packaging merges?

¯\(ツ)/¯

a. We should update the script in a separate change.
b. This will merge to develop when packaging branch merges (or we cherry-pick this to develop if needed).

b ==> a is critical.

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

ms-jihua · 2017-02-27T18:19:09Z

tests/unittests/CoreText/CTFontTests.mm

+TEST(CTFont, GetVerticalTranslationsForGLyphs) {
+    auto font = woc::MakeAutoCF<CTFontRef>(CTFontCreateWithName(CFSTR("Arial"), 20, nullptr));
+    UniChar chars[4] = { 'T', 'e', 's', 't' };


can you also test the translations for ' ' and '_'?

ms-jihua · 2017-02-27T18:27:36Z

1em is approximately 1 'character height', and varies depending on font size and designer preference, but is generally = ~the height in px of a capital H at the current font size.

A design unit is a constant fraction, usually 1/1024, of an em. The actual fraction (designUnitsPerEm) is up to the designer, but it's usually something like that. Coordinates for fonts (ie: height of an 'r') are usually specified in whole design units (font coordinates usually don't look like 254.876 of an em).

rajsesh · 2017-02-27T19:01:05Z

https://en.wikipedia.org/wiki/Em_(typography

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

rajsesh · 2017-02-27T19:03:57Z

🕐

ms-jihua · 2017-02-27T20:55:34Z

Frameworks/CoreText/CTFont.mm

+                return CGSizeZero;
+            }
+
+            return CGSize{ -__CTFontScaleMetric(font, metrics.advanceWidth >> 1),


mm, I understand that this >> 1 is probably safe, but the gains here don't seem worth overriding principle of least surprise.

any compiler worth its salt will optimize /2 to >>1; we can likely rely on that behaviour and prefer /2 where it's clearer.

parentheses will probably make this clearer too?

I think that using bitshifts as multiply/divide by powers of two is common enough, and should be a simple enough piece to parse in the future.

it's common, but i also had to do a double-take and make sure that advanceWidth wasn't 1) a bit flag somehow 2) a signed int. imma double down and say you should just write what you mean here and let the compiler optimize.

/me assists with the doubling down

ms-jihua · 2017-02-27T20:56:37Z

tests/unittests/CoreText/CTFontTests.mm

-    CTFontGetVerticalTranslationsForGlyphs(font, glyphs, translations, 4);
-    EXPECT_NEAR(-6.108, translations[0].width, c_errorMargin);
-    EXPECT_NEAR(-5.562, translations[1].width, c_errorMargin);
+    UniChar chars[6] = { 'T', 'e', 's', 't', ' ', '_' };


vaguely interested in '-' and '|' also, but we probably won't find anything surprising, so i'll leave it up to you

pradipd · 2017-02-27T21:06:55Z

Thanks for the info.

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

rajsesh · 2017-02-27T23:28:16Z

Pending other approvals of course.

msft-Jeyaram · 2017-02-27T23:51:49Z

static const wchar_t* g_logTag = L"CTFont";

nit: what does the g means? #Closed

Refers to: Frameworks/CoreText/CTFont.mm:41 in a2dac20. [](commit_id = a2dac20, deletion_comment = False)

msft-Jeyaram · 2017-02-27T23:53:51Z

tests/unittests/CoreText/CTFontTests.mm

+TEST(CTFont, GetVerticalTranslationsForGLyphs) {
+    auto font = woc::MakeAutoCF<CTFontRef>(CTFontCreateWithName(CFSTR("Arial"), 20, nullptr));
+    UniChar chars[6] = { 'T', 'e', 's', 't', ' ', '_' };


UniChar [](start = 4, length = 7)

UniChar? no shouty

UniChar is from CF?

aballway · 2017-02-28T00:10:34Z

static const wchar_t* g_logTag = L"CTFont";

global, presumably. Not related to this PR though

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

Refers to: Frameworks/CoreText/CTFont.mm:41 in a2dac20. [](commit_id = a2dac20, deletion_comment = False)

DHowett-MSFT · 2017-02-28T00:10:45Z

@msft-Jeyaram UniChar isn't shouty; UNICHAR would be!

DHowett-MSFT

Please make sure this is in two different commits (one for the packaging change, one for the implementation) before merging.

DHowett-MSFT · 2017-02-28T00:11:27Z

Frameworks/CoreText/CTFont.mm

+                return CGSizeZero;
+            }
+
+            return CGSize{ -__CTFontScaleMetric(font, metrics.advanceWidth >> 1),


/me assists with the doubling down

msft-Jeyaram · 2017-02-28T00:12:28Z

:p I figured after i made the comment LOL

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

DHowett-MSFT

do the commit thing tho

ms-jihua

+1 to do the commit thing

aballway requested review from rajsesh and ms-jihua February 27, 2017 18:00

msftclas added the cla-not-required label Feb 27, 2017

aballway force-pushed the coretext branch from b31a279 to 3266e7f Compare February 27, 2017 18:01

ms-jihua reviewed Feb 27, 2017

View reviewed changes

aballway requested a review from bbowman February 27, 2017 18:22

ms-jihua reviewed Feb 27, 2017

View reviewed changes

rajsesh approved these changes Feb 27, 2017

View reviewed changes

msft-Jeyaram reviewed Feb 27, 2017

View reviewed changes

DHowett-MSFT suggested changes Feb 28, 2017

View reviewed changes

DHowett-MSFT approved these changes Feb 28, 2017

View reviewed changes

ms-jihua approved these changes Feb 28, 2017

View reviewed changes

aballway force-pushed the coretext branch from 10ee872 to 474ac36 Compare February 28, 2017 19:42

Implement CTFontGetVerticalTranslationsForGlyphs

726322d

aballway force-pushed the coretext branch from 9a57c57 to 726322d Compare February 28, 2017 20:03

aballway merged commit 3740a7e into microsoft:packaging Mar 1, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement CTFontGetVerticalTranslationsForGlyphs #2104

Implement CTFontGetVerticalTranslationsForGlyphs #2104

aballway commented Feb 27, 2017 •

edited

Loading

pradipd commented Feb 27, 2017 •

edited

Loading

aballway commented Feb 27, 2017

ms-jihua Feb 27, 2017

ms-jihua Feb 27, 2017

aballway Feb 27, 2017

ms-jihua Feb 27, 2017

aballway Feb 27, 2017

ms-jihua Feb 27, 2017

aballway Feb 27, 2017 •

edited

Loading

rajsesh Feb 27, 2017

ms-jihua Feb 27, 2017

ms-jihua commented Feb 27, 2017

rajsesh commented Feb 27, 2017

rajsesh commented Feb 27, 2017

ms-jihua Feb 27, 2017

DHowett-MSFT Feb 27, 2017

DHowett-MSFT Feb 27, 2017

aballway Feb 27, 2017

ms-jihua Feb 27, 2017

DHowett-MSFT Feb 28, 2017

ms-jihua Feb 27, 2017

pradipd commented Feb 27, 2017

rajsesh commented Feb 27, 2017

msft-Jeyaram commented Feb 27, 2017 •

edited by aballway

Loading

msft-Jeyaram Feb 27, 2017

aballway Feb 28, 2017

aballway commented Feb 28, 2017

DHowett-MSFT commented Feb 28, 2017

DHowett-MSFT left a comment

DHowett-MSFT Feb 28, 2017

msft-Jeyaram commented Feb 28, 2017

DHowett-MSFT left a comment

ms-jihua left a comment

Implement CTFontGetVerticalTranslationsForGlyphs #2104

Implement CTFontGetVerticalTranslationsForGlyphs #2104

Conversation

aballway commented Feb 27, 2017 • edited Loading

pradipd commented Feb 27, 2017 • edited Loading

aballway commented Feb 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aballway Feb 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ms-jihua commented Feb 27, 2017

rajsesh commented Feb 27, 2017

rajsesh commented Feb 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pradipd commented Feb 27, 2017

rajsesh commented Feb 27, 2017

msft-Jeyaram commented Feb 27, 2017 • edited by aballway Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aballway commented Feb 28, 2017

DHowett-MSFT commented Feb 28, 2017

DHowett-MSFT left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msft-Jeyaram commented Feb 28, 2017

DHowett-MSFT left a comment

Choose a reason for hiding this comment

ms-jihua left a comment

Choose a reason for hiding this comment

aballway commented Feb 27, 2017 •

edited

Loading

pradipd commented Feb 27, 2017 •

edited

Loading

aballway Feb 27, 2017 •

edited

Loading

msft-Jeyaram commented Feb 27, 2017 •

edited by aballway

Loading