Skip to content
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

Introduce the concept of "selection spans" instead of "rects" #17638

Merged
merged 4 commits into from
Aug 15, 2024

Conversation

DHowett
Copy link
Member

@DHowett DHowett commented Jul 31, 2024

Right now, selections are stored as a set of points (start, end, pivot)
and converted into rectangles based on the contents of the screen buffer
every single render pass.

They are also painted fairly late in the rendering cycle.

This pull request makes a few changes to how selections happen.

Selections are now requested earlier in the rendering pass, during the
creation of the RenderInfo, and stored as a span<point_span>. To
make that span work, the selection data must be durably stored
somewhere.

Therefore, Terminal and conhost now take advantage of their new
generational selection data (#17676) to create and cache arrays of
point_spans covering their selections.

Unlike selection rects, linear (non-box) selections take only a single
entry in the span set. Also unlike selection rects, but like search
highlights, selection spans use buffer coordinates.

In the future, AtlasEngine can use the information sent in during
PrepareRenderInfo to influence text rendering decisions such as the
foreground and background color for a grapheme cluster or the gridlines.

Unfortunately, GDI is stuck with the old selection rendering
implementation and therefore, we still need to produce selection rects
with viewport coordinates
and filter them down to the render engine's
dirty region. This is because GDI wants to invert all on-screen
pixels in the selection range after the final render to make sure
sixels, pixels, and lines are all displayed in the appropriate vintage
style.

As part of this change, I've migrated conhost's search highlight to not
read text directly out of the buffer but instead use the new
CopyRequest added by @tusharsnx in #16377.

Many of the tests for conhost behavior had a copy of conhost's code in
them, which sorta begs the question. I migrated them to use hardcoded
expected values to better catch regressions.

In Terminal, the "SelectAfterScroll" ControlCore tests weren't testing
anything they said they were, so I made them do so.

This comment has been minimized.

@DHowett DHowett force-pushed the dev/duhowett/selection-color-rewrite branch from 33835a8 to 49edd3c Compare August 1, 2024 02:38

This comment has been minimized.

@DHowett DHowett force-pushed the dev/duhowett/selection-color-rewrite branch from 49edd3c to d62b9c9 Compare August 3, 2024 21:15
src/renderer/base/renderer.cpp Fixed Show fixed Hide fixed
src/renderer/base/renderer.cpp Fixed Show fixed Hide fixed
src/renderer/base/renderer.cpp Fixed Show fixed Hide fixed

This comment has been minimized.

@DHowett DHowett force-pushed the dev/duhowett/selection-color-rewrite branch from d62b9c9 to 05fa7a6 Compare August 3, 2024 22:32

This comment has been minimized.

@DHowett DHowett force-pushed the dev/duhowett/selection-color-rewrite branch from 05fa7a6 to 05e185d Compare August 4, 2024 00:39

This comment has been minimized.

@DHowett DHowett force-pushed the dev/duhowett/selection-color-rewrite branch from 05e185d to 7757aae Compare August 4, 2024 04:26

This comment has been minimized.

@DHowett DHowett force-pushed the dev/duhowett/selection-color-rewrite branch from 7757aae to 1549268 Compare August 6, 2024 18:24

This comment has been minimized.

This comment has been minimized.

@DHowett DHowett force-pushed the dev/duhowett/selection-color-rewrite branch from a2d2a85 to 2a0d012 Compare August 7, 2024 01:00

This comment has been minimized.

@DHowett DHowett force-pushed the dev/duhowett/selection-color-rewrite branch from 2a0d012 to 6f8269c Compare August 7, 2024 16:35

This comment has been minimized.

DHowett added a commit that referenced this pull request Aug 9, 2024
In #17638, I am moving selection to an earlier phase of rendering (so
that further phases can take it into account). Since I am drafting off
the design of search highlights, one of the required changes is moving
to passing `span`s of `point_span`s around to make selection effectively
zero-copy.

We can't easily have zero-copy selection propagation without caching,
and we can't have caching without mandatory cache invalidation.

This pull request moves both conhost and Terminal to use
`til::generational` for all selection members that impact the ranges
that would be produced from `GetSelectionRects`.

