Skip to content

Commit

Permalink
Fix accuracy bugs around float/double/int conversions (#15098)
Browse files Browse the repository at this point in the history
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 ✅
  • Loading branch information
lhecker authored Apr 4, 2023
1 parent da995a0 commit 2a839d8
Show file tree
Hide file tree
Showing 7 changed files with 55 additions and 59 deletions.
37 changes: 17 additions & 20 deletions src/cascadia/TerminalControl/ControlCore.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -256,9 +256,9 @@ namespace winrt::Microsoft::Terminal::Control::implementation
_AttachedHandlers(*this, nullptr);
}

bool ControlCore::Initialize(const double actualWidth,
const double actualHeight,
const double compositionScale)
bool ControlCore::Initialize(const float actualWidth,
const float actualHeight,
const float compositionScale)
{
assert(_settings);

Expand Down Expand Up @@ -298,8 +298,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
// and react accordingly.
_updateFont(true);

const til::size windowSize{ static_cast<til::CoordType>(windowWidth),
static_cast<til::CoordType>(windowHeight) };
const til::size windowSize{ til::math::rounding, windowWidth, windowHeight };

// First set up the dx engine with the window size in pixels.
// Then, using the font, get the number of characters that can fit.
Expand Down Expand Up @@ -853,8 +852,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
// concerned with initialization process. Value forwarded to event handler.
void ControlCore::_updateFont(const bool initialUpdate)
{
const auto newDpi = static_cast<int>(static_cast<double>(USER_DEFAULT_SCREEN_DPI) *
_compositionScale);
const auto newDpi = static_cast<int>(lrint(_compositionScale * USER_DEFAULT_SCREEN_DPI));

_terminal->SetFontInfo(_actualFont);

Expand Down Expand Up @@ -975,8 +973,8 @@ namespace winrt::Microsoft::Terminal::Control::implementation
return;
}

auto cx = gsl::narrow_cast<til::CoordType>(_panelWidth * _compositionScale);
auto cy = gsl::narrow_cast<til::CoordType>(_panelHeight * _compositionScale);
auto cx = gsl::narrow_cast<til::CoordType>(lrint(_panelWidth * _compositionScale));
auto cy = gsl::narrow_cast<til::CoordType>(lrint(_panelHeight * _compositionScale));

// 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.
Expand All @@ -986,7 +984,6 @@ namespace winrt::Microsoft::Terminal::Control::implementation
// Convert our new dimensions to characters
const auto viewInPixels = Viewport::FromDimensions({ 0, 0 }, { cx, cy });
const auto vp = _renderEngine->GetViewportInCharacters(viewInPixels);
const auto currentVP = _terminal->GetViewport();

_terminal->ClearSelection();

Expand All @@ -1005,13 +1002,13 @@ namespace winrt::Microsoft::Terminal::Control::implementation
}
}

void ControlCore::SizeChanged(const double width,
const double height)
void ControlCore::SizeChanged(const float width,
const float height)
{
SizeOrScaleChanged(width, height, _compositionScale);
}

