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 scroll and swap chain invalidation #15425

Merged
merged 2 commits into from
May 26, 2023

Conversation

lhecker
Copy link
Member

@lhecker lhecker commented May 24, 2023

_p.MarkAllAsDirty() sets _p.scrollOffset to 0, so we need to use
that instead of _api.scrollOffset when getting the offset.
Additionally, the previous code failed to release the swap chain
when recreating the backend, which is technically not correct.
I'm not sure to what issues this might have led, as it didn't had any
negative effects on my PC, but it's definitely not according to spec.

Validation Steps Performed

Difficult to test but it seems alright.

@lhecker lhecker added Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal. Priority-2 A description (P2) Area-AtlasEngine labels May 24, 2023
{
_p.MarkAllAsDirty();
}

if (const auto offset = _api.scrollOffset)
if (const auto offset = _p.scrollOffset)
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 a minor bug. It'll not lead to any obvious issues, it's just wrong. _p.MarkAllAsDirty() above sets the _p.scrollOffset to 0, because it just shouldn't be scrolling anymore when everything's invalid.

I made the rest of the function use _p (p as in "rendering Payload"; yes very creative, I know lol) as well to be consistent throughout the function.

_b->ReleaseResources();
_p.deviceContext->ClearState();
_p.deviceContext->Flush();
}
_createSwapChain();
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 failed to release the swap chain itself before clearing and flushing the device. That's just not right, I think. I'm not sure what effect this could've had (because it never led to any issues for me), but it's definitely not allowed according to D3D11.

_createSwapChain will now call _destroySwapChain.

@@ -162,6 +162,10 @@ void AtlasEngine::_recreateAdapter()

void AtlasEngine::_recreateBackend()
{
// D3D11 defers the destruction of objects and only one swap chain can be associated with a
// HWND, IWindow, or composition surface at a time. --> Destroy it while we still have the old device.
_destroySwapChain();
Copy link
Member Author

Choose a reason for hiding this comment

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

We forgot to flush the device before recreating it (and the swap chain). It's also technically not allowed.

@DHowett
Copy link
Member

DHowett commented May 26, 2023

This is marked for servicing into 1.18, but I can't tell what bug it fixes.

@lhecker
Copy link
Member Author

lhecker commented May 26, 2023

[...] but I can't tell what bug it fixes.

It's a theoretical bug and if it exists it's 100% one of those ambiguous ones. Like "I reconnect via RDP after standby and I got an error message and no output". We should backport it.

@DHowett DHowett merged commit a19d30a into main May 26, 2023
@DHowett DHowett deleted the dev/lhecker/atlas-engine-fixup3 branch May 26, 2023 19:30
DHowett pushed a commit that referenced this pull request May 26, 2023
`_p.MarkAllAsDirty()` sets `_p.scrollOffset` to 0, so we need to use
that instead of `_api.scrollOffset` when getting the offset.
Additionally, the previous code failed to release the swap chain
when recreating the backend, which is technically not correct.
I'm not sure to what issues this might have led, as it didn't had any
negative effects on my PC, but it's definitely not according to spec.

## Validation Steps Performed
Difficult to test but it seems alright.

(cherry picked from commit a19d30a)
Service-Card-Id: 89315195
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 Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) Product-Terminal The new Windows Terminal.
Projects
Development

Successfully merging this pull request may close these issues.

3 participants