AtlasEngine: Minor bug fixes #16219

lhecker · 2023-10-23T16:23:50Z

This commit fixes 4 minor bugs:

Forgot to set the maximum swap chain latency. Without it, it defaults
to up to 3 frames of latency. We don't need this, because our renderer
is simple and fast and is expected to draw frames within <1ms.
ClearType treats the alpha channel as ignored, whereas custom shaders
can manipulate the alpha channel freely. This meant that using both
simultaneously would produce weird effects, like text having black
background. We now force grayscale AA instead.
The builtin retro shader should not be effected by the previous point.
When the cbuffer is entirely unused in a custom shader, it has so far
resulted in constant redraws. This happened because the D3D reflection
GetDesc call will then return E_FAIL in this situation.
The new code on the other hand will now assume that a failure
to get the description is equal to the variable being unused.

Closes #15960

Validation Steps Performed

A custom passthrough shader works with grayscale and ClearType AA
while also changing the opacity with Ctrl+Shift+Scroll ✅
Same for the builtin retro shader, but ClearType works ✅
The passthrough shader doesn't result in constant redrawing ✅

src/renderer/atlas/BackendD3D.cpp

DHowett · 2023-10-23T16:50:46Z

src/renderer/atlas/BackendD3D.cpp

            {
+                // Depending on the version of the d3dcompiler_*.dll, the next two functions either return nullptr
+                // on failure or an instance of CInvalidSRConstantBuffer or CInvalidSRVariable respectively,
+                // which cause GetDesc() to return E_FAIL. In other words, we have to assume that any failure in the


That doesn't seem like what this code does?

This code only changes _requiresContinuous... when we successfully get all the buffers, variables, and descriptors.

Therefore, we just use whatever value was previously set. Isn't that worse?

My answer to all 3 comments is: If you expand the section above this diff, you'll see that it resets all relevant members before making any further changes (including _requiresContinuousRedraw).

UGH thanks. Sorry for the bad code.

carlos-zamora · 2023-10-24T21:35:56Z

src/renderer/atlas/AtlasEngine.api.cpp

    // If the user asks for ClearType, but also for a transparent background
    // (which our ClearType shader doesn't simultaneously support)
    // then we need to sneakily force the renderer to grayscale AA.
-    const auto antialiasingMode = _api.enableTransparentBackground && _api.antialiasingMode == AntialiasingMode::ClearType ? AntialiasingMode::Grayscale : _api.antialiasingMode;
-    const bool enableTransparentBackground = _api.enableTransparentBackground || !_api.s->misc->customPixelShaderPath.empty() || _api.s->misc->useRetroTerminalEffect;


_api.s->misc->useRetroTerminalEffect is completely gone now. Is that intentional?

Yep! It's the 3rd bullet point in the commit message. 😅

DHowett · 2023-10-31T20:48:55Z

builds failed? wot

DHowett · 2023-10-31T20:49:01Z

oh it was a rebuild. ugh sorry.

This commit fixes 4 minor bugs: * Forgot to set the maximum swap chain latency. Without it, it defaults to up to 3 frames of latency. We don't need this, because our renderer is simple and fast and is expected to draw frames within <1ms. * ClearType treats the alpha channel as ignored, whereas custom shaders can manipulate the alpha channel freely. This meant that using both simultaneously would produce weird effects, like text having black background. We now force grayscale AA instead. * The builtin retro shader should not be effected by the previous point. * When the cbuffer is entirely unused in a custom shader, it has so far resulted in constant redraws. This happened because the D3D reflection `GetDesc` call will then return `E_FAIL` in this situation. The new code on the other hand will now assume that a failure to get the description is equal to the variable being unused. Closes #15960 ## Validation Steps Performed * A custom passthrough shader works with grayscale and ClearType AA while also changing the opacity with Ctrl+Shift+Scroll ✅ * Same for the builtin retro shader, but ClearType works ✅ * The passthrough shader doesn't result in constant redrawing ✅ (cherry picked from commit 0289cb0) Service-Card-Id: 90915276 Service-Version: 1.18

This commit fixes 4 minor bugs: * Forgot to set the maximum swap chain latency. Without it, it defaults to up to 3 frames of latency. We don't need this, because our renderer is simple and fast and is expected to draw frames within <1ms. * ClearType treats the alpha channel as ignored, whereas custom shaders can manipulate the alpha channel freely. This meant that using both simultaneously would produce weird effects, like text having black background. We now force grayscale AA instead. * The builtin retro shader should not be effected by the previous point. * When the cbuffer is entirely unused in a custom shader, it has so far resulted in constant redraws. This happened because the D3D reflection `GetDesc` call will then return `E_FAIL` in this situation. The new code on the other hand will now assume that a failure to get the description is equal to the variable being unused. Closes #15960 ## Validation Steps Performed * A custom passthrough shader works with grayscale and ClearType AA while also changing the opacity with Ctrl+Shift+Scroll ✅ * Same for the builtin retro shader, but ClearType works ✅ * The passthrough shader doesn't result in constant redrawing ✅ (cherry picked from commit 0289cb0) Service-Card-Id: 90915277 Service-Version: 1.19

This commit fixes 4 minor bugs: * Forgot to set the maximum swap chain latency. Without it, it defaults to up to 3 frames of latency. We don't need this, because our renderer is simple and fast and is expected to draw frames within <1ms. * ClearType treats the alpha channel as ignored, whereas custom shaders can manipulate the alpha channel freely. This meant that using both simultaneously would produce weird effects, like text having black background. We now force grayscale AA instead. * The builtin retro shader should not be effected by the previous point. * When the cbuffer is entirely unused in a custom shader, it has so far resulted in constant redraws. This happened because the D3D reflection `GetDesc` call will then return `E_FAIL` in this situation. The new code on the other hand will now assume that a failure to get the description is equal to the variable being unused. Closes #15960 ## Validation Steps Performed * A custom passthrough shader works with grayscale and ClearType AA while also changing the opacity with Ctrl+Shift+Scroll ✅ * Same for the builtin retro shader, but ClearType works ✅ * The passthrough shader doesn't result in constant redrawing ✅ (cherry picked from commit 0289cb0) Service-Card-Id: 90915276 Service-Version: 1.18

- AtlasEngine: Minor bug fixes (GH-16219) - Fix the fix for the fix of nearby font loading (GH-16196) - Added selectionBackground to light color schemes (GH-16243) - Another theoretical fix for a crash (GH-16267) - Fix tabs being printed in cmd.exe prompts (GH-16273) Related work items: MSFT-47266988

AtlasEngine: Minor bug fixes

f38dbfc

microsoft-github-policy-service bot added the Issue-Bug It either shouldn't be doing this or needs an investigation. label Oct 23, 2023

microsoft-github-policy-service bot added Area-AtlasEngine Priority-3 A description (P3) Product-Terminal The new Windows Terminal. labels Oct 23, 2023

DHowett requested changes Oct 23, 2023

View reviewed changes

microsoft-github-policy-service bot added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Oct 23, 2023

DHowett approved these changes Oct 24, 2023

View reviewed changes

carlos-zamora reviewed Oct 24, 2023

View reviewed changes

carlos-zamora approved these changes Oct 24, 2023

View reviewed changes

lhecker merged commit 0289cb0 into main Oct 31, 2023
14 of 16 checks passed

lhecker deleted the dev/lhecker/atlasengine-fixup branch October 31, 2023 13:25

adonis-jimenez mentioned this pull request Nov 18, 2023

Windows Terminal Preview v1.19.3172.0 #16337

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AtlasEngine: Minor bug fixes #16219

AtlasEngine: Minor bug fixes #16219

lhecker commented Oct 23, 2023 •

edited

Loading

DHowett Oct 23, 2023

lhecker Oct 23, 2023

DHowett Oct 24, 2023

carlos-zamora Oct 24, 2023

lhecker Oct 24, 2023

DHowett commented Oct 31, 2023

DHowett commented Oct 31, 2023

AtlasEngine: Minor bug fixes #16219

AtlasEngine: Minor bug fixes #16219

Conversation

lhecker commented Oct 23, 2023 • edited Loading

Validation Steps Performed

DHowett Oct 23, 2023

Choose a reason for hiding this comment

lhecker Oct 23, 2023

Choose a reason for hiding this comment

DHowett Oct 24, 2023

Choose a reason for hiding this comment

carlos-zamora Oct 24, 2023

Choose a reason for hiding this comment

lhecker Oct 24, 2023

Choose a reason for hiding this comment

DHowett commented Oct 31, 2023

DHowett commented Oct 31, 2023

lhecker commented Oct 23, 2023 •

edited

Loading