void ControlCore::ScaleChanged(const double scale)
void ControlCore::ScaleChanged(const float scale)
{
if (!_renderEngine)
{
Expand All @@ -1020,24 +1017,24 @@ namespace winrt::Microsoft::Terminal::Control::implementation
SizeOrScaleChanged(_panelWidth, _panelHeight, scale);
}

void ControlCore::SizeOrScaleChanged(const double width,
const double height,
const double scale)
void ControlCore::SizeOrScaleChanged(const float width,
const float height,
const float scale)
{
const auto scaleChanged = _compositionScale != scale;
// _refreshSizeUnderLock redraws the entire terminal.
// Don't call it if we don't have to.
if (_panelWidth == width && _panelHeight == height && _compositionScale == scale)
if (_panelWidth == width && _panelHeight == height && !scaleChanged)
{
return;
}
const auto oldScale = _compositionScale;

_panelWidth = width;
_panelHeight = height;
_compositionScale = scale;

auto lock = _terminal->LockForWriting();
if (oldScale != scale)
if (scaleChanged)
{
// _updateFont relies on the new _compositionScale set above
_updateFont();
Expand Down Expand Up @@ -1267,7 +1264,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
winrt::Windows::Foundation::Size ControlCore::FontSizeInDips() const
{
const auto fontSize = _actualFont.GetSize();
const auto scale = 1.0f / static_cast<float>(_compositionScale);
const auto scale = 1.0f / _compositionScale;
return {
fontSize.width * scale,
fontSize.height * scale,
Expand Down
18 changes: 9 additions & 9 deletions src/cascadia/TerminalControl/ControlCore.h
Original file line number Diff line number Diff line change
Expand Up @@ -58,9 +58,9 @@ namespace winrt::Microsoft::Terminal::Control::implementation
TerminalConnection::ITerminalConnection connection);
~ControlCore();

bool Initialize(const double actualWidth,
const double actualHeight,
const double compositionScale);
bool Initialize(const float actualWidth,
const float actualHeight,
const float compositionScale);
void EnablePainting();

void Detach();
Expand All @@ -78,9 +78,9 @@ namespace winrt::Microsoft::Terminal::Control::implementation
uint64_t SwapChainHandle() const;
void AttachToNewControl(const Microsoft::Terminal::Control::IKeyBindings& keyBindings);

void SizeChanged(const double width, const double height);
void ScaleChanged(const double scale);
void SizeOrScaleChanged(const double width, const double height, const double scale);
void SizeChanged(const float width, const float height);
void ScaleChanged(const float scale);
void SizeOrScaleChanged(const float width, const float height, const float scale);

void AdjustFontSize(float fontSizeDelta);
void ResetFontSize();
Expand Down Expand Up @@ -286,9 +286,9 @@ namespace winrt::Microsoft::Terminal::Control::implementation

// These members represent the size of the surface that we should be
// rendering to.
double _panelWidth{ 0 };
double _panelHeight{ 0 };
double _compositionScale{ 0 };
float _panelWidth{ 0 };
float _panelHeight{ 0 };
float _compositionScale{ 0 };

uint64_t _owningHwnd{ 0 };

Expand Down
12 changes: 6 additions & 6 deletions src/cascadia/TerminalControl/ControlCore.idl
Original file line number Diff line number Diff line change
Expand Up @@ -64,9 +64,9 @@ namespace Microsoft.Terminal.Control
IControlAppearance unfocusedAppearance,
Microsoft.Terminal.TerminalConnection.ITerminalConnection connection);

Boolean Initialize(Double actualWidth,
Double actualHeight,
Double compositionScale);
Boolean Initialize(Single actualWidth,
Single actualHeight,
Single compositionScale);

void UpdateSettings(IControlSettings settings, IControlAppearance appearance);
void ApplyAppearance(Boolean focused);
Expand Down Expand Up @@ -108,9 +108,9 @@ namespace Microsoft.Terminal.Control

void ResetFontSize();
void AdjustFontSize(Single fontSizeDelta);
void SizeChanged(Double width, Double height);
void ScaleChanged(Double scale);
void SizeOrScaleChanged(Double width, Double height, Double scale);
void SizeChanged(Single width, Single height);
void ScaleChanged(Single scale);
void SizeOrScaleChanged(Single width, Single height, Single scale);

void ToggleShaderEffects();
void ToggleReadOnlyMode();
Expand Down
30 changes: 15 additions & 15 deletions src/cascadia/TerminalControl/TermControl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -927,8 +927,8 @@ namespace winrt::Microsoft::Terminal::Control::implementation
return false;
}

const auto panelWidth = SwapChainPanel().ActualWidth();
const auto panelHeight = SwapChainPanel().ActualHeight();
const auto panelWidth = static_cast<float>(SwapChainPanel().ActualWidth());
const auto panelHeight = static_cast<float>(SwapChainPanel().ActualHeight());
const auto panelScaleX = SwapChainPanel().CompositionScaleX();
const auto panelScaleY = SwapChainPanel().CompositionScaleY();

Expand Down Expand Up @@ -2245,7 +2245,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
// instantiating a DxEngine/AtlasEngine.
// GH#10211 - UNDER NO CIRCUMSTANCE should this fail. If it does, the
// whole app will crash instantaneously on launch, which is no good.
double scale;
float scale;
if (settings.UseAtlasEngine())
{
auto engine = std::make_unique<::Microsoft::Console::Render::AtlasEngine>();
Expand All @@ -2267,21 +2267,21 @@ namespace winrt::Microsoft::Terminal::Control::implementation
// ComCtl scrollbars, but it's certainly close enough.
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
if (scrollState != ScrollbarState::Hidden)
{
width += scrollbarSize;
}

double height = rows * actualFontSize.height;
float height = rows * static_cast<float>(actualFontSize.height);
const auto thickness = ParseThicknessFromPadding(padding);
// GH#2061 - make sure to account for the size the padding _will be_ scaled to
width += scale * (thickness.Left + thickness.Right);
height += scale * (thickness.Top + thickness.Bottom);
width += scale * static_cast<float>(thickness.Left + thickness.Right);
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:
Expand Down Expand Up @@ -2311,20 +2311,20 @@ namespace winrt::Microsoft::Terminal::Control::implementation
if (_initializedTerminal)
{
const auto fontSize = _core.FontSize();
double width = fontSize.Width;
double height = fontSize.Height;
auto width = fontSize.Width;
auto height = fontSize.Height;
// Reserve additional space if scrollbar is intended to be visible
if (_core.Settings().ScrollState() != ScrollbarState::Hidden)
{
width += ScrollBar().ActualWidth();
width += static_cast<float>(ScrollBar().ActualWidth());
}

// Account for the size of any padding
const auto padding = GetPadding();
width += padding.Left + padding.Right;
height += padding.Top + padding.Bottom;
width += static_cast<float>(padding.Left + padding.Right);
height += static_cast<float>(padding.Top + padding.Bottom);

return { gsl::narrow_cast<float>(width), gsl::narrow_cast<float>(height) };
return { width, height };
}
else
{
Expand Down Expand Up @@ -2363,7 +2363,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
}

const auto gridSize = dimension - nonTerminalArea;
const auto cells = static_cast<int>(gridSize / fontDimension);
const auto cells = floor(gridSize / fontDimension);
return cells * fontDimension + nonTerminalArea;
}

Expand Down
5 changes: 3 additions & 2 deletions src/cascadia/UnitTests_Control/ControlInteractivityTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1033,8 +1033,9 @@ namespace ControlUnitTests
cursorPosition1.to_core_point());

Log::Comment(L" --- Resize the terminal to be 10 columns wider ---");
const auto newSizeInDips{ til::size{ 40, 20 } * fontSize };
core->SizeChanged(newSizeInDips.width, newSizeInDips.height);
const auto newWidth = 40.0f * fontSize.width;
const auto newHeight = 20.0f * fontSize.height;
core->SizeChanged(newWidth, newHeight);

Log::Comment(L" --- Click on a spot that's NOW INSIDE the buffer ---");
// (32 + 35 + 1) = 68 = 'D'
Expand Down
10 changes: 4 additions & 6 deletions src/cascadia/WindowsTerminal/AppHost.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -579,10 +579,8 @@ void AppHost::_HandleCreateWindow(const HWND hwnd, til::rect proposedRect, Launc

auto initialSize = _windowLogic.GetLaunchDimensions(dpix);

const auto islandWidth = Utils::ClampToShortMax(
static_cast<long>(ceil(initialSize.Width)), 1);
const auto islandHeight = Utils::ClampToShortMax(
static_cast<long>(ceil(initialSize.Height)), 1);
const auto islandWidth = Utils::ClampToShortMax(lrintf(initialSize.Width), 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
// add the titlebar space.
Expand Down Expand Up @@ -1260,8 +1258,8 @@ void AppHost::_handleMoveContent(const winrt::Windows::Foundation::IInspectable&
{
const auto initialSize = _windowLogic.GetLaunchDimensions(dpi);

const auto islandWidth = Utils::ClampToShortMax(static_cast<long>(ceil(initialSize.Width)), 1);
const auto islandHeight = Utils::ClampToShortMax(static_cast<long>(ceil(initialSize.Height)), 1);
const auto islandWidth = Utils::ClampToShortMax(lrintf(initialSize.Width), 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
// add the titlebar space.
Expand Down
2 changes: 1 addition & 1 deletion src/inc/til/math.h
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ namespace til
constexpr O narrow_float(T 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{};
}
Expand Down

0 comments on commit 2a839d8

Please sign in to comment.