Fix accuracy bugs around float/double/int conversions (#15098)

I noticed this bug while resizing my window on my 150% scale display.
Every 3 "snaps" of the window size, it would fail to resize the text
buffer. I found that this occurs, because we convert the swap chain
size from a float into a double, which converts my 597.333313 height
into 597.33331298828125, which then multiplied by 1.5 results in
895.999969482421875. If you just cast this to an integer, it'll
result in a height of 895px instead of the expected 896px.

This PR addresses the issue in two ways:
* Replace casts to integers with `lrint` or `floor`, etc.
* Remove many of the redundant double <> float conversions.

## PR Checklist
* Resizing my window always resizes the text buffer 
This commit is contained in:
Leonard Hecker 2023-04-04 18:33:17 +02:00 committed by GitHub
parent da995a014f
commit 2a839d8c5a
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
7 changed files with 55 additions and 59 deletions

View File

@ -256,9 +256,9 @@ namespace winrt::Microsoft::Terminal::Control::implementation
_AttachedHandlers(*this, nullptr); _AttachedHandlers(*this, nullptr);
} }
bool ControlCore::Initialize(const double actualWidth, bool ControlCore::Initialize(const float actualWidth,
const double actualHeight, const float actualHeight,
const double compositionScale) const float compositionScale)
{ {
assert(_settings); assert(_settings);
@ -298,8 +298,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
// and react accordingly. // and react accordingly.
_updateFont(true); _updateFont(true);
const til::size windowSize{ static_cast<til::CoordType>(windowWidth), const til::size windowSize{ til::math::rounding, windowWidth, windowHeight };
static_cast<til::CoordType>(windowHeight) };
// First set up the dx engine with the window size in pixels. // First set up the dx engine with the window size in pixels.
// Then, using the font, get the number of characters that can fit. // Then, using the font, get the number of characters that can fit.
@ -853,8 +852,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
// concerned with initialization process. Value forwarded to event handler. // concerned with initialization process. Value forwarded to event handler.
void ControlCore::_updateFont(const bool initialUpdate) void ControlCore::_updateFont(const bool initialUpdate)
{ {
const auto newDpi = static_cast<int>(static_cast<double>(USER_DEFAULT_SCREEN_DPI) * const auto newDpi = static_cast<int>(lrint(_compositionScale * USER_DEFAULT_SCREEN_DPI));
_compositionScale);
_terminal->SetFontInfo(_actualFont); _terminal->SetFontInfo(_actualFont);
@ -975,8 +973,8 @@ namespace winrt::Microsoft::Terminal::Control::implementation
return; return;
} }
auto cx = gsl::narrow_cast<til::CoordType>(_panelWidth * _compositionScale); auto cx = gsl::narrow_cast<til::CoordType>(lrint(_panelWidth * _compositionScale));
auto cy = gsl::narrow_cast<til::CoordType>(_panelHeight * _compositionScale); auto cy = gsl::narrow_cast<til::CoordType>(lrint(_panelHeight * _compositionScale));
// Don't actually resize so small that a single character wouldn't fit // Don't actually resize so small that a single character wouldn't fit
// in either dimension. The buffer really doesn't like being size 0. // in either dimension. The buffer really doesn't like being size 0.
@ -986,7 +984,6 @@ namespace winrt::Microsoft::Terminal::Control::implementation
// Convert our new dimensions to characters // Convert our new dimensions to characters
const auto viewInPixels = Viewport::FromDimensions({ 0, 0 }, { cx, cy }); const auto viewInPixels = Viewport::FromDimensions({ 0, 0 }, { cx, cy });
const auto vp = _renderEngine->GetViewportInCharacters(viewInPixels); const auto vp = _renderEngine->GetViewportInCharacters(viewInPixels);
const auto currentVP = _terminal->GetViewport();
_terminal->ClearSelection(); _terminal->ClearSelection();
@ -1005,13 +1002,13 @@ namespace winrt::Microsoft::Terminal::Control::implementation
} }
} }
void ControlCore::SizeChanged(const double width, void ControlCore::SizeChanged(const float width,
const double height) const float height)
{ {
SizeOrScaleChanged(width, height, _compositionScale); SizeOrScaleChanged(width, height, _compositionScale);
} }
void ControlCore::ScaleChanged(const double scale) void ControlCore::ScaleChanged(const float scale)
{ {
if (!_renderEngine) if (!_renderEngine)
{ {
@ -1020,24 +1017,24 @@ namespace winrt::Microsoft::Terminal::Control::implementation
SizeOrScaleChanged(_panelWidth, _panelHeight, scale); SizeOrScaleChanged(_panelWidth, _panelHeight, scale);
} }
void ControlCore::SizeOrScaleChanged(const double width, void ControlCore::SizeOrScaleChanged(const float width,
const double height, const float height,
const double scale) const float scale)
{ {
const auto scaleChanged = _compositionScale != scale;
// _refreshSizeUnderLock redraws the entire terminal. // _refreshSizeUnderLock redraws the entire terminal.
// Don't call it if we don't have to. // Don't call it if we don't have to.
if (_panelWidth == width && _panelHeight == height && _compositionScale == scale) if (_panelWidth == width && _panelHeight == height && !scaleChanged)
{ {
return; return;
} }
const auto oldScale = _compositionScale;
_panelWidth = width; _panelWidth = width;
_panelHeight = height; _panelHeight = height;
_compositionScale = scale; _compositionScale = scale;
auto lock = _terminal->LockForWriting(); auto lock = _terminal->LockForWriting();
if (oldScale != scale) if (scaleChanged)
{ {
// _updateFont relies on the new _compositionScale set above // _updateFont relies on the new _compositionScale set above
_updateFont(); _updateFont();
@ -1267,7 +1264,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
winrt::Windows::Foundation::Size ControlCore::FontSizeInDips() const winrt::Windows::Foundation::Size ControlCore::FontSizeInDips() const
{ {
const auto fontSize = _actualFont.GetSize(); const auto fontSize = _actualFont.GetSize();
const auto scale = 1.0f / static_cast<float>(_compositionScale); const auto scale = 1.0f / _compositionScale;
return { return {
fontSize.width * scale, fontSize.width * scale,
fontSize.height * scale, fontSize.height * scale,

View File

@ -58,9 +58,9 @@ namespace winrt::Microsoft::Terminal::Control::implementation
TerminalConnection::ITerminalConnection connection); TerminalConnection::ITerminalConnection connection);
~ControlCore(); ~ControlCore();
bool Initialize(const double actualWidth, bool Initialize(const float actualWidth,
const double actualHeight, const float actualHeight,
const double compositionScale); const float compositionScale);
void EnablePainting(); void EnablePainting();
void Detach(); void Detach();
@ -78,9 +78,9 @@ namespace winrt::Microsoft::Terminal::Control::implementation
uint64_t SwapChainHandle() const; uint64_t SwapChainHandle() const;
void AttachToNewControl(const Microsoft::Terminal::Control::IKeyBindings& keyBindings); void AttachToNewControl(const Microsoft::Terminal::Control::IKeyBindings& keyBindings);
void SizeChanged(const double width, const double height); void SizeChanged(const float width, const float height);
void ScaleChanged(const double scale); void ScaleChanged(const float scale);
void SizeOrScaleChanged(const double width, const double height, const double scale); void SizeOrScaleChanged(const float width, const float height, const float scale);
void AdjustFontSize(float fontSizeDelta); void AdjustFontSize(float fontSizeDelta);
void ResetFontSize(); void ResetFontSize();
@ -286,9 +286,9 @@ namespace winrt::Microsoft::Terminal::Control::implementation
// These members represent the size of the surface that we should be // These members represent the size of the surface that we should be
// rendering to. // rendering to.
double _panelWidth{ 0 }; float _panelWidth{ 0 };
double _panelHeight{ 0 }; float _panelHeight{ 0 };
double _compositionScale{ 0 }; float _compositionScale{ 0 };
uint64_t _owningHwnd{ 0 }; uint64_t _owningHwnd{ 0 };

View File

@ -64,9 +64,9 @@ namespace Microsoft.Terminal.Control
IControlAppearance unfocusedAppearance, IControlAppearance unfocusedAppearance,
Microsoft.Terminal.TerminalConnection.ITerminalConnection connection); Microsoft.Terminal.TerminalConnection.ITerminalConnection connection);
Boolean Initialize(Double actualWidth, Boolean Initialize(Single actualWidth,
Double actualHeight, Single actualHeight,
Double compositionScale); Single compositionScale);
void UpdateSettings(IControlSettings settings, IControlAppearance appearance); void UpdateSettings(IControlSettings settings, IControlAppearance appearance);
void ApplyAppearance(Boolean focused); void ApplyAppearance(Boolean focused);
@ -108,9 +108,9 @@ namespace Microsoft.Terminal.Control
void ResetFontSize(); void ResetFontSize();
void AdjustFontSize(Single fontSizeDelta); void AdjustFontSize(Single fontSizeDelta);
void SizeChanged(Double width, Double height); void SizeChanged(Single width, Single height);
void ScaleChanged(Double scale); void ScaleChanged(Single scale);
void SizeOrScaleChanged(Double width, Double height, Double scale); void SizeOrScaleChanged(Single width, Single height, Single scale);
void ToggleShaderEffects(); void ToggleShaderEffects();
void ToggleReadOnlyMode(); void ToggleReadOnlyMode();

View File

@ -927,8 +927,8 @@ namespace winrt::Microsoft::Terminal::Control::implementation
return false; return false;
} }
const auto panelWidth = SwapChainPanel().ActualWidth(); const auto panelWidth = static_cast<float>(SwapChainPanel().ActualWidth());
const auto panelHeight = SwapChainPanel().ActualHeight(); const auto panelHeight = static_cast<float>(SwapChainPanel().ActualHeight());
const auto panelScaleX = SwapChainPanel().CompositionScaleX(); const auto panelScaleX = SwapChainPanel().CompositionScaleX();
const auto panelScaleY = SwapChainPanel().CompositionScaleY(); const auto panelScaleY = SwapChainPanel().CompositionScaleY();
@ -2245,7 +2245,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
// instantiating a DxEngine/AtlasEngine. // instantiating a DxEngine/AtlasEngine.
// GH#10211 - UNDER NO CIRCUMSTANCE should this fail. If it does, the // GH#10211 - UNDER NO CIRCUMSTANCE should this fail. If it does, the
// whole app will crash instantaneously on launch, which is no good. // whole app will crash instantaneously on launch, which is no good.
double scale; float scale;
if (settings.UseAtlasEngine()) if (settings.UseAtlasEngine())
{ {
auto engine = std::make_unique<::Microsoft::Console::Render::AtlasEngine>(); auto engine = std::make_unique<::Microsoft::Console::Render::AtlasEngine>();
@ -2267,7 +2267,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
// ComCtl scrollbars, but it's certainly close enough. // ComCtl scrollbars, but it's certainly close enough.
const auto scrollbarSize = GetSystemMetricsForDpi(SM_CXVSCROLL, dpi); const auto scrollbarSize = GetSystemMetricsForDpi(SM_CXVSCROLL, dpi);
double width = cols * actualFontSize.width; float width = cols * static_cast<float>(actualFontSize.width);
// Reserve additional space if scrollbar is intended to be visible // Reserve additional space if scrollbar is intended to be visible
if (scrollState != ScrollbarState::Hidden) if (scrollState != ScrollbarState::Hidden)
@ -2275,13 +2275,13 @@ namespace winrt::Microsoft::Terminal::Control::implementation
width += scrollbarSize; width += scrollbarSize;
} }
double height = rows * actualFontSize.height; float height = rows * static_cast<float>(actualFontSize.height);
const auto thickness = ParseThicknessFromPadding(padding); const auto thickness = ParseThicknessFromPadding(padding);
// GH#2061 - make sure to account for the size the padding _will be_ scaled to // GH#2061 - make sure to account for the size the padding _will be_ scaled to
width += scale * (thickness.Left + thickness.Right); width += scale * static_cast<float>(thickness.Left + thickness.Right);
height += scale * (thickness.Top + thickness.Bottom); height += scale * static_cast<float>(thickness.Top + thickness.Bottom);
return { gsl::narrow_cast<float>(width), gsl::narrow_cast<float>(height) }; return { width, height };
} }
// Method Description: // Method Description:
@ -2311,20 +2311,20 @@ namespace winrt::Microsoft::Terminal::Control::implementation
if (_initializedTerminal) if (_initializedTerminal)
{ {
const auto fontSize = _core.FontSize(); const auto fontSize = _core.FontSize();
double width = fontSize.Width; auto width = fontSize.Width;
double height = fontSize.Height; auto height = fontSize.Height;
// Reserve additional space if scrollbar is intended to be visible // Reserve additional space if scrollbar is intended to be visible
if (_core.Settings().ScrollState() != ScrollbarState::Hidden) if (_core.Settings().ScrollState() != ScrollbarState::Hidden)
{ {
width += ScrollBar().ActualWidth(); width += static_cast<float>(ScrollBar().ActualWidth());
} }
// Account for the size of any padding // Account for the size of any padding
const auto padding = GetPadding(); const auto padding = GetPadding();
width += padding.Left + padding.Right; width += static_cast<float>(padding.Left + padding.Right);
height += padding.Top + padding.Bottom; height += static_cast<float>(padding.Top + padding.Bottom);
return { gsl::narrow_cast<float>(width), gsl::narrow_cast<float>(height) }; return { width, height };
} }
else else
{ {
@ -2363,7 +2363,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
} }
const auto gridSize = dimension - nonTerminalArea; const auto gridSize = dimension - nonTerminalArea;
const auto cells = static_cast<int>(gridSize / fontDimension); const auto cells = floor(gridSize / fontDimension);
return cells * fontDimension + nonTerminalArea; return cells * fontDimension + nonTerminalArea;
} }

