Skip to content

Commit

Permalink
Fix horizontal scrolling bugs in AtlasEngine/DxEngine (#15707)
Browse files Browse the repository at this point in the history 
This commit fixes a number of issues around horizontal scrolling.
DxEngine only had one bug, where the clip rect would cause any content
outside of the actual viewport to be invisible. AtlasEngine had more
bugs, mostly around the conversion from textbuffer-relative coordinates
to viewport-relative coordinates, since AtlasEngine stores things like
the cursor position, attributes, etc., relative to the viewport.

It also renames `cellCount` to `viewportCellCount`, because I realized
that it might have to deal with a `textBufferCellCount` or similar in
the future. I hope that the new name is more descriptive of what it
refers to.

Future improvements to AtlasEngine in particular would be to not copy
the entire `Settings` struct every time the horizontal scroll offset
changes, and to trim trailing whitespace before shaping text.

This is in preparation for #1860

## Validation Steps Performed
* Patch `RenderingTests` to run in the main (and not alt) buffer
* Horizontal scrolling of line renditions and attributes works ✅
* Selection retains its position (mostly) ✅
  • Loading branch information
@lhecker
lhecker authored Jul 18, 2023
1 parent 6873c85 commit 89c39b0
Show file tree
Hide file tree
Showing 11 changed files with 110 additions and 83 deletions.
24 changes: 18 additions & 6 deletions src/renderer/atlas/AtlasEngine.api.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ constexpr HRESULT vec2_narrow(U x, U y, vec2<T>& out) noexcept
if (delta < 0)
{
_api.invalidatedRows.start = gsl::narrow_cast<u16>(clamp<int>(_api.invalidatedRows.start + delta, u16min, u16max));
_api.invalidatedRows.end = _api.s->cellCount.y;
_api.invalidatedRows.end = _api.s->viewportCellCount.y;
}
else
{
Expand Down Expand Up @@ -182,17 +182,29 @@ constexpr HRESULT vec2_narrow(U x, U y, vec2<T>& out) noexcept
}

[[nodiscard]] HRESULT AtlasEngine::UpdateViewport(const til::inclusive_rect& srNewViewport) noexcept
try
{
const u16x2 cellCount{
gsl::narrow_cast<u16>(std::max(1, srNewViewport.right - srNewViewport.left + 1)),
gsl::narrow_cast<u16>(std::max(1, srNewViewport.bottom - srNewViewport.top + 1)),
const u16x2 viewportCellCount{
gsl::narrow<u16>(std::max(1, srNewViewport.right - srNewViewport.left + 1)),
gsl::narrow<u16>(std::max(1, srNewViewport.bottom - srNewViewport.top + 1)),
};
const u16x2 viewportOffset{
gsl::narrow<u16>(srNewViewport.left),
gsl::narrow<u16>(srNewViewport.top),
};
if (_api.s->cellCount != cellCount)

if (_api.s->viewportCellCount != viewportCellCount)
{
_api.s.write()->cellCount = cellCount;
_api.s.write()->viewportCellCount = viewportCellCount;
}
if (_api.s->viewportOffset != viewportOffset)
{
_api.s.write()->viewportOffset = viewportOffset;
}

return S_OK;
}
CATCH_RETURN()

[[nodiscard]] HRESULT AtlasEngine::GetProposedFont(const FontInfoDesired& fontInfoDesired, _Out_ FontInfo& fontInfo, const int dpi) noexcept
try
Expand Down
97 changes: 49 additions & 48 deletions src/renderer/atlas/AtlasEngine.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,17 +78,17 @@ try

// Clamp invalidation rects into valid value ranges.
{
_api.invalidatedCursorArea.left = std::min(_api.invalidatedCursorArea.left, _p.s->cellCount.x);
_api.invalidatedCursorArea.top = std::min(_api.invalidatedCursorArea.top, _p.s->cellCount.y);
_api.invalidatedCursorArea.right = clamp(_api.invalidatedCursorArea.right, _api.invalidatedCursorArea.left, _p.s->cellCount.x);
_api.invalidatedCursorArea.bottom = clamp(_api.invalidatedCursorArea.bottom, _api.invalidatedCursorArea.top, _p.s->cellCount.y);
_api.invalidatedCursorArea.left = std::min(_api.invalidatedCursorArea.left, _p.s->viewportCellCount.x);
_api.invalidatedCursorArea.top = std::min(_api.invalidatedCursorArea.top, _p.s->viewportCellCount.y);
_api.invalidatedCursorArea.right = clamp(_api.invalidatedCursorArea.right, _api.invalidatedCursorArea.left, _p.s->viewportCellCount.x);
_api.invalidatedCursorArea.bottom = clamp(_api.invalidatedCursorArea.bottom, _api.invalidatedCursorArea.top, _p.s->viewportCellCount.y);
}
{
_api.invalidatedRows.start = std::min(_api.invalidatedRows.start, _p.s->cellCount.y);
_api.invalidatedRows.end = clamp(_api.invalidatedRows.end, _api.invalidatedRows.start, _p.s->cellCount.y);
_api.invalidatedRows.start = std::min(_api.invalidatedRows.start, _p.s->viewportCellCount.y);
_api.invalidatedRows.end = clamp(_api.invalidatedRows.end, _api.invalidatedRows.start, _p.s->viewportCellCount.y);
}
{
const auto limit = gsl::narrow_cast<i16>(_p.s->cellCount.y & 0x7fff);
const auto limit = gsl::narrow_cast<i16>(_p.s->viewportCellCount.y & 0x7fff);
const auto offset = gsl::narrow_cast<i16>(clamp<int>(_api.scrollOffset, -limit, limit));
const auto nothingInvalid = _api.invalidatedRows.start == _api.invalidatedRows.end;

Expand All @@ -97,9 +97,9 @@ try
// Mark the newly scrolled in rows as invalidated
if (offset < 0)
{
const u16 begRow = _p.s->cellCount.y + offset;
const u16 begRow = _p.s->viewportCellCount.y + offset;
_api.invalidatedRows.start = nothingInvalid ? begRow : std::min(_api.invalidatedRows.start, begRow);
_api.invalidatedRows.end = _p.s->cellCount.y;
_api.invalidatedRows.end = _p.s->viewportCellCount.y;
}
else
{
Expand All @@ -112,7 +112,7 @@ try
_api.dirtyRect = {
0,
_api.invalidatedRows.start,
_p.s->cellCount.x,
_p.s->viewportCellCount.x,
_api.invalidatedRows.end,
};

Expand All @@ -134,7 +134,7 @@ try
// the contents of the entire swap chain is redundant, but more importantly because the scroll rect
// is the subset of the contents that are being scrolled into. If you scroll the entire viewport
// then the scroll rect is empty, which Present1() will loudly complain about.
if (_p.invalidatedRows == range<u16>{ 0, _p.s->cellCount.y })
if (_p.invalidatedRows == range<u16>{ 0, _p.s->viewportCellCount.y })
{
_p.MarkAllAsDirty();
}
Expand Down Expand Up @@ -282,7 +282,7 @@ CATCH_RETURN()

[[nodiscard]] HRESULT AtlasEngine::PrepareLineTransform(const LineRendition lineRendition, const til::CoordType targetRow, const til::CoordType viewportLeft) noexcept
{
const auto y = gsl::narrow_cast<u16>(clamp<til::CoordType>(targetRow, 0, _p.s->cellCount.y));
const auto y = gsl::narrow_cast<u16>(clamp<til::CoordType>(targetRow, 0, _p.s->viewportCellCount.y));
_p.rows[y]->lineRendition = lineRendition;
_api.lineRendition = lineRendition;
return S_OK;
Expand All @@ -296,14 +296,15 @@ CATCH_RETURN()
[[nodiscard]] HRESULT AtlasEngine::PaintBufferLine(std::span<const Cluster> clusters, til::point coord, const bool fTrimLeft, const bool lineWrapped) noexcept
try
{
const auto y = gsl::narrow_cast<u16>(clamp<int>(coord.y, 0, _p.s->cellCount.y));
const auto y = gsl::narrow_cast<u16>(clamp<int>(coord.y, 0, _p.s->viewportCellCount.y));

if (_api.lastPaintBufferLineCoord.y != y)
{
_flushBufferLine();
}

const auto x = gsl::narrow_cast<u16>(clamp<int>(coord.x, 0, _p.s->cellCount.x));
const auto shift = gsl::narrow_cast<u8>(_api.lineRendition != LineRendition::SingleWidth);
const auto x = gsl::narrow_cast<u16>(clamp<int>(coord.x - (_p.s->viewportOffset.x >> shift), 0, _p.s->viewportCellCount.x));
auto columnEnd = x;

// _api.bufferLineColumn contains 1 more item than _api.bufferLine, as it represents the
Expand All @@ -330,7 +331,6 @@ try
}

{
const auto shift = gsl::narrow_cast<u8>(_api.lineRendition != LineRendition::SingleWidth);
const auto row = _p.colorBitmap.begin() + _p.colorBitmapRowStride * y;
auto beg = row + (static_cast<size_t>(x) << shift);
auto end = row + (static_cast<size_t>(columnEnd) << shift);
Expand Down Expand Up @@ -368,9 +368,10 @@ CATCH_RETURN()
try
{
const auto shift = gsl::narrow_cast<u8>(_api.lineRendition != LineRendition::SingleWidth);
const auto y = gsl::narrow_cast<u16>(clamp<til::CoordType>(coordTarget.y, 0, _p.s->cellCount.y));
const auto from = gsl::narrow_cast<u16>(clamp<til::CoordType>(coordTarget.x << shift, 0, _p.s->cellCount.x - 1));
const auto to = gsl::narrow_cast<u16>(clamp<size_t>((coordTarget.x + cchLine) << shift, from, _p.s->cellCount.x));
const auto x = std::max(0, coordTarget.x - (_p.s->viewportOffset.x >> shift));
const auto y = gsl::narrow_cast<u16>(clamp<til::CoordType>(coordTarget.y, 0, _p.s->viewportCellCount.y));
const auto from = gsl::narrow_cast<u16>(clamp<til::CoordType>(x << shift, 0, _p.s->viewportCellCount.x - 1));
const auto to = gsl::narrow_cast<u16>(clamp<size_t>((x + cchLine) << shift, from, _p.s->viewportCellCount.x));
const auto fg = gsl::narrow_cast<u32>(color) | 0xff000000;
_p.rows[y]->gridLineRanges.emplace_back(lines, fg, from, to);
return S_OK;
Expand All @@ -385,9 +386,9 @@ try
// As such we got to call _flushBufferLine() here just to be sure.
_flushBufferLine();

const auto y = gsl::narrow_cast<u16>(clamp<til::CoordType>(rect.top, 0, _p.s->cellCount.y));
const auto from = gsl::narrow_cast<u16>(clamp<til::CoordType>(rect.left, 0, _p.s->cellCount.x - 1));
const auto to = gsl::narrow_cast<u16>(clamp<til::CoordType>(rect.right, from, _p.s->cellCount.x));
const auto y = gsl::narrow_cast<u16>(clamp<til::CoordType>(rect.top, 0, _p.s->viewportCellCount.y));
const auto from = gsl::narrow_cast<u16>(clamp<til::CoordType>(rect.left, 0, _p.s->viewportCellCount.x - 1));
const auto to = gsl::narrow_cast<u16>(clamp<til::CoordType>(rect.right, from, _p.s->viewportCellCount.x));

auto& row = *_p.rows[y];
row.selectionFrom = from;
Expand Down Expand Up @@ -433,30 +434,30 @@ try

if (options.isOn)
{
const auto point = options.coordCursor;
// TODO: options.coordCursor can contain invalid out of bounds coordinates when
// the window is being resized and the cursor is on the last line of the viewport.
const auto top = clamp(point.y, 0, _p.s->cellCount.y - 1);
const auto bottom = top + 1;
const auto cursorWidth = 1 + (options.fIsDoubleWidth & (options.cursorType != CursorType::VerticalBar));
const auto top = options.coordCursor.y;
const auto bottom = top + 1;
const auto shift = gsl::narrow_cast<u8>(_p.rows[top]->lineRendition != LineRendition::SingleWidth);
auto left = options.coordCursor.x - (_p.s->viewportOffset.x >> shift);
auto right = left + cursorWidth;

left <<= shift;
right <<= shift;

_p.cursorRect = {
std::max<til::CoordType>(left, 0),
std::max<til::CoordType>(top, 0),
std::min<til::CoordType>(right, _p.s->viewportCellCount.x),
std::min<til::CoordType>(bottom, _p.s->viewportCellCount.y),
};

auto left = std::max(point.x, 0);
auto right = std::max(left + cursorWidth, 0);

if (_p.rows[top]->lineRendition != LineRendition::SingleWidth)
if (_p.cursorRect)
{
left <<= 1;
right <<= 1;
_p.dirtyRectInPx.left = std::min(_p.dirtyRectInPx.left, left * _p.s->font->cellSize.x);
_p.dirtyRectInPx.top = std::min(_p.dirtyRectInPx.top, top * _p.s->font->cellSize.y);
_p.dirtyRectInPx.right = std::max(_p.dirtyRectInPx.right, right * _p.s->font->cellSize.x);
_p.dirtyRectInPx.bottom = std::max(_p.dirtyRectInPx.bottom, bottom * _p.s->font->cellSize.y);
}

left = std::min(left, _p.s->cellCount.x - cursorWidth);
right = std::min(right, i32{ _p.s->cellCount.x });

_p.cursorRect = { left, top, right, bottom };
_p.dirtyRectInPx.left = std::min(_p.dirtyRectInPx.left, left * _p.s->font->cellSize.x);
_p.dirtyRectInPx.top = std::min(_p.dirtyRectInPx.top, top * _p.s->font->cellSize.y);
_p.dirtyRectInPx.right = std::max(_p.dirtyRectInPx.right, right * _p.s->font->cellSize.x);
_p.dirtyRectInPx.bottom = std::max(_p.dirtyRectInPx.bottom, bottom * _p.s->font->cellSize.y);
}

return S_OK;
Expand Down Expand Up @@ -501,7 +502,7 @@ void AtlasEngine::_handleSettingsUpdate()
{
const auto targetChanged = _p.s->target != _api.s->target;
const auto fontChanged = _p.s->font != _api.s->font;
const auto cellCountChanged = _p.s->cellCount != _api.s->cellCount;
const auto cellCountChanged = _p.s->viewportCellCount != _api.s->viewportCellCount;

_p.s = _api.s;

Expand Down Expand Up @@ -573,7 +574,7 @@ void AtlasEngine::_recreateFontDependentResources()
void AtlasEngine::_recreateCellCountDependentResources()
{
// Let's guess that every cell consists of a surrogate pair.
const auto projectedTextSize = static_cast<size_t>(_p.s->cellCount.x) * 2;
const auto projectedTextSize = static_cast<size_t>(_p.s->viewportCellCount.x) * 2;
// IDWriteTextAnalyzer::GetGlyphs says:
// The recommended estimate for the per-glyph output buffers is (3 * textLength / 2 + 16).
const auto projectedGlyphSize = 3 * projectedTextSize / 2 + 16;
Expand All @@ -590,16 +591,16 @@ void AtlasEngine::_recreateCellCountDependentResources()
_api.glyphAdvances = Buffer<f32>{ projectedGlyphSize };
_api.glyphOffsets = Buffer<DWRITE_GLYPH_OFFSET>{ projectedGlyphSize };

_p.unorderedRows = Buffer<ShapedRow>(_p.s->cellCount.y);
_p.rowsScratch = Buffer<ShapedRow*>(_p.s->cellCount.y);
_p.rows = Buffer<ShapedRow*>(_p.s->cellCount.y);
_p.unorderedRows = Buffer<ShapedRow>(_p.s->viewportCellCount.y);
_p.rowsScratch = Buffer<ShapedRow*>(_p.s->viewportCellCount.y);
_p.rows = Buffer<ShapedRow*>(_p.s->viewportCellCount.y);

// Our render loop heavily relies on memcpy() which is up to between 1.5x (Intel)
// and 40x (AMD) faster for allocations with an alignment of 32 or greater.
// backgroundBitmapStride is a "count" of u32 and not in bytes,
// so we round up to multiple of 8 because 8 * sizeof(u32) == 32.
_p.colorBitmapRowStride = (static_cast<size_t>(_p.s->cellCount.x) + 7) & ~7;
_p.colorBitmapDepthStride = _p.colorBitmapRowStride * _p.s->cellCount.y;
_p.colorBitmapRowStride = (static_cast<size_t>(_p.s->viewportCellCount.x) + 7) & ~7;
_p.colorBitmapDepthStride = _p.colorBitmapRowStride * _p.s->viewportCellCount.y;
_p.colorBitmap = Buffer<u32, 32>(_p.colorBitmapDepthStride * 2);
_p.backgroundBitmap = { _p.colorBitmap.data(), _p.colorBitmapDepthStride };
_p.foregroundBitmap = { _p.colorBitmap.data() + _p.colorBitmapDepthStride, _p.colorBitmapDepthStride };
Expand Down
5 changes: 4 additions & 1 deletion src/renderer/atlas/AtlasEngine.r.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -463,7 +463,10 @@ void AtlasEngine::_present()
{
const auto offsetInPx = _p.scrollOffset * _p.s->font->cellSize.y;
const auto width = _p.s->targetSize.x;
const auto height = _p.s->cellCount.y * _p.s->font->cellSize.y;
// We don't use targetSize.y here, because "height" refers to the bottom coordinate of the last text row
// in the buffer. We then add the "offsetInPx" (which is negative when scrolling text upwards) and thus
// end up with a "bottom" value that is the bottom of the last row of text that we haven't invalidated.
const auto height = _p.s->viewportCellCount.y * _p.s->font->cellSize.y;
const auto top = std::max(0, offsetInPx);
const auto bottom = height + std::min(0, offsetInPx);

Expand Down
8 changes: 4 additions & 4 deletions src/renderer/atlas/BackendD2D.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ void BackendD2D::_handleSettingsUpdate(const RenderingPayload& p)
const auto renderTargetChanged = !_renderTarget;
const auto fontChanged = _fontGeneration != p.s->font.generation();
const auto cursorChanged = _cursorGeneration != p.s->cursor.generation();
const auto cellCountChanged = _cellCount != p.s->cellCount;
const auto cellCountChanged = _viewportCellCount != p.s->viewportCellCount;

if (renderTargetChanged)
{
Expand Down Expand Up @@ -120,8 +120,8 @@ void BackendD2D::_handleSettingsUpdate(const RenderingPayload& p)
.dpiY = static_cast<f32>(p.s->font->dpi),
};
const D2D1_SIZE_U size{
p.s->cellCount.x,
p.s->cellCount.y,
p.s->viewportCellCount.x,
p.s->viewportCellCount.y,
};
const D2D1_MATRIX_3X2_F transform{
.m11 = static_cast<f32>(p.s->font->cellSize.x),
Expand All @@ -145,7 +145,7 @@ void BackendD2D::_handleSettingsUpdate(const RenderingPayload& p)
_generation = p.s.generation();
_fontGeneration = p.s->font.generation();
_cursorGeneration = p.s->cursor.generation();
_cellCount = p.s->cellCount;
_viewportCellCount = p.s->viewportCellCount;
}

void BackendD2D::_drawBackground(const RenderingPayload& p) noexcept
Expand Down
2 changes: 1 addition & 1 deletion src/renderer/atlas/BackendD2D.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ namespace Microsoft::Console::Render::Atlas
til::generation_t _generation;
til::generation_t _fontGeneration;
til::generation_t _cursorGeneration;
u16x2 _cellCount{};
u16x2 _viewportCellCount{};

#if ATLAS_DEBUG_SHOW_DIRTY
i32r _presentRects[9]{};
Expand Down
20 changes: 10 additions & 10 deletions src/renderer/atlas/BackendD3D.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -269,7 +269,7 @@ void BackendD3D::_handleSettingsUpdate(const RenderingPayload& p)

const auto fontChanged = _fontGeneration != p.s->font.generation();
const auto miscChanged = _miscGeneration != p.s->misc.generation();
const auto cellCountChanged = _cellCount != p.s->cellCount;
const auto cellCountChanged = _viewportCellCount != p.s->viewportCellCount;

if (fontChanged)
{
Expand Down Expand Up @@ -298,7 +298,7 @@ void BackendD3D::_handleSettingsUpdate(const RenderingPayload& p)
_fontGeneration = p.s->font.generation();
_miscGeneration = p.s->misc.generation();
_targetSize = p.s->targetSize;
_cellCount = p.s->cellCount;
_viewportCellCount = p.s->viewportCellCount;
}

void BackendD3D::_updateFontDependents(const RenderingPayload& p)
Expand Down Expand Up @@ -509,8 +509,8 @@ void BackendD3D::_recreateBackgroundColorBitmap(const RenderingPayload& p)
_backgroundBitmapView.reset();

const D3D11_TEXTURE2D_DESC desc{
.Width = p.s->cellCount.x,
.Height = p.s->cellCount.y,
.Width = p.s->viewportCellCount.x,
.Height = p.s->viewportCellCount.y,
.MipLevels = 1,
.ArraySize = 1,
.Format = DXGI_FORMAT_R8G8B8A8_UNORM,
Expand All @@ -534,8 +534,8 @@ void BackendD3D::_recreateConstBuffer(const RenderingPayload& p) const
{
PSConstBuffer data{};
data.backgroundColor = colorFromU32Premultiply<f32x4>(p.s->misc->backgroundColor);
data.cellSize = { static_cast<f32>(p.s->font->cellSize.x), static_cast<f32>(p.s->font->cellSize.y) };
data.cellCount = { static_cast<f32>(p.s->cellCount.x), static_cast<f32>(p.s->cellCount.y) };
data.backgroundCellSize = { static_cast<f32>(p.s->font->cellSize.x), static_cast<f32>(p.s->font->cellSize.y) };
data.backgroundCellCount = { static_cast<f32>(p.s->viewportCellCount.x), static_cast<f32>(p.s->viewportCellCount.y) };
DWrite_GetGammaRatios(_gamma, data.gammaRatios);
data.enhancedContrast = p.s->font->antialiasingMode == AntialiasingMode::ClearType ? _cleartypeEnhancedContrast : _grayscaleEnhancedContrast;
data.underlineWidth = p.s->font->underline.height;
Expand Down Expand Up @@ -891,7 +891,7 @@ void BackendD3D::_flushQuads(const RenderingPayload& p)
void BackendD3D::_recreateInstanceBuffers(const RenderingPayload& p)
{
// We use the viewport size of the terminal as the initial estimate for the amount of instances we'll see.
const auto minCapacity = static_cast<size_t>(p.s->cellCount.x) * p.s->cellCount.y;
const auto minCapacity = static_cast<size_t>(p.s->viewportCellCount.x) * p.s->viewportCellCount.y;
auto newCapacity = std::max(_instancesCount, minCapacity);
auto newSize = newCapacity * sizeof(QuadInstance);
// Round up to multiples of 64kB to avoid reallocating too often.
Expand Down Expand Up @@ -1090,7 +1090,7 @@ void BackendD3D::_drawTextOverlapSplit(const RenderingPayload& p, u16 y)

i32 columnAdvance = 1;
i32 columnAdvanceInPx{ p.s->font->cellSize.x };
i32 cellCount{ p.s->cellCount.x };
i32 cellCount{ p.s->viewportCellCount.x };

if (p.rows[y]->lineRendition != LineRendition::SingleWidth)
{
Expand Down Expand Up @@ -2092,8 +2092,8 @@ void BackendD3D::_executeCustomShader(RenderingPayload& p)
.time = std::chrono::duration<f32>(std::chrono::steady_clock::now() - _customShaderStartTime).count(),
.scale = static_cast<f32>(p.s->font->dpi) / static_cast<f32>(USER_DEFAULT_SCREEN_DPI),
.resolution = {
static_cast<f32>(_cellCount.x * p.s->font->cellSize.x),
static_cast<f32>(_cellCount.y * p.s->font->cellSize.y),
static_cast<f32>(_viewportCellCount.x * p.s->font->cellSize.x),
static_cast<f32>(_viewportCellCount.y * p.s->font->cellSize.y),
},
.background = colorFromU32Premultiply<f32x4>(p.s->misc->backgroundColor),
};
Expand Down
Loading

0 comments on commit 89c39b0

Please sign in to comment.