From 60b44c856ef0d1a95d6611c5be53d0f7efd6166b Mon Sep 17 00:00:00 2001 From: Moshe Schorr Date: Fri, 7 Aug 2020 22:04:53 +0300 Subject: [PATCH] Batch RTL runs to ensure proper draw order (#7190) Consecutive RTL GlyphRuns are drawn from the last to the first. References #538, #7149, all those issues asking for RTL closed as dupes. As @miniksa suggested in a comment on #7149 -- handle the thingy on the render side. If we have GlyphRuns abcdEFGh, where EFG are RTL, we draw them now in order abcdGFEh. This has ransom-noting, because I didn't touch the font scaling at all. This should fix the majority of RTL issues, except it *doesn't* fix issues with colors, because those get split in the TextBuffer phase in the renderer I think, so they show up separately by the GlyphRun phase. --- ...1ff90dc512c83c902762b02f284c1c61603b4a.txt | 10 ++ .github/actions/spell-check/expect/expect.txt | 33 ----- src/renderer/dx/CustomTextLayout.cpp | 140 ++++++++++++------ src/renderer/dx/CustomTextLayout.h | 4 + 4 files changed, 112 insertions(+), 75 deletions(-) create mode 100644 .github/actions/spell-check/expect/af1ff90dc512c83c902762b02f284c1c61603b4a.txt diff --git a/.github/actions/spell-check/expect/af1ff90dc512c83c902762b02f284c1c61603b4a.txt b/.github/actions/spell-check/expect/af1ff90dc512c83c902762b02f284c1c61603b4a.txt new file mode 100644 index 00000000000..66fb99de72d --- /dev/null +++ b/.github/actions/spell-check/expect/af1ff90dc512c83c902762b02f284c1c61603b4a.txt @@ -0,0 +1,10 @@ +abcd +dst +EFG +EFGh +EMPTYBOX +GFEh +nrcs +Remoting +Scs +Shobjidl diff --git a/.github/actions/spell-check/expect/expect.txt b/.github/actions/spell-check/expect/expect.txt index 01b64c42d78..5a74915444a 100644 --- a/.github/actions/spell-check/expect/expect.txt +++ b/.github/actions/spell-check/expect/expect.txt @@ -99,7 +99,6 @@ AStomps ASYNCWINDOWPOS atch ATest -atg attr ATTRCOLOR aumid @@ -160,7 +159,6 @@ bitsavers bitset BKCOLOR BKGND -BKMK Bksp blog Blt @@ -295,7 +293,6 @@ codepage codepoint codeproject COINIT -colo colorizing colororacle colorref @@ -352,7 +349,6 @@ conpty conptylib consecteturadipiscingelit conserv -consoleaccessibility consoleapi CONSOLECONTROL CONSOLEENDTASK @@ -640,7 +636,6 @@ DROPDOWNLIST DROPFILES drv dsm -Dst DSwap DTest dtor @@ -683,7 +678,6 @@ Elems elif elseif emacs -emptybox enabledelayedexpansion endian endif @@ -752,7 +746,6 @@ fdw fea fesb FFDE -FFF FFrom FGCOLOR fgetc @@ -769,7 +762,6 @@ FILETYPE FILEW FILLATTR FILLCONSOLEOUTPUT -Filledbox FILTERONPASTE finalizer FINDCASE @@ -821,7 +813,6 @@ fsproj fstream fte Ftm -fullcolor fullscreen fullwidth func @@ -1011,7 +1002,6 @@ hwheel hwnd HWNDPARENT hxx -hyperlink IAccessibility IAction IApi @@ -1034,7 +1024,6 @@ ICore IData IDCANCEL IDD -IDefault IDesktop IDictionary IDISHWND @@ -1384,7 +1373,6 @@ mincore mindbogglingly mingw minkernel -minmax minwin minwindef Mip @@ -1545,7 +1533,6 @@ NOYIELD NOZORDER NPM npos -NRCS NSTATUS ntapi ntcon @@ -1722,7 +1709,6 @@ pguid pgup PHANDLE phhook -phsl phwnd pid pidl @@ -1752,7 +1738,6 @@ POBJECT Podcast POINTSLIST POLYTEXTW -popclip popd POPF poppack @@ -1935,7 +1920,6 @@ REGSTR reingest Relayout RELBINPATH -remoting renderengine rendersize reparent @@ -1943,7 +1927,6 @@ reparenting replatformed Replymessage repositorypath -rerendered rescap Resequence Reserialize @@ -1989,7 +1972,6 @@ RMENU roadmap robomac roundtrip -ROWSTOSCROLL rparen RRF RRRGGGBB @@ -2051,7 +2033,6 @@ SCROLLSCALE SCROLLSCREENBUFFER Scrollup Scrolluppage -SCS scursor sddl sdeleted @@ -2121,7 +2102,6 @@ Shl shlguid shlobj shlwapi -shobjidl SHORTPATH SHOWCURSOR SHOWMAXIMIZED @@ -2194,14 +2174,11 @@ STARTWPARMSW Statusline stdafx STDAPI -stdarg stdcall -stddef stderr stdexcept stdin stdio -stdlib STDMETHODCALLTYPE STDMETHODIMP stdout @@ -2295,7 +2272,6 @@ telnetd telnetpp templated terminalcore -terminalnuget TERMINALSCROLLING terminfo TEs @@ -2351,7 +2327,6 @@ TMult tmultiple tmux todo -Tofill tofrom tokenhelpers tokenized @@ -2447,7 +2422,6 @@ undef Unescape unexpand Unfocus -unfocuses unhighlighting unhosted unicode @@ -2753,33 +2727,26 @@ WWith wx wxh xa -xab xact xamarin xaml Xamlmeta xargs xaz -xb -xbc xbf xbutton XBUTTONDBLCLK XBUTTONDOWN XBUTTONUP -xca XCast -xce XCENTER XColors xcopy XCount -xdd xdy xe XEncoding xff -xffff XFile xlang XManifest diff --git a/src/renderer/dx/CustomTextLayout.cpp b/src/renderer/dx/CustomTextLayout.cpp index c9f30c039a7..43b491cb54e 100644 --- a/src/renderer/dx/CustomTextLayout.cpp +++ b/src/renderer/dx/CustomTextLayout.cpp @@ -847,57 +847,113 @@ CATCH_RETURN(); auto mutableOrigin = origin; // Draw each run separately. - for (UINT32 runIndex = 0; runIndex < _runs.size(); ++runIndex) + for (INT32 runIndex = 0; runIndex < gsl::narrow(_runs.size()); ++runIndex) { // Get the run const Run& run = _runs.at(runIndex); - // Prepare the glyph run and description objects by converting our - // internal storage representation into something that matches DWrite's structures. - DWRITE_GLYPH_RUN glyphRun; - glyphRun.bidiLevel = run.bidiLevel; - glyphRun.fontEmSize = _format->GetFontSize() * run.fontScale; - glyphRun.fontFace = run.fontFace.Get(); - glyphRun.glyphAdvances = &_glyphAdvances.at(run.glyphStart); - glyphRun.glyphCount = run.glyphCount; - glyphRun.glyphIndices = &_glyphIndices.at(run.glyphStart); - glyphRun.glyphOffsets = &_glyphOffsets.at(run.glyphStart); - glyphRun.isSideways = false; - - DWRITE_GLYPH_RUN_DESCRIPTION glyphRunDescription; - glyphRunDescription.clusterMap = _glyphClusters.data(); - glyphRunDescription.localeName = _localeName.data(); - glyphRunDescription.string = _text.data(); - glyphRunDescription.stringLength = run.textLength; - glyphRunDescription.textPosition = run.textStart; - - // Calculate the origin for the next run based on the amount of space - // that would be consumed. We are doing this calculation now, not after, - // because if the text is RTL then we need to advance immediately, before the - // write call since DirectX expects the origin to the RIGHT of the text for RTL. - const auto postOriginX = std::accumulate(_glyphAdvances.begin() + run.glyphStart, - _glyphAdvances.begin() + run.glyphStart + run.glyphCount, - mutableOrigin.x); - - // Check for RTL, if it is, apply space adjustment. - if (WI_IsFlagSet(glyphRun.bidiLevel, 1)) + if (!WI_IsFlagSet(run.bidiLevel, 1)) { - mutableOrigin.x = postOriginX; + RETURN_IF_FAILED(_DrawGlyphRun(clientDrawingContext, renderer, mutableOrigin, run)); } + // This is the RTL behavior. We will advance to the last contiguous RTL run, draw that, + // and then keep on going backwards from there, and then move runIndex beyond. + // Let's say we have runs abcdEFGh, where runs EFG are RTL. + // Then we will draw them in the order abcdGFEh + else + { + const INT32 originalRunIndex = runIndex; + INT32 lastIndexRTL = runIndex; + + // Step 1: Get to the last contiguous RTL run from here + while (lastIndexRTL < gsl::narrow(_runs.size()) - 1) // only could ever advance if there's something left + { + const Run& nextRun = _runs.at(gsl::narrow_cast(lastIndexRTL + 1)); + if (WI_IsFlagSet(nextRun.bidiLevel, 1)) + { + lastIndexRTL++; + } + else + { + break; + } + } + + // Go from the last to the first and draw + for (runIndex = lastIndexRTL; runIndex >= originalRunIndex; runIndex--) + { + const Run& currentRun = _runs.at(runIndex); + RETURN_IF_FAILED(_DrawGlyphRun(clientDrawingContext, renderer, mutableOrigin, currentRun)); + } + runIndex = lastIndexRTL; // and the for loop will take the increment to the last one + } + } + } + CATCH_RETURN(); + return S_OK; +} - // Try to draw it - RETURN_IF_FAILED(renderer->DrawGlyphRun(clientDrawingContext, - mutableOrigin.x, - mutableOrigin.y, - DWRITE_MEASURING_MODE_NATURAL, - &glyphRun, - &glyphRunDescription, - run.drawingEffect.Get())); - - // Either way, we should be at this point by the end of writing this sequence, - // whether it was LTR or RTL. +// Routine Description: +// - Draw the given run +// - The origin is updated to be after the run. +// Arguments: +// - clientDrawingContext - Optional pointer to information that the renderer might need +// while attempting to graphically place the text onto the screen +// - renderer - The interface to be used for actually putting text onto the screen +// - origin - pixel point of top left corner on final surface for drawing +// - run - the run to be drawn +[[nodiscard]] HRESULT CustomTextLayout::_DrawGlyphRun(_In_opt_ void* clientDrawingContext, + gsl::not_null renderer, + D2D_POINT_2F& mutableOrigin, + const Run& run) noexcept +{ + try + { + // Prepare the glyph run and description objects by converting our + // internal storage representation into something that matches DWrite's structures. + DWRITE_GLYPH_RUN glyphRun; + glyphRun.bidiLevel = run.bidiLevel; + glyphRun.fontEmSize = _format->GetFontSize() * run.fontScale; + glyphRun.fontFace = run.fontFace.Get(); + glyphRun.glyphAdvances = &_glyphAdvances.at(run.glyphStart); + glyphRun.glyphCount = run.glyphCount; + glyphRun.glyphIndices = &_glyphIndices.at(run.glyphStart); + glyphRun.glyphOffsets = &_glyphOffsets.at(run.glyphStart); + glyphRun.isSideways = false; + + DWRITE_GLYPH_RUN_DESCRIPTION glyphRunDescription; + glyphRunDescription.clusterMap = _glyphClusters.data(); + glyphRunDescription.localeName = _localeName.data(); + glyphRunDescription.string = _text.data(); + glyphRunDescription.stringLength = run.textLength; + glyphRunDescription.textPosition = run.textStart; + + // Calculate the origin for the next run based on the amount of space + // that would be consumed. We are doing this calculation now, not after, + // because if the text is RTL then we need to advance immediately, before the + // write call since DirectX expects the origin to the RIGHT of the text for RTL. + const auto postOriginX = std::accumulate(_glyphAdvances.begin() + run.glyphStart, + _glyphAdvances.begin() + run.glyphStart + run.glyphCount, + mutableOrigin.x); + + // Check for RTL, if it is, apply space adjustment. + if (WI_IsFlagSet(glyphRun.bidiLevel, 1)) + { mutableOrigin.x = postOriginX; } + + // Try to draw it + RETURN_IF_FAILED(renderer->DrawGlyphRun(clientDrawingContext, + mutableOrigin.x, + mutableOrigin.y, + DWRITE_MEASURING_MODE_NATURAL, + &glyphRun, + &glyphRunDescription, + run.drawingEffect.Get())); + + // Either way, we should be at this point by the end of writing this sequence, + // whether it was LTR or RTL. + mutableOrigin.x = postOriginX; } CATCH_RETURN(); return S_OK; diff --git a/src/renderer/dx/CustomTextLayout.h b/src/renderer/dx/CustomTextLayout.h index cf2a63b574b..fe197425794 100644 --- a/src/renderer/dx/CustomTextLayout.h +++ b/src/renderer/dx/CustomTextLayout.h @@ -147,6 +147,10 @@ namespace Microsoft::Console::Render [[nodiscard]] HRESULT _DrawGlyphRuns(_In_opt_ void* clientDrawingContext, IDWriteTextRenderer* renderer, const D2D_POINT_2F origin) noexcept; + [[nodiscard]] HRESULT _DrawGlyphRun(_In_opt_ void* clientDrawingContext, + gsl::not_null renderer, + D2D_POINT_2F& mutableOrigin, + const Run& run) noexcept; [[nodiscard]] static constexpr UINT32 _EstimateGlyphCount(const UINT32 textLength) noexcept;