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.

Sign up for GitHub

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 accuracy bugs around float/double/int conversions #15098

Merged
merged 2 commits into from
Apr 4, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c8865d4
Fix accuracy bugs around float/double conversions
lhecker Apr 3, 2023
e2edab0
Fix compilation
lhecker Apr 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 17 additions & 20 deletions src/cascadia/TerminalControl/ControlCore.cpp
View file
Open in desktop
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));
Copy link
Member

Choose a reason for hiding this comment

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

this cast seems extraneous or at the very best silly given the auto

Copy link
Member Author

Choose a reason for hiding this comment

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

Otherwise AuditMode complains that long isn't automatically convertible to int (the parameter type for SetDPI).


_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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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 @@ -1261,8 +1259,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
View file
Open in desktop
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