-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Fix horizontal scrolling bugs in AtlasEngine/DxEngine #15707
Conversation
@@ -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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It also renames
cellCount
toviewportCellCount
, because I realized that it might have to deal with atextBufferCellCount
or similar in the future. I hope that the new name is more descriptive of what it refers to.
gsl::narrow<u16>(std::max(1, srNewViewport.right - srNewViewport.left + 1)), | ||
gsl::narrow<u16>(std::max(1, srNewViewport.bottom - srNewViewport.top + 1)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hope that this makes it a little bit safer than before.
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)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, the only change is that coordTarget.x
is replaced with x
.
coordTarget.x
is in textbuffer-relative coordinates (so when you scroll horizontally, the left edge of the viewport might be something non-0), but we need viewport relative coordinates where the left edge is always 0.
To do so we first need to horizontal scroll-offset viewportOffset.x
from the coordTarget.x
, but this breaks with line renditions, because the caller already divides coordTarget.x
by 2, whereas viewportOffset.x
is not being divided by 2.
And so we first shift viewportOffset.x
to the right, then subtract and then finally shift it to the left again to get viewport-relative, unshifted coordinates.
auto right = std::max(left + cursorWidth, 0); | ||
|
||
if (_p.rows[top]->lineRendition != LineRendition::SingleWidth) | ||
if (_p.cursorRect) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just like above we need to shift to the right, subtract and and then shift to the left. The only additional difference here is that we now need to check if the cursorRect
is empty (out of bounds), because the cursor might be on, but simply scrolled off to the side.
@@ -509,7 +509,7 @@ CATCH_RETURN() | |||
clipRect.top = origin.y + drawingContext->topClipOffset; | |||
clipRect.bottom = origin.y + drawingContext->cellSize.height - drawingContext->bottomClipOffset; | |||
clipRect.left = 0; | |||
clipRect.right = drawingContext->targetSize.width; | |||
clipRect.right = FLT_MAX; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👀
This seems... interesting. Any chance this breaks something by not clipping on the right side anymore? My attempted fix was (I believe)
clipRect.left = origin.x;
on the line above, but I don't know which is better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... I think that would work as well, but only if we also do clipRect.right = origin.x + width
.
I believe FLT_MAX
is correct, because we never limited the left
border of the clip-rect either.
This implies to me that the clipRect.right = drawingContext->targetSize.width;
is just a hold-over from when we didn't draw entire rows at a time.
const auto cursorWidth = 1 + (options.fIsDoubleWidth & (options.cursorType != CursorType::VerticalBar)); | ||
const auto top = options.coordCursor.y; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The todo from 437L is no longer valid?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed
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) ✅ (cherry picked from commit 89c39b0) Service-Card-Id: 90625739 Service-Version: 1.18
This regressed in #15707. By having the `viewportOffset` on the `Settings` object we accidentally invalidate the entire viewport every time it scrolls. That doesn't break anything of course, but it's better to prevent this. This PR additionally contains a fix for clamping the y coordinates coming from `Renderer`: Since `viewportCellCount.y` is a count and thus exclusive, but clamp's max is inclusive, we must subtract 1.
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
toviewportCellCount
, because I realizedthat it might have to deal with a
textBufferCellCount
or similar inthe 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 offsetchanges, and to trim trailing whitespace before shaping text.
This is in preparation for #1860
Validation Steps Performed
RenderingTests
to run in the main (and not alt) buffer