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: Harden against empty target sizes #15615

Merged

Conversation

lhecker
Copy link
Member

@lhecker lhecker commented Jun 27, 2023

The WPF control has a minor bug where it initializes the renderer
when there isn't even a window yet. When it then calls SetWindowSize
it'll pass the result of GetWindowRect which is 0,0,0,0.
This made AtlasEngine unhappy because it restricted the glyph atlas
size to some multiple of the window size. If the window size is 0,0
then there couldn't be a glyph atlas and so it crashed.

Validation Steps Performed

  • Fixes WPF test control crash on startup ✅

@lhecker lhecker added Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) Area-AtlasEngine labels Jun 27, 2023
Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

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

it did not :D

@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jun 27, 2023
@lhecker lhecker marked this pull request as draft June 27, 2023 22:14
@lhecker
Copy link
Member Author

lhecker commented Jun 28, 2023

How do you test this? With WpfTerminalTestNetCore? I can't reproduce any issues to begin with...

@microsoft-github-policy-service microsoft-github-policy-service bot added Needs-Attention The core contributors need to come back around and look at this ASAP. and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Jun 28, 2023
@DHowett
Copy link
Member

DHowett commented Jun 28, 2023

How do you test this? With WpfTerminalTestNetCore? I can't reproduce any issues to begin with...

it is dangerous to go alone. take this! https://github.com/microsoft/terminal/tree/dev/duhowett/hax/wpf-atlas

@lhecker lhecker force-pushed the dev/lhecker/atlas-engine-fixup branch from d4cccea to 626cdb3 Compare June 30, 2023 13:59
@lhecker lhecker changed the title AtlasEngine: Fix failure to render if the default font is used AtlasEngine: Harden against empty target sizes Jun 30, 2023
@lhecker lhecker marked this pull request as ready for review June 30, 2023 13:59
@lhecker lhecker requested a review from DHowett June 30, 2023 14:00
@lhecker lhecker force-pushed the dev/lhecker/atlas-engine-fixup branch 2 times, most recently from 96d0c3c to 721dc99 Compare June 30, 2023 14:23
gsl::narrow_cast<u16>(clamp<til::CoordType>(pixels.height, 1, u16max)),
};

if (_api.s->targetSize != newSize)
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 PR now fixes the issue from two directions:

  • Avoid ever setting targetSize to anything that has a 0 on either width or height

const auto area = std::min(maxArea, std::min(maxAreaByFont, min));

auto area = std::min(maxAreaByFont, std::max(minAreaByFont, minAreaByGrowth));
area = clamp(area, minArea, maxArea);
Copy link
Member Author

Choose a reason for hiding this comment

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

  • Avoid reducing area below minArea at all times

(minArea should probably be called absoluteMinArea or something.)

Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

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

Love it! Please update the body as well, since it's got a question mark emoji in it

@carlos-zamora carlos-zamora added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jun 30, 2023
DHowett pushed a commit that referenced this pull request Jul 27, 2023
The WPF control has a minor bug where it initializes the renderer
when there isn't even a window yet. When it then calls `SetWindowSize`
it'll pass the result of `GetWindowRect` which is `0,0,0,0`.
This made AtlasEngine unhappy because it restricted the glyph atlas
size to some multiple of the window size. If the window size is `0,0`
then there couldn't be a glyph atlas and so it crashed.

## Validation Steps Performed
* Fixes WPF test control crash on startup ✅

(cherry picked from commit a084834)
Service-Card-Id: 90012548
Service-Version: 1.18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-AtlasEngine AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Attention The core contributors need to come back around and look at this ASAP. Priority-2 A description (P2)
Projects
Development

Successfully merging this pull request may close these issues.

3 participants