This required a move from `std::optional<>` to a boolean to determine
whether a selection was active in Terminal.

We will no longer regenerate the selection rects from the selection
anchors plus the text buffer *every single frame*.

Apart from being annoying to read, there is one downside.

If you begin a selection on a narrow character, _and that narrow
character later turns into a wide character_, we will show it as
half-selected.

This should be a rare-enough case that we can accept it as a regression.
@DHowett DHowett force-pushed the dev/duhowett/selection-color-rewrite branch from 6f8269c to 899c001 Compare August 9, 2024 18:28
};
std::optional<SelectionAnchors> _selection;
bool _blockSelection = false;
til::generational<SelectionAnchors> _selection{ til::generation_t{ 1 } };
Copy link
Member Author

Choose a reason for hiding this comment

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

Leonard said generation 0 would be OK. The first change to selection would set it to 1, and the default state is "no selection".

src/cascadia/TerminalCore/Terminal.hpp Show resolved Hide resolved

This comment has been minimized.

@DHowett DHowett force-pushed the dev/duhowett/selection-color-rewrite branch from 899c001 to 2e7feb1 Compare August 12, 2024 22:21

This comment has been minimized.

@DHowett DHowett force-pushed the dev/duhowett/selection-color-rewrite branch 2 times, most recently from 0f14d0d to 1a086d0 Compare August 12, 2024 23:18
@DHowett DHowett force-pushed the dev/duhowett/selection-color-rewrite branch from 1a086d0 to a738100 Compare August 12, 2024 23:56
@DHowett DHowett changed the title WIP: rewrite selections to support selection colors and stuff - notes to follow Introduce the concept of "selection spans" to the renderer Aug 12, 2024
@DHowett DHowett changed the title Introduce the concept of "selection spans" to the renderer Introduce the concept of "selection spans" Aug 12, 2024
@DHowett DHowett changed the title Introduce the concept of "selection spans" Introduce the concept of "selection spans" instead of "rects" Aug 12, 2024
@DHowett DHowett marked this pull request as ready for review August 13, 2024 00:05
Copy link
Member

@carlos-zamora carlos-zamora left a comment

Choose a reason for hiding this comment

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

Nice work! Love how much simpler it is now 😊

src/renderer/base/renderer.cpp Outdated Show resolved Hide resolved
src/renderer/base/renderer.cpp Outdated Show resolved Hide resolved
src/host/selectionInput.cpp Outdated Show resolved Hide resolved
src/host/selectionInput.cpp Outdated Show resolved Hide resolved
Copy link
Member

@lhecker lhecker left a comment

Choose a reason for hiding this comment

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

The only thing I had in mind were things that wouldn't even qualify as nits. 😅 LGTM. ✅

@DHowett DHowett force-pushed the dev/duhowett/selection-color-rewrite branch 2 times, most recently from 403b93a to 5ae4c34 Compare August 15, 2024 16:15
@DHowett
Copy link
Member Author

DHowett commented Aug 15, 2024

@carlos-zamora I made your const changes and I'm letting the build spin. I'm surprised Audit mode didn't yell at me for it?

@DHowett
Copy link
Member Author

DHowett commented Aug 15, 2024

Diff from before the rebase:

diff --git a/src/host/selectionInput.cpp b/src/host/selectionInput.cpp
index 426214465..d6190a113 100644
--- a/src/host/selectionInput.cpp
+++ b/src/host/selectionInput.cpp
@@ -675,14 +675,14 @@ bool Selection::_HandleColorSelection(const INPUT_KEY_INFO* const pInputKeyInfo)
         if (fShiftPressed)
         {
             // Search the selection and color *that*
-            auto req = TextBuffer::CopyRequest::FromConfig(textBuffer,
-                                                           til::point{ _d->srSelectionRect.left, _d->srSelectionRect.top },
-                                                           til::point{ _d->srSelectionRect.right, _d->srSelectionRect.bottom },
-                                                           true /* multi-line search doesn't make sense; concatenate all lines */,
-                                                           false /* we filtered out block search above */,
-                                                           true /* trim block selection */,
-                                                           true);
-            auto str = textBuffer.GetPlainText(req);
+            const auto req = TextBuffer::CopyRequest::FromConfig(textBuffer,
+                                                                 til::point{ _d->srSelectionRect.left, _d->srSelectionRect.top },
+                                                                 til::point{ _d->srSelectionRect.right, _d->srSelectionRect.bottom },
+                                                                 true /* multi-line search doesn't make sense; concatenate all lines */,
+                                                                 false /* we filtered out block search above */,
+                                                                 true /* trim block selection */,
+                                                                 true);
+            const auto str = textBuffer.GetPlainText(req);
             // Clear the selection and call the search / mark function.
             ClearSelection();
 
diff --git a/src/renderer/base/renderer.cpp b/src/renderer/base/renderer.cpp
index 4b4dcf2ce..33892087e 100644
--- a/src/renderer/base/renderer.cpp
+++ b/src/renderer/base/renderer.cpp
@@ -328,7 +328,7 @@ void Renderer::TriggerSelection()
 try
 {
     const auto spans = _pData->GetSelectionSpans();
-    if (spans.size() != _lastSelectionPaintSize || (spans.size() && _lastSelectionPaintSpan != til::point_span{ spans.front().start, spans.back().end }))
+    if (spans.size() != _lastSelectionPaintSize || (!spans.empty() && _lastSelectionPaintSpan != til::point_span{ spans.front().start, spans.back().end }))
     {
         std::vector<til::rect> newSelectionViewportRects;
 
@@ -338,7 +338,7 @@ try
             _lastSelectionPaintSpan = til::point_span{ spans.front().start, spans.back().end };
 
             const auto& buffer = _pData->GetTextBuffer();
-            auto bufferWidth = buffer.GetSize().Width();
+            const auto bufferWidth = buffer.GetSize().Width();
             const til::rect vp{ _viewport.ToExclusive() };
             for (auto&& sp : spans)
             {
diff --git a/src/renderer/uia/UiaRenderer.cpp b/src/renderer/uia/UiaRenderer.cpp
index 8e5bfb824..155ea792a 100644
--- a/src/renderer/uia/UiaRenderer.cpp
+++ b/src/renderer/uia/UiaRenderer.cpp
@@ -20,7 +20,6 @@ UiaEngine::UiaEngine(IUiaEventDispatcher* dispatcher) :
     _textBufferChanged{ false },
     _cursorChanged{ false },
     _isEnabled{ true },
-    _prevSelection{},
     _prevCursorRegion{},
     RenderEngineBase()
 {
@@ -110,34 +109,13 @@ CATCH_RETURN();
 // - rectangles - One or more rectangles describing character positions on the grid
 // Return Value:
 // - S_OK
-[[nodiscard]] HRESULT UiaEngine::InvalidateSelection(std::span<const til::rect> rectangles) noexcept
-try
+[[nodiscard]] HRESULT UiaEngine::InvalidateSelection(std::span<const til::rect> /*rectangles*/) noexcept
 {
-    // early exit: different number of rows
-    if (_prevSelection.size() != rectangles.size())
-    {
-        _selectionChanged = true;
-        _prevSelection.assign(rectangles.begin(), rectangles.end());
-        return S_OK;
-    }
-
-    _selectionChanged = false; // assume they're the same
-
-    auto i = rectangles.begin();
-    auto j = _prevSelection.begin();
-    // safe to iterate j until i is exhausted because we checked their sizes
-    for (; i != rectangles.end(); ++i, ++j)
-    {
-        if (*i != *j)
-        {
-            _selectionChanged = true;
-            _prevSelection.assign(rectangles.begin(), rectangles.end());
-            break;
-        }
-    }
+    // INVARIANT: Renderer checks the incoming selection spans and only calls InvalidateSelection
+    // if they have actually changed.
+    _selectionChanged = true;
     return S_OK;
 }
-CATCH_LOG_RETURN_HR(E_FAIL);
 
 // Routine Description:
 // - Scrolls the existing dirty region (if it exists) and
diff --git a/src/renderer/uia/UiaRenderer.hpp b/src/renderer/uia/UiaRenderer.hpp
index 8bdeb15f4..c6038751f 100644
--- a/src/renderer/uia/UiaRenderer.hpp
+++ b/src/renderer/uia/UiaRenderer.hpp
@@ -74,7 +74,6 @@ namespace Microsoft::Console::Render
 
         Microsoft::Console::Types::IUiaEventDispatcher* _dispatcher;
 
-        std::vector<til::rect> _prevSelection;
         til::rect _prevCursorRegion;
     };
 }

@DHowett
Copy link
Member Author

DHowett commented Aug 15, 2024

I bet the changes to SetViewportPosition have broken the scroll test. Let me re-figure that one.

conhost: rewrite colorsearch to use GetPlainText and GetSelectionSpans

I also removed a spurious GetSelectionRects in the clipboard code

conhost: Get the tests working with GetSelectionSpans

conhost: remove GetSelectionRects completely

conhost (consider merging): use GetTextSpans
TerminalCore: Get the tests working with Selection spans
The renderer will consume *buffer-relative selection spans* and prepare
them into *viewport-relative rects*.

PaintSelection will still filter rects by whether they are in the dirty
area.
@DHowett DHowett force-pushed the dev/duhowett/selection-color-rewrite branch from 5ae4c34 to 05c9435 Compare August 15, 2024 17:12
@DHowett
Copy link
Member Author

DHowett commented Aug 15, 2024

Latest push:

commit 492fb1b3744d3615cee7b04403617c49695d562a
Author: Dustin L. Howett <[email protected]>
Date:   Thu Aug 15 12:12:15 2024 -0500

    fixup! terminal: port TerminalSelection to GetSelectionSpans

diff --git a/src/cascadia/UnitTests_TerminalCore/SelectionTest.cpp b/src/cascadia/UnitTests_TerminalCore/SelectionTest.cpp
index 5b21b3e41..11f01c752 100644
--- a/src/cascadia/UnitTests_TerminalCore/SelectionTest.cpp
+++ b/src/cascadia/UnitTests_TerminalCore/SelectionTest.cpp
@@ -259,8 +259,8 @@ namespace TerminalCoreUnitTests
         {
             Terminal term{ Terminal::TestDummyMarker{} };
             DummyRenderer renderer{ &term };
-            til::CoordType scrollbackLines = 5;
-            term.Create({ 100, 100 }, scrollbackLines, renderer);
+            til::CoordType scrollbackLines = 100;
+            term.Create({ 120, 30 }, scrollbackLines, renderer);
 
             const til::CoordType contentScrollLines = 15;
             // Simulate a content-initiated scroll down by 15 lines

@DHowett DHowett merged commit 1ef4979 into main Aug 15, 2024
20 checks passed
@DHowett DHowett deleted the dev/duhowett/selection-color-rewrite branch August 15, 2024 19:00
DHowett added a commit that referenced this pull request Aug 19, 2024
…a overlay (#17725)

With the merge of #17638, selections are now accumulated early in the
rendering process. This allows Atlas, which currently makes decisions
about cell foreground/background at the time of text rendering,
awareness of the selection ranges *before* text rendering begins.

As a result, we can now paint the selection into the background and
foreground bitmaps. We no longer need to overlay a rectangle, or series
of rectangles, on top of the rendering surface and alpha blend the
selection color onto the final image.

As a reminder, "alpha selection" was always a stopgap because we didn't
have durable per-cell foreground and background customization in the
original DxEngine.

Selection foregrounds are not customizable, and will be chosen using the
same color distancing algorithm as the cursor. We can make them
customizable "easily" (once we figure out the schema for it) for #3580.

`ATLAS_DEBUG_SHOW_DIRTY` was using the `Selection` shading type to draw
colored regions. I didn't want to break that, so I elected to rename the
`Selection` shading type to `FilledRect` and keep its value. It helps
that the shader didn't have any special treatment for
`SHADING_TYPE_SELECTION`.

This fixes the entire category of issues created by selection being an
80%-opacity white rectangle. However, given that it changes the imputed
colors of the text it will reveal `SGR 8` concealed/hidden characters.

Refs #17355
Refs #14859
Refs #11181
Refs #8716
Refs #4971
Closes #3561
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants