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.

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

AtlasEngine: Improve robustness against weird font sizes #17258

Merged
merged 2 commits into from
May 14, 2024

Conversation

lhecker
Copy link
Member

@lhecker lhecker commented May 13, 2024

This clamps the font sizes between 1 and 100. Additionally, it fixes
a warning that I randomly noticed when reproducing the issue: D2D
complained that EndDraw must be called before releasing resources.
Finally, this fixes a crash when the terminal size is exactly (1,1)
cells, which happened because the initial (invalid) size was (1,1) too.

This doesn't fully fix all font-size related issues, but that's
currently difficult to achieve, as for instance the swap chain size
isn't actually based on the window size, nay, it's based on the cell
size multiplied by the cell count. So if the cell size is egregiously
large then we get a swap chain size that's larger than the display and
potentially larger than what the GPU supports which results in errors.

Closes #17227

@lhecker lhecker added Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal. Priority-3 A description (P3) Area-AtlasEngine labels May 13, 2024
@DHowett
Copy link
Member

DHowett commented May 13, 2024

Audit mode go boom

@@ -404,9 +404,9 @@ namespace Microsoft::Console::Render::Atlas
til::generational<CursorSettings> cursor;
til::generational<MiscellaneousSettings> misc;
// Size of the viewport / swap chain in pixel.
u16x2 targetSize{ 1, 1 };
u16x2 targetSize{ 0, 0 };
Copy link
Member

Choose a reason for hiding this comment

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

make sure HwndTerminal doesn't implode here!

Copy link
Member Author

Choose a reason for hiding this comment

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

This is not relevant for HwndTerminal or ControlCore. The problem here is that before bumping the generational revision I check if the targetSize is different and for a target of 1x1 pixel this won't work.

@DHowett
Copy link
Member

DHowett commented May 13, 2024

Added to 1.20 as well

@DHowett DHowett enabled auto-merge May 14, 2024 19:16
@DHowett DHowett added this pull request to the merge queue May 14, 2024
Merged via the queue into main with commit f62d2d5 May 14, 2024
20 checks passed
@DHowett DHowett deleted the dev/lhecker/atlas-engine-font-size-bugs branch May 14, 2024 20:00
DHowett pushed a commit that referenced this pull request May 17, 2024
This clamps the font sizes between 1 and 100. Additionally, it fixes
a warning that I randomly noticed when reproducing the issue: D2D
complained that `EndDraw` must be called before releasing resources.
Finally, this fixes a crash when the terminal size is exactly (1,1)
cells, which happened because the initial (invalid) size was (1,1) too.

This doesn't fully fix all font-size related issues, but that's
currently difficult to achieve, as for instance the swap chain size
isn't actually based on the window size, nay, it's based on the cell
size multiplied by the cell count. So if the cell size is egregiously
large then we get a swap chain size that's larger than the display and
potentially larger than what the GPU supports which results in errors.

Closes #17227

(cherry picked from commit f62d2d5)
Service-Card-Id: 92546470
Service-Version: 1.21
DHowett pushed a commit that referenced this pull request May 17, 2024
This clamps the font sizes between 1 and 100. Additionally, it fixes
a warning that I randomly noticed when reproducing the issue: D2D
complained that `EndDraw` must be called before releasing resources.
Finally, this fixes a crash when the terminal size is exactly (1,1)
cells, which happened because the initial (invalid) size was (1,1) too.

This doesn't fully fix all font-size related issues, but that's
currently difficult to achieve, as for instance the swap chain size
isn't actually based on the window size, nay, it's based on the cell
size multiplied by the cell count. So if the cell size is egregiously
large then we get a swap chain size that's larger than the display and
potentially larger than what the GPU supports which results in errors.

Closes #17227

(cherry picked from commit f62d2d5)
Service-Card-Id: 92546859
Service-Version: 1.20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-AtlasEngine Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Terminal The new Windows Terminal.
Projects
Development

Successfully merging this pull request may close these issues.

Unlimited zoom-in using Ctrl+Wheel causes renderer to crash
3 participants