View File

@ -1033,8 +1033,9 @@ namespace ControlUnitTests
cursorPosition1.to_core_point()); cursorPosition1.to_core_point());
Log::Comment(L" --- Resize the terminal to be 10 columns wider ---"); Log::Comment(L" --- Resize the terminal to be 10 columns wider ---");
const auto newSizeInDips{ til::size{ 40, 20 } * fontSize }; const auto newWidth = 40.0f * fontSize.width;
core->SizeChanged(newSizeInDips.width, newSizeInDips.height); const auto newHeight = 20.0f * fontSize.height;
core->SizeChanged(newWidth, newHeight);
Log::Comment(L" --- Click on a spot that's NOW INSIDE the buffer ---"); Log::Comment(L" --- Click on a spot that's NOW INSIDE the buffer ---");
// (32 + 35 + 1) = 68 = 'D' // (32 + 35 + 1) = 68 = 'D'

View File

@ -579,10 +579,8 @@ void AppHost::_HandleCreateWindow(const HWND hwnd, til::rect proposedRect, Launc
auto initialSize = _windowLogic.GetLaunchDimensions(dpix); auto initialSize = _windowLogic.GetLaunchDimensions(dpix);
const auto islandWidth = Utils::ClampToShortMax( const auto islandWidth = Utils::ClampToShortMax(lrintf(initialSize.Width), 1);
static_cast<long>(ceil(initialSize.Width)), 1); const auto islandHeight = Utils::ClampToShortMax(lrintf(initialSize.Height), 1);
const auto islandHeight = Utils::ClampToShortMax(
static_cast<long>(ceil(initialSize.Height)), 1);
// Get the size of a window we'd need to host that client rect. This will // Get the size of a window we'd need to host that client rect. This will
// add the titlebar space. // add the titlebar space.
@ -1260,8 +1258,8 @@ void AppHost::_handleMoveContent(const winrt::Windows::Foundation::IInspectable&
{ {
const auto initialSize = _windowLogic.GetLaunchDimensions(dpi); const auto initialSize = _windowLogic.GetLaunchDimensions(dpi);
const auto islandWidth = Utils::ClampToShortMax(static_cast<long>(ceil(initialSize.Width)), 1); const auto islandWidth = Utils::ClampToShortMax(lrintf(initialSize.Width), 1);
const auto islandHeight = Utils::ClampToShortMax(static_cast<long>(ceil(initialSize.Height)), 1); const auto islandHeight = Utils::ClampToShortMax(lrintf(initialSize.Height), 1);
// Get the size of a window we'd need to host that client rect. This will // Get the size of a window we'd need to host that client rect. This will
// add the titlebar space. // add the titlebar space.

View File

@ -17,7 +17,7 @@ namespace til
constexpr O narrow_float(T val) constexpr O narrow_float(T val)
{ {
const auto o = gsl::narrow_cast<O>(val); const auto o = gsl::narrow_cast<O>(val);
if (std::isnan(val) || static_cast<T>(o) != val) if (static_cast<T>(o) != val)
{ {
throw gsl::narrowing_error{}; throw gsl::narrowing_error{};
} }