From f89220041d82d1c360830937139e271ffc890c3a Mon Sep 17 00:00:00 2001 From: Michael Niksa Date: Wed, 26 Feb 2020 11:19:28 -0800 Subject: [PATCH 01/17] Don't split surrogate pairs when breaking runs for scaling. Affects emoji rendering. #4704 --- src/renderer/dx/CustomTextLayout.cpp | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/renderer/dx/CustomTextLayout.cpp b/src/renderer/dx/CustomTextLayout.cpp index ea809fe956f..834f9ac88d1 100644 --- a/src/renderer/dx/CustomTextLayout.cpp +++ b/src/renderer/dx/CustomTextLayout.cpp @@ -361,13 +361,16 @@ CustomTextLayout::CustomTextLayout(gsl::not_null const factory // If scale corrections were needed, we need to split the run. for (auto [index, scale] : _glyphScaleCorrections) { + // Don't split in the middle of a surrogate pair. + const auto after = IS_HIGH_SURROGATE(_text.at(index)) ? 2 : 1; + // Split after the adjustment first so it // takes a copy of all the run properties before we modify them. // GH 4665: This is the other half of the potential future perf item. // If glyphs needing the same scale are coalesced, we could // break fewer times and have fewer runs. - _SetCurrentRun(index + 1); - _SplitCurrentRun(index + 1); + _SetCurrentRun(index + after); + _SplitCurrentRun(index + after); // Now split just this glyph off. _SetCurrentRun(index); From 98b483c8ee92ac6f7b4c57db035dd31e33a141ca Mon Sep 17 00:00:00 2001 From: Michael Niksa Date: Thu, 27 Feb 2020 15:42:17 -0800 Subject: [PATCH 02/17] merge #4438 by @milizhang --- src/renderer/dx/CustomTextLayout.cpp | 66 ++++++++++++++++++++++++---- 1 file changed, 58 insertions(+), 8 deletions(-) diff --git a/src/renderer/dx/CustomTextLayout.cpp b/src/renderer/dx/CustomTextLayout.cpp index 834f9ac88d1..4441ac4f487 100644 --- a/src/renderer/dx/CustomTextLayout.cpp +++ b/src/renderer/dx/CustomTextLayout.cpp @@ -411,15 +411,63 @@ try // Walk through advances and space out characters that are too small to consume their box. for (auto i = run.glyphStart; i < (run.glyphStart + run.glyphCount); i++) { + const auto iClusterBegin = i; + + // Get how many columns we expected the glyph to have and multiply into pixels. + UINT16 columns = 0; + { + // Because of typographic features such as ligatures, it is well possible for a cluster of glyph to + // represent multiple code points. Previous calls to IDWriteTextAnalyzer::GetGlyphs stores the mapping + // information between code points and glyphs in _glyphClusters. + // To properly allocate the columns for such glyph clusters, we need to find all characters that this + // cluster is representing and add column counts for all the characters together. + // Inside a glyph cluster, we should try our best to respect the advances and offsets to keep glyphs properly + // positioned. This means the glyph offset adjustment should be applied to all glyphs in cluster, and the + // column-based advance adjustment should be applied on the last glyph of the cluster. + + // Find the range for current glyph run in _glyphClusters. + const auto runStartIterator = _glyphClusters.begin() + run.textStart; + const auto runEndIterator = _glyphClusters.begin() + run.textStart + run.textLength; + + // Find the range of characters that the current glyph is representing. + const auto firstIterator = std::find(runStartIterator, runEndIterator, i - run.glyphStart); + const auto lastIterator = std::find_if(firstIterator, runEndIterator, [iGlyph = (i - run.glyphStart)](auto j) -> bool { return j > iGlyph; }); + + // Add all allocated column counts together. + for (auto j = firstIterator; j < lastIterator; j++) + { + const auto charIndex = std::distance(_glyphClusters.begin(), j); + columns += _textClusterColumns.at(charIndex); + } + + // See if we need to find the end of glyph cluster + if (i + 1 < (run.glyphStart + run.glyphCount) && + (runEndIterator == lastIterator || *lastIterator > (i - run.glyphStart + 1))) + { + if (runEndIterator == lastIterator) + { + // We are not yet at the end of the glyph run, but nothing in the cluster array maps to the next glyph. + // This means everything until end of run belongs to the same glyph cluster. + i = run.glyphStart + run.glyphCount - 1; + } + else + { + // We found the starting index of next glyph cluster. + // Decrement by one will get us the end of current cluster. + i = run.glyphStart + *lastIterator - 1; + } + } + } + + // At this point, we have: + // - iClusterBegin: index of the first glyph in the glyph cluster + // - i: index of the last glyph in the glyph cluster + // - columns: number of terminal columns this cluster should occupy + // Advance is how wide in pixels the glyph is auto& advance = _glyphAdvances.at(i); - // Offsets is how far to move the origin (in pixels) from where it is - auto& offset = _glyphOffsets.at(i); - - // Get how many columns we expected the glyph to have and mutiply into pixels. - const auto columns = _textClusterColumns.at(i); - const auto advanceExpected = static_cast(columns * _width); + const auto advanceExpected = static_cast(columns* _width); // If what we expect is bigger than what we have... pad it out. if (advanceExpected > advance) @@ -429,7 +477,9 @@ try // Move the X offset (pixels to the right from the left edge) by half the excess space // so half of it will be left of the glyph and the other half on the right. - offset.advanceOffset += diff / 2; + // Here we need to move every glyph in the cluster. + for (auto j = iClusterBegin; j <= i; j++) + _glyphOffsets.at(j).advanceOffset += diff / 2; // Set the advance to the perfect width we want. advance = advanceExpected; @@ -458,7 +508,7 @@ try // in case two adjacent glyphs need the same scale factor. _glyphScaleCorrections.push_back(std::tuple{ i, scaleProposed }); - // Set the advance to the perfect width that we want before figuring out if the scale factor doesn't match this run + // Set the advance to the perfect width that we want. advance = advanceExpected; } } From b49084718e66837667569ef65920500fa057e4d0 Mon Sep 17 00:00:00 2001 From: Michael Niksa Date: Thu, 27 Feb 2020 16:10:37 -0800 Subject: [PATCH 03/17] Allows sum of all advances for glyphs used in these columns to be considered for the column space allocated. Applies excess pen advance space to the final one only. --- src/renderer/dx/CustomTextLayout.cpp | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/src/renderer/dx/CustomTextLayout.cpp b/src/renderer/dx/CustomTextLayout.cpp index 4441ac4f487..441bf4967b6 100644 --- a/src/renderer/dx/CustomTextLayout.cpp +++ b/src/renderer/dx/CustomTextLayout.cpp @@ -465,24 +465,34 @@ try // - columns: number of terminal columns this cluster should occupy // Advance is how wide in pixels the glyph is + // Prior is for all pieces of the glyph run before the last one, which we might pad out. + const auto advancePrior = std::accumulate(_glyphAdvances.cbegin() + iClusterBegin, _glyphAdvances.cbegin() + i, 0.0f); + + // This is the last one in the run which we may pad out. auto& advance = _glyphAdvances.at(i); - const auto advanceExpected = static_cast(columns* _width); + const auto advanceTotal = advancePrior + advance; + + // This is how many columns we expected it to take based on the text buffer's representation. + const auto advanceExpected = static_cast(columns * _width); // If what we expect is bigger than what we have... pad it out. - if (advanceExpected > advance) + if (advanceExpected > advanceTotal) { // Get the amount of space we have leftover. - const auto diff = advanceExpected - advance; + const auto diff = advanceExpected - advanceTotal; // Move the X offset (pixels to the right from the left edge) by half the excess space // so half of it will be left of the glyph and the other half on the right. // Here we need to move every glyph in the cluster. for (auto j = iClusterBegin; j <= i; j++) + { _glyphOffsets.at(j).advanceOffset += diff / 2; + } - // Set the advance to the perfect width we want. - advance = advanceExpected; + // Set the advance of the final glyph in the set to all excess space not consumed by the first few so + // we get the perfect width we want. + advance = advanceExpected - advancePrior; } // If what we expect is smaller than what we have... rescale the font size to get a smaller glyph to fit. else if (advanceExpected < advance) From 98dcba94d098db7aa5bcc3f074141f60a8acd94b Mon Sep 17 00:00:00 2001 From: Michael Niksa Date: Fri, 28 Feb 2020 10:09:11 -0800 Subject: [PATCH 04/17] Use struct for scale corrections. Move them to use correct index (text, not glyph). Provide length to corrections as well. Change scaling down algorithm to be based on advances we already know. Correct remaining advance distances to deal with multiple glyph clusters. --- src/renderer/dx/CustomTextLayout.cpp | 139 +++++++++++++++------------ src/renderer/dx/CustomTextLayout.h | 9 +- 2 files changed, 83 insertions(+), 65 deletions(-) diff --git a/src/renderer/dx/CustomTextLayout.cpp b/src/renderer/dx/CustomTextLayout.cpp index 441bf4967b6..fc2c7d8470d 100644 --- a/src/renderer/dx/CustomTextLayout.cpp +++ b/src/renderer/dx/CustomTextLayout.cpp @@ -359,26 +359,26 @@ CustomTextLayout::CustomTextLayout(gsl::not_null const factory } // If scale corrections were needed, we need to split the run. - for (auto [index, scale] : _glyphScaleCorrections) + for (auto& c : _glyphScaleCorrections) { // Don't split in the middle of a surrogate pair. - const auto after = IS_HIGH_SURROGATE(_text.at(index)) ? 2 : 1; + /*const auto after = IS_HIGH_SURROGATE(_text.at(c.index)) ? 2 : 1;*/ // Split after the adjustment first so it // takes a copy of all the run properties before we modify them. // GH 4665: This is the other half of the potential future perf item. // If glyphs needing the same scale are coalesced, we could // break fewer times and have fewer runs. - _SetCurrentRun(index + after); - _SplitCurrentRun(index + after); + _SetCurrentRun(c.textIndex + c.textLength); + _SplitCurrentRun(c.textIndex + c.textLength); // Now split just this glyph off. - _SetCurrentRun(index); - _SplitCurrentRun(index); + _SetCurrentRun(c.textIndex); + _SplitCurrentRun(c.textIndex); // Get the run with the one glyph and adjust the scale. auto& run = _GetCurrentRun(); - run.fontScale = scale; + run.fontScale = c.scale; } _OrderRuns(); @@ -415,47 +415,46 @@ try // Get how many columns we expected the glyph to have and multiply into pixels. UINT16 columns = 0; + + // Because of typographic features such as ligatures, it is well possible for a cluster of glyph to + // represent multiple code points. Previous calls to IDWriteTextAnalyzer::GetGlyphs stores the mapping + // information between code points and glyphs in _glyphClusters. + // To properly allocate the columns for such glyph clusters, we need to find all characters that this + // cluster is representing and add column counts for all the characters together. + // Inside a glyph cluster, we should try our best to respect the advances and offsets to keep glyphs properly + // positioned. This means the glyph offset adjustment should be applied to all glyphs in cluster, and the + // column-based advance adjustment should be applied on the last glyph of the cluster. + + // Find the range for current glyph run in _glyphClusters. + const auto runStartIterator = _glyphClusters.begin() + run.textStart; + const auto runEndIterator = _glyphClusters.begin() + run.textStart + run.textLength; + + // Find the range of characters that the current glyph is representing. + const auto firstIterator = std::find(runStartIterator, runEndIterator, i - run.glyphStart); + const auto lastIterator = std::find_if(firstIterator, runEndIterator, [iGlyph = (i - run.glyphStart)](auto j) -> bool { return j > iGlyph; }); + + // Add all allocated column counts together. + for (auto j = firstIterator; j < lastIterator; j++) + { + const auto charIndex = std::distance(_glyphClusters.begin(), j); + columns += _textClusterColumns.at(charIndex); + } + + // See if we need to find the end of glyph cluster + if (i + 1 < (run.glyphStart + run.glyphCount) && + (runEndIterator == lastIterator || *lastIterator > (i - run.glyphStart + 1))) { - // Because of typographic features such as ligatures, it is well possible for a cluster of glyph to - // represent multiple code points. Previous calls to IDWriteTextAnalyzer::GetGlyphs stores the mapping - // information between code points and glyphs in _glyphClusters. - // To properly allocate the columns for such glyph clusters, we need to find all characters that this - // cluster is representing and add column counts for all the characters together. - // Inside a glyph cluster, we should try our best to respect the advances and offsets to keep glyphs properly - // positioned. This means the glyph offset adjustment should be applied to all glyphs in cluster, and the - // column-based advance adjustment should be applied on the last glyph of the cluster. - - // Find the range for current glyph run in _glyphClusters. - const auto runStartIterator = _glyphClusters.begin() + run.textStart; - const auto runEndIterator = _glyphClusters.begin() + run.textStart + run.textLength; - - // Find the range of characters that the current glyph is representing. - const auto firstIterator = std::find(runStartIterator, runEndIterator, i - run.glyphStart); - const auto lastIterator = std::find_if(firstIterator, runEndIterator, [iGlyph = (i - run.glyphStart)](auto j) -> bool { return j > iGlyph; }); - - // Add all allocated column counts together. - for (auto j = firstIterator; j < lastIterator; j++) + if (runEndIterator == lastIterator) { - const auto charIndex = std::distance(_glyphClusters.begin(), j); - columns += _textClusterColumns.at(charIndex); + // We are not yet at the end of the glyph run, but nothing in the cluster array maps to the next glyph. + // This means everything until end of run belongs to the same glyph cluster. + i = run.glyphStart + run.glyphCount - 1; } - - // See if we need to find the end of glyph cluster - if (i + 1 < (run.glyphStart + run.glyphCount) && - (runEndIterator == lastIterator || *lastIterator > (i - run.glyphStart + 1))) + else { - if (runEndIterator == lastIterator) - { - // We are not yet at the end of the glyph run, but nothing in the cluster array maps to the next glyph. - // This means everything until end of run belongs to the same glyph cluster. - i = run.glyphStart + run.glyphCount - 1; - } - else - { - // We found the starting index of next glyph cluster. - // Decrement by one will get us the end of current cluster. - i = run.glyphStart + *lastIterator - 1; - } + // We found the starting index of next glyph cluster. + // Decrement by one will get us the end of current cluster. + i = run.glyphStart + *lastIterator - 1; } } @@ -495,31 +494,43 @@ try advance = advanceExpected - advancePrior; } // If what we expect is smaller than what we have... rescale the font size to get a smaller glyph to fit. - else if (advanceExpected < advance) + else if (advanceExpected < advanceTotal) { - // We need to retrieve the design information for this specific glyph so we can figure out the appropriate - // height proportional to the width that we desire. - INT32 advanceInDesignUnits; - RETURN_IF_FAILED(run.fontFace->GetDesignGlyphAdvances(1, &_glyphIndices.at(i), &advanceInDesignUnits)); - - // When things are drawn, we want the font size (as specified in the base font in the original format) - // to be scaled by some factor. - // i.e. if the original font size was 16, we might want to draw this glyph with a 15.2 size font so - // the width (and height) of the glyph will shrink to fit the monospace cell box. - - // This pattern is copied from the DxRenderer's algorithm for figuring out the font height for a specific width - // and was advised by the DirectWrite team. - const float widthAdvance = static_cast(advanceInDesignUnits) / metrics.designUnitsPerEm; - const auto fontSizeWant = advanceExpected / widthAdvance; - const auto scaleProposed = fontSizeWant / _format->GetFontSize(); + //// We need to retrieve the design information for this specific glyph so we can figure out the appropriate + //// height proportional to the width that we desire. + //INT32 advanceInDesignUnits; + //RETURN_IF_FAILED(run.fontFace->GetDesignGlyphAdvances(1, &_glyphIndices.at(i), &advanceInDesignUnits)); + + //// When things are drawn, we want the font size (as specified in the base font in the original format) + //// to be scaled by some factor. + //// i.e. if the original font size was 16, we might want to draw this glyph with a 15.2 size font so + //// the width (and height) of the glyph will shrink to fit the monospace cell box. + + //// This pattern is copied from the DxRenderer's algorithm for figuring out the font height for a specific width + //// and was advised by the DirectWrite team. + //const float widthAdvance = static_cast(advanceInDesignUnits) / metrics.designUnitsPerEm; + //const auto fontSizeWant = advanceExpected / widthAdvance; + //const auto fontSizeWas = _format->GetFontSize(); + //const auto scaleProposed = fontSizeWant / fontSizeWas; + + const auto scaleProposed = advanceExpected / advanceTotal; + + const auto textIndex = gsl::narrow(std::distance(_glyphClusters.begin(), firstIterator)); + const auto textLength = gsl::narrow(std::distance(firstIterator, lastIterator)); // Store the glyph scale correction for future run breaking // GH 4665: In theory, we could also store the length of the new run and coalesce // in case two adjacent glyphs need the same scale factor. - _glyphScaleCorrections.push_back(std::tuple{ i, scaleProposed }); + _glyphScaleCorrections.push_back(ScaleCorrection{ textIndex, textLength, scaleProposed }); + + // Adjust all relevant advances by the scale factor. + for (auto j = iClusterBegin; j <= i; j++) + { + _glyphAdvances.at(j) *= scaleProposed; + } - // Set the advance to the perfect width that we want. - advance = advanceExpected; + //// Set the advance to the perfect width that we want. + //advance = advanceExpected; } } @@ -1169,7 +1180,7 @@ void CustomTextLayout::_SplitCurrentRun(const UINT32 splitPosition) frontHalf.glyphCount = mapOffset; // The back half starts at the index that's one past the end of the front - backHalf.glyphStart = mapOffset; + backHalf.glyphStart += mapOffset; // And the back half count (since it was copied from the front half above) // now just needs to be subtracted by how many we gave the front half. diff --git a/src/renderer/dx/CustomTextLayout.h b/src/renderer/dx/CustomTextLayout.h index 99d99ae1b19..02168b7dbc8 100644 --- a/src/renderer/dx/CustomTextLayout.h +++ b/src/renderer/dx/CustomTextLayout.h @@ -180,8 +180,15 @@ namespace Microsoft::Console::Render std::vector _glyphAdvances; + struct ScaleCorrection + { + UINT32 textIndex; + UINT32 textLength; + float scale; + }; + // These are used to further break the runs apart and adjust the font size so glyphs fit inside the cells. - std::vector> _glyphScaleCorrections; + std::vector _glyphScaleCorrections; #ifdef UNIT_TESTING public: From ec1d4fc2f084f360a40058b8ad7552c21a57b5db Mon Sep 17 00:00:00 2001 From: Michael Niksa Date: Fri, 28 Feb 2020 10:34:21 -0800 Subject: [PATCH 05/17] Put 0s for the columns on a piece of text bigger than one char. --- src/renderer/dx/CustomTextLayout.cpp | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/src/renderer/dx/CustomTextLayout.cpp b/src/renderer/dx/CustomTextLayout.cpp index fc2c7d8470d..ed501cf8315 100644 --- a/src/renderer/dx/CustomTextLayout.cpp +++ b/src/renderer/dx/CustomTextLayout.cpp @@ -45,8 +45,19 @@ CustomTextLayout::CustomTextLayout(gsl::not_null const factory for (const auto& cluster : clusters) { const auto cols = gsl::narrow(cluster.GetColumns()); + const auto text = cluster.GetText(); + + // Push back the number of columns for this bit of text. _textClusterColumns.push_back(cols); - _text += cluster.GetText(); + + // If there is more than one text character here, push 0s for the rest of the columns + // of the text run. + for (auto i = 1; i < text.size(); ++i) + { + _textClusterColumns.push_back(0); + } + + _text += text; } } From c7d5a95a12e9e89a4baf282033e6dc1ae7f73abf Mon Sep 17 00:00:00 2001 From: Michael Niksa Date: Fri, 28 Feb 2020 10:59:45 -0800 Subject: [PATCH 06/17] Add a bunch of comments. Delete a bunch of old code. --- src/renderer/dx/CustomTextLayout.cpp | 112 +++++++++++++++++++++------ 1 file changed, 89 insertions(+), 23 deletions(-) diff --git a/src/renderer/dx/CustomTextLayout.cpp b/src/renderer/dx/CustomTextLayout.cpp index ed501cf8315..08dc46a4f43 100644 --- a/src/renderer/dx/CustomTextLayout.cpp +++ b/src/renderer/dx/CustomTextLayout.cpp @@ -380,16 +380,102 @@ CustomTextLayout::CustomTextLayout(gsl::not_null const factory // GH 4665: This is the other half of the potential future perf item. // If glyphs needing the same scale are coalesced, we could // break fewer times and have fewer runs. - _SetCurrentRun(c.textIndex + c.textLength); - _SplitCurrentRun(c.textIndex + c.textLength); + + // Example + // Text: + // ABCDEFGHIJKLMNOPQRSTUVWXYZ + // LEN = 26 + // Runs: + // ^0----^1---------^2------- + // Scale Factors: + // 1.0 1.0 1.0 + // (arrows are run begin) + // 0: IDX = 0, LEN = 6 + // 1: IDX = 6, LEN = 11 + // 2: IDX = 17, LEN = 9 + + // From the scale correction... we get + // IDX = where the scale starts + // LEN = how long the scale adjustment runs + // SCALE = the scale factor. + + // We need to split the run so the SCALE factor + // only applies from IDX to LEN. + + // This is the index after the segment we're splitting. + const auto afterIndex = c.textIndex + c.textLength; + + // If the after index is still within the text, split the back + // half off first so we don't apply the scale factor to anything + // after this glyph/run segment. + // Example relative to above sample state: + // Correction says: IDX = 12, LEN = 2, FACTOR = 0.8 + // We must split off first at 14 to leave the existing factor from 14-16. + // (because the act of splitting copies all properties, we don't want to + // adjust the scale factor BEFORE splitting off the existing one.) + // Text: + // ABCDEFGHIJKLMNOPQRSTUVWXYZ + // LEN = 26 + // Runs: + // ^0----^1----xx^2-^3------- + // (xx is where we're going to put the correction when all is said and done. + // We're adjusting the scale of the "MN" text only.) + // Scale Factors: + // 1 1 1 1 + // (arrows are run begin) + // 0: IDX = 0, LEN = 6 + // 1: IDX = 6, LEN = 8 + // 2: IDX = 14, LEN = 3 + // 3: IDX = 17, LEN = 9 + if (afterIndex < _text.size()) + { + _SetCurrentRun(afterIndex); + _SplitCurrentRun(afterIndex); + } + // If it's after the text, don't bother. The correction will just apply + // from the begin point to the end of the text. + // Example relative to above sample state: + // Correction says: IDX = 24, LEN = 2 + // Text: + // ABCDEFGHIJKLMNOPQRSTUVWXYZ + // LEN = 26 + // Runs: + // ^0----^1---------^2-----xx + // xx is where we're going to put the correction when all is said and done. + // We don't need to split off the back portion because there's nothing after the xx. // Now split just this glyph off. + // Example versus the one above where we did already split the back half off.. + // Correction says: IDX = 12, LEN = 2, FACTOR = 0.8 + // Text: + // ABCDEFGHIJKLMNOPQRSTUVWXYZ + // LEN = 26 + // Runs: + // ^0----^1----^2^3-^4------- + // (The MN text has been broken into its own run, 2.) + // Scale Factors: + // 1 1 1 1 1 + // (arrows are run begin) + // 0: IDX = 0, LEN = 6 + // 1: IDX = 6, LEN = 6 + // 2: IDX = 12, LEN = 2 + // 2: IDX = 14, LEN = 3 + // 3: IDX = 17, LEN = 9 _SetCurrentRun(c.textIndex); _SplitCurrentRun(c.textIndex); - + // Get the run with the one glyph and adjust the scale. auto& run = _GetCurrentRun(); run.fontScale = c.scale; + // Correction says: IDX = 12, LEN = 2, FACTOR = 0.8 + // Text: + // ABCDEFGHIJKLMNOPQRSTUVWXYZ + // LEN = 26 + // Runs: + // ^0----^1----^2^3-^4------- + // (We've now only corrected run 2, selecting only the MN to 0.8) + // Scale Factors: + // 1 1 .8 1 1 } _OrderRuns(); @@ -507,23 +593,6 @@ try // If what we expect is smaller than what we have... rescale the font size to get a smaller glyph to fit. else if (advanceExpected < advanceTotal) { - //// We need to retrieve the design information for this specific glyph so we can figure out the appropriate - //// height proportional to the width that we desire. - //INT32 advanceInDesignUnits; - //RETURN_IF_FAILED(run.fontFace->GetDesignGlyphAdvances(1, &_glyphIndices.at(i), &advanceInDesignUnits)); - - //// When things are drawn, we want the font size (as specified in the base font in the original format) - //// to be scaled by some factor. - //// i.e. if the original font size was 16, we might want to draw this glyph with a 15.2 size font so - //// the width (and height) of the glyph will shrink to fit the monospace cell box. - - //// This pattern is copied from the DxRenderer's algorithm for figuring out the font height for a specific width - //// and was advised by the DirectWrite team. - //const float widthAdvance = static_cast(advanceInDesignUnits) / metrics.designUnitsPerEm; - //const auto fontSizeWant = advanceExpected / widthAdvance; - //const auto fontSizeWas = _format->GetFontSize(); - //const auto scaleProposed = fontSizeWant / fontSizeWas; - const auto scaleProposed = advanceExpected / advanceTotal; const auto textIndex = gsl::narrow(std::distance(_glyphClusters.begin(), firstIterator)); @@ -539,9 +608,6 @@ try { _glyphAdvances.at(j) *= scaleProposed; } - - //// Set the advance to the perfect width that we want. - //advance = advanceExpected; } } From 799a144682ac772fa4f200b8a28f0f24b61a76ea Mon Sep 17 00:00:00 2001 From: Michael Niksa Date: Fri, 28 Feb 2020 11:00:41 -0800 Subject: [PATCH 07/17] Don't need surrogate fix anymore with col fix. --- src/renderer/dx/CustomTextLayout.cpp | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/renderer/dx/CustomTextLayout.cpp b/src/renderer/dx/CustomTextLayout.cpp index 08dc46a4f43..df4a1ad52e8 100644 --- a/src/renderer/dx/CustomTextLayout.cpp +++ b/src/renderer/dx/CustomTextLayout.cpp @@ -372,9 +372,6 @@ CustomTextLayout::CustomTextLayout(gsl::not_null const factory // If scale corrections were needed, we need to split the run. for (auto& c : _glyphScaleCorrections) { - // Don't split in the middle of a surrogate pair. - /*const auto after = IS_HIGH_SURROGATE(_text.at(c.index)) ? 2 : 1;*/ - // Split after the adjustment first so it // takes a copy of all the run properties before we modify them. // GH 4665: This is the other half of the potential future perf item. From cf8d0d4ef144c86b5b69390c70e3b106a78b9266 Mon Sep 17 00:00:00 2001 From: Michael Niksa Date: Fri, 28 Feb 2020 14:28:18 -0800 Subject: [PATCH 08/17] Rewrite algorithm and add tons of comments to attempt to make it easier to understand in regards to indexes/offsets. --- src/renderer/dx/CustomTextLayout.cpp | 240 +++++++++++++++++---------- 1 file changed, 156 insertions(+), 84 deletions(-) diff --git a/src/renderer/dx/CustomTextLayout.cpp b/src/renderer/dx/CustomTextLayout.cpp index df4a1ad52e8..cfa0e1498c3 100644 --- a/src/renderer/dx/CustomTextLayout.cpp +++ b/src/renderer/dx/CustomTextLayout.cpp @@ -398,7 +398,7 @@ CustomTextLayout::CustomTextLayout(gsl::not_null const factory // We need to split the run so the SCALE factor // only applies from IDX to LEN. - + // This is the index after the segment we're splitting. const auto afterIndex = c.textIndex + c.textLength; @@ -418,7 +418,7 @@ CustomTextLayout::CustomTextLayout(gsl::not_null const factory // (xx is where we're going to put the correction when all is said and done. // We're adjusting the scale of the "MN" text only.) // Scale Factors: - // 1 1 1 1 + // 1 1 1 1 // (arrows are run begin) // 0: IDX = 0, LEN = 6 // 1: IDX = 6, LEN = 8 @@ -460,7 +460,7 @@ CustomTextLayout::CustomTextLayout(gsl::not_null const factory // 3: IDX = 17, LEN = 9 _SetCurrentRun(c.textIndex); _SplitCurrentRun(c.textIndex); - + // Get the run with the one glyph and adjust the scale. auto& run = _GetCurrentRun(); run.fontScale = c.scale; @@ -502,110 +502,182 @@ try DWRITE_FONT_METRICS1 metrics; run.fontFace->GetMetrics(&metrics); - // Walk through advances and space out characters that are too small to consume their box. - for (auto i = run.glyphStart; i < (run.glyphStart + run.glyphCount); i++) + // Glyph Indicies represents the number inside the selected font where the glyph image/paths are found. + // Text represents the original text we gave in. + // Glyph Clusters represents the map between Text and Glyph Indicies. + // - There is one Glyph Clusters map column per character of text. + // - The value of the cluster at any given position is relative to the 0 index of this run. + // (a.k.a. it resets to 0 for every run) + // - If multiple Glyph Cluster map values point to the same index, then multiple text chars were used + // to create the same glyph cluster. + // - The delta between the value from one Glyph Cluster's value and the next one is how many + // Glyph Indicies are consumed to make that cluster. + + // We're going to walk the map to find what spans of text and glyph indices make one cluster. + const auto clusterMapBegin = _glyphClusters.cbegin() + run.textStart; + const auto clusterMapEnd = clusterMapBegin + run.textLength; + + // Walk through every glyph in the run, collect them into clusters, then adjust them to fit in + // however many columns are expected for display by the text buffer. + for (auto clusterBegin = clusterMapBegin; clusterBegin < clusterMapEnd; /* we will increment this inside the loop*/) { - const auto iClusterBegin = i; - - // Get how many columns we expected the glyph to have and multiply into pixels. - UINT16 columns = 0; - - // Because of typographic features such as ligatures, it is well possible for a cluster of glyph to - // represent multiple code points. Previous calls to IDWriteTextAnalyzer::GetGlyphs stores the mapping - // information between code points and glyphs in _glyphClusters. - // To properly allocate the columns for such glyph clusters, we need to find all characters that this - // cluster is representing and add column counts for all the characters together. - // Inside a glyph cluster, we should try our best to respect the advances and offsets to keep glyphs properly - // positioned. This means the glyph offset adjustment should be applied to all glyphs in cluster, and the - // column-based advance adjustment should be applied on the last glyph of the cluster. - - // Find the range for current glyph run in _glyphClusters. - const auto runStartIterator = _glyphClusters.begin() + run.textStart; - const auto runEndIterator = _glyphClusters.begin() + run.textStart + run.textLength; - - // Find the range of characters that the current glyph is representing. - const auto firstIterator = std::find(runStartIterator, runEndIterator, i - run.glyphStart); - const auto lastIterator = std::find_if(firstIterator, runEndIterator, [iGlyph = (i - run.glyphStart)](auto j) -> bool { return j > iGlyph; }); - - // Add all allocated column counts together. - for (auto j = firstIterator; j < lastIterator; j++) - { - const auto charIndex = std::distance(_glyphClusters.begin(), j); - columns += _textClusterColumns.at(charIndex); - } - - // See if we need to find the end of glyph cluster - if (i + 1 < (run.glyphStart + run.glyphCount) && - (runEndIterator == lastIterator || *lastIterator > (i - run.glyphStart + 1))) - { - if (runEndIterator == lastIterator) - { - // We are not yet at the end of the glyph run, but nothing in the cluster array maps to the next glyph. - // This means everything until end of run belongs to the same glyph cluster. - i = run.glyphStart + run.glyphCount - 1; - } - else - { - // We found the starting index of next glyph cluster. - // Decrement by one will get us the end of current cluster. - i = run.glyphStart + *lastIterator - 1; - } - } - - // At this point, we have: - // - iClusterBegin: index of the first glyph in the glyph cluster - // - i: index of the last glyph in the glyph cluster - // - columns: number of terminal columns this cluster should occupy - - // Advance is how wide in pixels the glyph is - // Prior is for all pieces of the glyph run before the last one, which we might pad out. - const auto advancePrior = std::accumulate(_glyphAdvances.cbegin() + iClusterBegin, _glyphAdvances.cbegin() + i, 0.0f); - - // This is the last one in the run which we may pad out. - auto& advance = _glyphAdvances.at(i); - - const auto advanceTotal = advancePrior + advance; - - // This is how many columns we expected it to take based on the text buffer's representation. + // One or more glyphs might belong to a single cluster. + // Consider the following examples: + + // 1. + // U+00C1 is Á. + // That is text of length one. + // A given font might have a complete single glyph for this + // which will be mapped into the _glyphIndicies array. + // _text[0] = Á + // _glyphIndicies[0] = 153 + // _glyphClusters[0] = 0 + // _glyphClusters[1] = 1 + // The delta between the value of Clusters 0 and 1 is 1. + // The number of times "0" is specified is once. + // This means that we've represented one text with one glyph. + + // 2. + // U+0041 is A and U+0301 is a combinine acute accent ´. + // That is a text length of two. + // A given font might have two glyphs for this + // which will be mapped into the _glyphIndicies array. + // _text[0] = A + // _text[1] = ´ (U+0301, combining acute) + // _glyphIndicies[0] = 153 + // _glyphIndicies[1] = 421 + // _glyphClusters[0] = 0 + // _glyphClusters[1] = 0 + // _glyphClusters[2] = 2 + // The delta between the value of Clusters 0/1 and 2 is 2. + // The number of times "0" is specified is twice. + // This means that we've represented two text with two glyphs. + + // There are two more scenarios that can occur that get us into + // NxM territory (N text by M glyphs) + + // 3. + // U+00C1 is Á. + // That is text of length one. + // A given font might represent this as two glyphs + // which will be mapped into the _glyphIndicies array. + // _text[0] = Á + // _glyphIndicies[0] = 153 + // _glyphIndicies[1] = 421 + // _glyphClusters[0] = 0 + // _glyphClusters[1] = 2 + // The delta between the value of Clusters 0 and 1 is 2. + // The number of times "0" is specified is once. + // This means that we've represented one text with two glyphs. + + // 4. + // U+0041 is A and U+0301 is a combinine acute accent ´. + // That is a text length of two. + // A given font might represent this as one glyph + // which will be mapped into the _glyphIndicies array. + // _text[0] = A + // _text[1] = ´ (U+0301, combining acute) + // _glyphIndicies[0] = 984 + // _glyphClusters[0] = 0 + // _glyphClusters[1] = 0 + // _glyphClusters[2] = 1 + // The delta between the value of Clusters 0/1 and 2 is 1. + // The number of times "0" is specified is twice. + // This means that we've represented two text with one glyph. + + // Finally, there's one more dimension. + // Due to supporting a specific coordinate system, the text buffer + // has told us how many columns it expects the text it gave us to take + // when displayed. + // That is stored in _textClusterColumns with one value for each + // character in the _text array. + // It isn't aware of how glyphs actually get mapped. + // So it's giving us a column count in terms of text characters + // but expecting it to be applied to all the glyphs in the cluster + // required to represent that text. + // We'll collect that up and use it at the end to adjust our drawing. + + // Our goal below is to walk through and figure out... + // A. How many glyphs belong to this cluster? + // B. Which text characters belong with those glyphs? + // C. How many columns, in total, were we told we could use + // to draw the glyphs? + + // This is the value under the beginning position in the map. + const auto clusterValue = *clusterBegin; + + // Find the cluster end point inside the map. + // We want to walk forward in the map until it changes (or we reach the end). + const auto clusterEnd = std::find_if(clusterBegin, clusterMapEnd, [clusterValue](auto compareVal) -> bool { return clusterValue != compareVal; }); + + // The beginning of the text span is just how far the beginning of the cluster is into the map. + const auto clusterTextBegin = std::distance(_glyphClusters.cbegin(), clusterBegin); + + // The distance from beginning to end is the cluster text length. + const auto clusterTextLength = std::distance(clusterBegin, clusterEnd); + + // The beginning of the glyph span is just the original cluster value. + const auto clusterGlyphBegin = clusterValue + run.glyphStart; + + // The difference between the value inside the end iterator and the original value is the glyph length. + // If the end iterator was off the end of the map, then it's the total run glyph count minus wherever we started. + const auto clusterGlyphLength = (clusterEnd != clusterMapEnd ? *clusterEnd : run.glyphCount) - clusterValue; + + // Now we can specify the spans within the text-index and glyph-index based vectors + // that store our drawing metadata. + // All the text ones run [clusterTextBegin, clusterTextBegin + clusterTextLength) + // All the cluster ones run [clusterGlyphBegin, clusterGlyphBegin + clusterGlyphLength) + + // Get how many columns we expected the glyph to have. + const auto columns = base::saturated_cast(std::accumulate(_textClusterColumns.cbegin() + clusterTextBegin, + _textClusterColumns.cbegin() + clusterTextBegin + clusterTextLength, + 0u)); + + // Multiply into pixels to get the "advance" we expect this text/glyphs to take when drawn. const auto advanceExpected = static_cast(columns * _width); + // Sum up the advances across the entire cluster to find what the actual value is that we've been told. + const auto advanceActual = std::accumulate(_glyphAdvances.cbegin() + clusterGlyphBegin, + _glyphAdvances.cbegin() + clusterGlyphBegin + clusterGlyphLength, + 0.0f); + // If what we expect is bigger than what we have... pad it out. - if (advanceExpected > advanceTotal) + if (advanceExpected > advanceActual) { // Get the amount of space we have leftover. - const auto diff = advanceExpected - advanceTotal; + const auto diff = advanceExpected - advanceActual; // Move the X offset (pixels to the right from the left edge) by half the excess space // so half of it will be left of the glyph and the other half on the right. // Here we need to move every glyph in the cluster. - for (auto j = iClusterBegin; j <= i; j++) - { - _glyphOffsets.at(j).advanceOffset += diff / 2; - } + std::for_each_n(_glyphOffsets.begin() + clusterGlyphBegin, + clusterGlyphLength, + [halfDiff = diff / 2](DWRITE_GLYPH_OFFSET& offset) -> void { offset.advanceOffset += halfDiff; }); // Set the advance of the final glyph in the set to all excess space not consumed by the first few so // we get the perfect width we want. - advance = advanceExpected - advancePrior; + _glyphAdvances.at((size_t)clusterGlyphBegin + clusterGlyphLength - 1) += diff; } // If what we expect is smaller than what we have... rescale the font size to get a smaller glyph to fit. - else if (advanceExpected < advanceTotal) + else if (advanceExpected < advanceActual) { - const auto scaleProposed = advanceExpected / advanceTotal; - - const auto textIndex = gsl::narrow(std::distance(_glyphClusters.begin(), firstIterator)); - const auto textLength = gsl::narrow(std::distance(firstIterator, lastIterator)); + const auto scaleProposed = advanceExpected / advanceActual; // Store the glyph scale correction for future run breaking // GH 4665: In theory, we could also store the length of the new run and coalesce // in case two adjacent glyphs need the same scale factor. - _glyphScaleCorrections.push_back(ScaleCorrection{ textIndex, textLength, scaleProposed }); + _glyphScaleCorrections.push_back(ScaleCorrection{ + gsl::narrow(clusterTextBegin), + gsl::narrow(clusterTextLength), + scaleProposed }); // Adjust all relevant advances by the scale factor. - for (auto j = iClusterBegin; j <= i; j++) - { - _glyphAdvances.at(j) *= scaleProposed; - } + std::for_each_n(_glyphAdvances.begin() + clusterGlyphBegin, + clusterGlyphLength, + [scaleProposed](float& advance) -> void { advance *= scaleProposed; }); } + + clusterBegin = clusterEnd; } // Certain fonts, like Batang, contain glyphs for hidden control From f5c90cb0d05ae8320113ce0608869956f0b8df35 Mon Sep 17 00:00:00 2001 From: Michael Niksa Date: Fri, 28 Feb 2020 14:31:33 -0800 Subject: [PATCH 09/17] fix analysis warnings. --- src/renderer/dx/CustomTextLayout.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/renderer/dx/CustomTextLayout.cpp b/src/renderer/dx/CustomTextLayout.cpp index cfa0e1498c3..b19bd1626cd 100644 --- a/src/renderer/dx/CustomTextLayout.cpp +++ b/src/renderer/dx/CustomTextLayout.cpp @@ -519,6 +519,7 @@ try // Walk through every glyph in the run, collect them into clusters, then adjust them to fit in // however many columns are expected for display by the text buffer. +#pragma warning(suppress : 26496) // clusterBegin is updated at the bottom of the loop but analysis isn't seeing it. for (auto clusterBegin = clusterMapBegin; clusterBegin < clusterMapEnd; /* we will increment this inside the loop*/) { // One or more glyphs might belong to a single cluster. @@ -656,7 +657,7 @@ try // Set the advance of the final glyph in the set to all excess space not consumed by the first few so // we get the perfect width we want. - _glyphAdvances.at((size_t)clusterGlyphBegin + clusterGlyphLength - 1) += diff; + _glyphAdvances.at(static_cast(clusterGlyphBegin) + clusterGlyphLength - 1) += diff; } // If what we expect is smaller than what we have... rescale the font size to get a smaller glyph to fit. else if (advanceExpected < advanceActual) From e557af4d27e66aa91cecd21bc29ec6ac28497404 Mon Sep 17 00:00:00 2001 From: Michael Niksa Date: Fri, 28 Feb 2020 14:32:55 -0800 Subject: [PATCH 10/17] typo --- src/renderer/dx/CustomTextLayout.cpp | 34 ++++++++++++++-------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/src/renderer/dx/CustomTextLayout.cpp b/src/renderer/dx/CustomTextLayout.cpp index b19bd1626cd..b40bd914082 100644 --- a/src/renderer/dx/CustomTextLayout.cpp +++ b/src/renderer/dx/CustomTextLayout.cpp @@ -502,16 +502,16 @@ try DWRITE_FONT_METRICS1 metrics; run.fontFace->GetMetrics(&metrics); - // Glyph Indicies represents the number inside the selected font where the glyph image/paths are found. + // Glyph Indices represents the number inside the selected font where the glyph image/paths are found. // Text represents the original text we gave in. - // Glyph Clusters represents the map between Text and Glyph Indicies. + // Glyph Clusters represents the map between Text and Glyph Indices. // - There is one Glyph Clusters map column per character of text. // - The value of the cluster at any given position is relative to the 0 index of this run. // (a.k.a. it resets to 0 for every run) // - If multiple Glyph Cluster map values point to the same index, then multiple text chars were used // to create the same glyph cluster. // - The delta between the value from one Glyph Cluster's value and the next one is how many - // Glyph Indicies are consumed to make that cluster. + // Glyph Indices are consumed to make that cluster. // We're going to walk the map to find what spans of text and glyph indices make one cluster. const auto clusterMapBegin = _glyphClusters.cbegin() + run.textStart; @@ -529,9 +529,9 @@ try // U+00C1 is Á. // That is text of length one. // A given font might have a complete single glyph for this - // which will be mapped into the _glyphIndicies array. + // which will be mapped into the _glyphIndices array. // _text[0] = Á - // _glyphIndicies[0] = 153 + // _glyphIndices[0] = 153 // _glyphClusters[0] = 0 // _glyphClusters[1] = 1 // The delta between the value of Clusters 0 and 1 is 1. @@ -542,11 +542,11 @@ try // U+0041 is A and U+0301 is a combinine acute accent ´. // That is a text length of two. // A given font might have two glyphs for this - // which will be mapped into the _glyphIndicies array. + // which will be mapped into the _glyphIndices array. // _text[0] = A // _text[1] = ´ (U+0301, combining acute) - // _glyphIndicies[0] = 153 - // _glyphIndicies[1] = 421 + // _glyphIndices[0] = 153 + // _glyphIndices[1] = 421 // _glyphClusters[0] = 0 // _glyphClusters[1] = 0 // _glyphClusters[2] = 2 @@ -561,10 +561,10 @@ try // U+00C1 is Á. // That is text of length one. // A given font might represent this as two glyphs - // which will be mapped into the _glyphIndicies array. + // which will be mapped into the _glyphIndices array. // _text[0] = Á - // _glyphIndicies[0] = 153 - // _glyphIndicies[1] = 421 + // _glyphIndices[0] = 153 + // _glyphIndices[1] = 421 // _glyphClusters[0] = 0 // _glyphClusters[1] = 2 // The delta between the value of Clusters 0 and 1 is 2. @@ -575,10 +575,10 @@ try // U+0041 is A and U+0301 is a combinine acute accent ´. // That is a text length of two. // A given font might represent this as one glyph - // which will be mapped into the _glyphIndicies array. + // which will be mapped into the _glyphIndices array. // _text[0] = A // _text[1] = ´ (U+0301, combining acute) - // _glyphIndicies[0] = 984 + // _glyphIndices[0] = 984 // _glyphClusters[0] = 0 // _glyphClusters[1] = 0 // _glyphClusters[2] = 1 @@ -1268,7 +1268,7 @@ void CustomTextLayout::_SplitCurrentRun(const UINT32 splitPosition) // f i ñ e // CLUSTERMAP (_glyphClusters) // 0 0 1 3 - // GLYPH INDICIES (_glyphIndicies) + // GLYPH INDICIES (_glyphIndices) // 19 81 23 72 // With _runs length = 1 // _runs(0): @@ -1284,7 +1284,7 @@ void CustomTextLayout::_SplitCurrentRun(const UINT32 splitPosition) // f i ñ e // CLUSTERMAP (_glyphClusters) // 0 0 1 3 - // GLYPH INDICIES (_glyphIndicies) + // GLYPH INDICIES (_glyphIndices) // 19 81 23 72 // With _runs length = 2 // _runs(0): @@ -1318,10 +1318,10 @@ void CustomTextLayout::_SplitCurrentRun(const UINT32 splitPosition) // In this case, that's 1 corresponding with the ñ. const auto mapOffset = _glyphClusters.at(backHalf.textStart); - // The front half's glyph start index (position in _glyphIndicies) + // The front half's glyph start index (position in _glyphIndices) // stays the same. - // The front half's glyph count (items in _glyphIndicies to consume) + // The front half's glyph count (items in _glyphIndices to consume) // is the offset value as that's now one past the end of the front half. // (and count is end index + 1) frontHalf.glyphCount = mapOffset; From 3c1cd50b6874fa848df27c78cecd67d4bd9d78a0 Mon Sep 17 00:00:00 2001 From: Michael Niksa Date: Mon, 2 Mar 2020 09:09:58 -0800 Subject: [PATCH 11/17] If we use M for the width instead of UNICODE_SPACE, then a lot of nonmonospaced fonts work by consequence. Monospace fonts should be fine because M and 0x20 should be the same width... --- src/renderer/dx/DxRenderer.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/renderer/dx/DxRenderer.cpp b/src/renderer/dx/DxRenderer.cpp index 21898d5f74d..e939687f18d 100644 --- a/src/renderer/dx/DxRenderer.cpp +++ b/src/renderer/dx/DxRenderer.cpp @@ -1988,7 +1988,7 @@ float DxEngine::GetScaling() const noexcept DWRITE_FONT_METRICS1 fontMetrics; face->GetMetrics(&fontMetrics); - const UINT32 spaceCodePoint = UNICODE_SPACE; + const UINT32 spaceCodePoint = L'M'; UINT16 spaceGlyphIndex; THROW_IF_FAILED(face->GetGlyphIndicesW(&spaceCodePoint, 1, &spaceGlyphIndex)); From 7b3f6e1d3db426298e2159569a0116c7fad76411 Mon Sep 17 00:00:00 2001 From: Michael Niksa Date: Mon, 2 Mar 2020 09:16:53 -0800 Subject: [PATCH 12/17] Use fill instead of loop per PR feedback. --- src/renderer/dx/CustomTextLayout.cpp | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/renderer/dx/CustomTextLayout.cpp b/src/renderer/dx/CustomTextLayout.cpp index b40bd914082..eaf64f11dc1 100644 --- a/src/renderer/dx/CustomTextLayout.cpp +++ b/src/renderer/dx/CustomTextLayout.cpp @@ -52,10 +52,7 @@ CustomTextLayout::CustomTextLayout(gsl::not_null const factory // If there is more than one text character here, push 0s for the rest of the columns // of the text run. - for (auto i = 1; i < text.size(); ++i) - { - _textClusterColumns.push_back(0); - } + std::fill(_textClusterColumns.begin() + 1, _textClusterColumns.end(), gsl::narrow_cast(0u)); _text += text; } From 087e92084a98101f37f8d4bfce20ebfec20e7b30 Mon Sep 17 00:00:00 2001 From: Michael Niksa Date: Mon, 2 Mar 2020 09:19:21 -0800 Subject: [PATCH 13/17] Changing for_each_n to for_each avoids VS 16.4 compiler/optimizer bug with it stomping one of the registers allocated for the N count. --- src/renderer/dx/CustomTextLayout.cpp | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/renderer/dx/CustomTextLayout.cpp b/src/renderer/dx/CustomTextLayout.cpp index eaf64f11dc1..7319101b0f7 100644 --- a/src/renderer/dx/CustomTextLayout.cpp +++ b/src/renderer/dx/CustomTextLayout.cpp @@ -648,9 +648,9 @@ try // Move the X offset (pixels to the right from the left edge) by half the excess space // so half of it will be left of the glyph and the other half on the right. // Here we need to move every glyph in the cluster. - std::for_each_n(_glyphOffsets.begin() + clusterGlyphBegin, - clusterGlyphLength, - [halfDiff = diff / 2](DWRITE_GLYPH_OFFSET& offset) -> void { offset.advanceOffset += halfDiff; }); + std::for_each(_glyphOffsets.begin() + clusterGlyphBegin, + _glyphOffsets.begin() + clusterGlyphBegin + clusterGlyphLength, + [halfDiff = diff / 2](DWRITE_GLYPH_OFFSET& offset) -> void { offset.advanceOffset += halfDiff; }); // Set the advance of the final glyph in the set to all excess space not consumed by the first few so // we get the perfect width we want. @@ -670,9 +670,9 @@ try scaleProposed }); // Adjust all relevant advances by the scale factor. - std::for_each_n(_glyphAdvances.begin() + clusterGlyphBegin, - clusterGlyphLength, - [scaleProposed](float& advance) -> void { advance *= scaleProposed; }); + std::for_each(_glyphAdvances.begin() + clusterGlyphBegin, + _glyphAdvances.begin() + clusterGlyphBegin + clusterGlyphLength, + [scaleProposed](float& advance) -> void { advance *= scaleProposed; }); } clusterBegin = clusterEnd; @@ -1339,7 +1339,7 @@ void CustomTextLayout::_SplitCurrentRun(const UINT32 splitPosition) // slide all the glyph mapping values to be relative to the new // backHalf.glyphStart, or adjust it by the offset we just set it to. const auto updateBegin = _glyphClusters.begin() + backHalf.textStart; - std::for_each_n(updateBegin, backHalf.textLength, [mapOffset](UINT16& n) { + std::for_each(updateBegin, updateBegin + backHalf.textLength, [mapOffset](UINT16& n) { n -= mapOffset; }); } From 99533e447a54ea79ec6ebcb5210eb1569352583b Mon Sep 17 00:00:00 2001 From: Michael Niksa Date: Mon, 2 Mar 2020 09:22:40 -0800 Subject: [PATCH 14/17] Carlos-based spell checking. --- src/renderer/dx/CustomTextLayout.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/renderer/dx/CustomTextLayout.cpp b/src/renderer/dx/CustomTextLayout.cpp index 7319101b0f7..bc620fae6c4 100644 --- a/src/renderer/dx/CustomTextLayout.cpp +++ b/src/renderer/dx/CustomTextLayout.cpp @@ -569,7 +569,7 @@ try // This means that we've represented one text with two glyphs. // 4. - // U+0041 is A and U+0301 is a combinine acute accent ´. + // U+0041 is A and U+0301 is a combining acute accent ´. // That is a text length of two. // A given font might represent this as one glyph // which will be mapped into the _glyphIndices array. @@ -1265,7 +1265,7 @@ void CustomTextLayout::_SplitCurrentRun(const UINT32 splitPosition) // f i ñ e // CLUSTERMAP (_glyphClusters) // 0 0 1 3 - // GLYPH INDICIES (_glyphIndices) + // GLYPH INDICES (_glyphIndices) // 19 81 23 72 // With _runs length = 1 // _runs(0): @@ -1281,7 +1281,7 @@ void CustomTextLayout::_SplitCurrentRun(const UINT32 splitPosition) // f i ñ e // CLUSTERMAP (_glyphClusters) // 0 0 1 3 - // GLYPH INDICIES (_glyphIndices) + // GLYPH INDICES (_glyphIndices) // 19 81 23 72 // With _runs length = 2 // _runs(0): From cdb58e591a5b8cbd0983e0dcab6a813ce7d1da2c Mon Sep 17 00:00:00 2001 From: Michael Niksa Date: Mon, 2 Mar 2020 09:59:04 -0800 Subject: [PATCH 15/17] premature change to fill was a logical problem. this needs to be inserted, so go back to fill_n with the back_inserter as Dustin originally suggested. --- src/renderer/dx/CustomTextLayout.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/renderer/dx/CustomTextLayout.cpp b/src/renderer/dx/CustomTextLayout.cpp index bc620fae6c4..feea11283a5 100644 --- a/src/renderer/dx/CustomTextLayout.cpp +++ b/src/renderer/dx/CustomTextLayout.cpp @@ -52,7 +52,7 @@ CustomTextLayout::CustomTextLayout(gsl::not_null const factory // If there is more than one text character here, push 0s for the rest of the columns // of the text run. - std::fill(_textClusterColumns.begin() + 1, _textClusterColumns.end(), gsl::narrow_cast(0u)); + std::fill_n(std::back_inserter(_textClusterColumns), cols - 1, gsl::narrow_cast(0u)); _text += text; } From a1b8b77a8dfc2bc34a3fa3977a4139db60806755 Mon Sep 17 00:00:00 2001 From: Michael Niksa Date: Mon, 2 Mar 2020 10:01:48 -0800 Subject: [PATCH 16/17] Just... for safety sake. Let's clamp the subtraction here to 0. I don't want an underflow if columns is weirdly 0. --- src/renderer/dx/CustomTextLayout.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/renderer/dx/CustomTextLayout.cpp b/src/renderer/dx/CustomTextLayout.cpp index feea11283a5..4ecb2430a60 100644 --- a/src/renderer/dx/CustomTextLayout.cpp +++ b/src/renderer/dx/CustomTextLayout.cpp @@ -52,7 +52,7 @@ CustomTextLayout::CustomTextLayout(gsl::not_null const factory // If there is more than one text character here, push 0s for the rest of the columns // of the text run. - std::fill_n(std::back_inserter(_textClusterColumns), cols - 1, gsl::narrow_cast(0u)); + std::fill_n(std::back_inserter(_textClusterColumns), base::ClampSub(cols, 1u), gsl::narrow_cast(0u)); _text += text; } From f0c87bda0f0b2eadb66a4add16a1114a5ac7e7cc Mon Sep 17 00:00:00 2001 From: Michael Niksa Date: Mon, 2 Mar 2020 10:07:24 -0800 Subject: [PATCH 17/17] Dustin had an even better idea via Teams to use .resize since it has a fill operator. --- src/renderer/dx/CustomTextLayout.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/renderer/dx/CustomTextLayout.cpp b/src/renderer/dx/CustomTextLayout.cpp index 4ecb2430a60..a46db8030ef 100644 --- a/src/renderer/dx/CustomTextLayout.cpp +++ b/src/renderer/dx/CustomTextLayout.cpp @@ -52,7 +52,7 @@ CustomTextLayout::CustomTextLayout(gsl::not_null const factory // If there is more than one text character here, push 0s for the rest of the columns // of the text run. - std::fill_n(std::back_inserter(_textClusterColumns), base::ClampSub(cols, 1u), gsl::narrow_cast(0u)); + _textClusterColumns.resize(_textClusterColumns.size() + base::ClampSub(cols, 1u), gsl::narrow_cast(0u)); _text += text; }