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

Gaps between block elements when using Direct2D #17224

Closed
o-sdn-o opened this issue May 9, 2024 · 8 comments · Fixed by #17278
Closed

Gaps between block elements when using Direct2D #17224

o-sdn-o opened this issue May 9, 2024 · 8 comments · Fixed by #17278
Assignees
Labels
Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Area-Settings UI Anything specific to the SUI Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Product-Terminal The new Windows Terminal.

Comments

@o-sdn-o
Copy link

o-sdn-o commented May 9, 2024

Windows Terminal version

Current main

Windows build number

10.0.19045.4291

Other Software

No response

Steps to reproduce

  • System-wide scale and layout (display settings): 100%

  • WT PowerShell profile settings:

    • Font face: "Cascadia Mono"
    • Font size: "12.2"
    • Line height: "1.2"
    • Font weight: "Normal"
    • Builtin Glyphs: "On"
    • image
    • Graphics API: "Direct2D"
    • image
  • Type "▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄" at the pwsh prompt

    "▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄"

Expected Behavior

Block elements are fused into a solid bar.

Actual Behavior

image

@o-sdn-o o-sdn-o added Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels May 9, 2024
Copy link

github-actions bot commented May 9, 2024

Hi I'm an AI powered bot that finds similar issues based off the issue title.

Please view the issues below to see if they solve your problem, and if the issue describes your problem please consider closing this one and thumbs upping the other issue to help us prioritize it. Thank you!

Open similar issues:

Closed similar issues:

Note: You can give me feedback by thumbs upping or thumbs downing this comment.

@o-sdn-o o-sdn-o changed the title Box drawing glyphs are not pixel aligned Block elements are not pixel aligned May 9, 2024
@o-sdn-o o-sdn-o changed the title Block elements are not pixel aligned Gaps between block elements May 9, 2024
@o-sdn-o
Copy link
Author

o-sdn-o commented May 9, 2024

It seems that despite the fact that Direct2D provides graphics acceleration, builtin glyphs are not activated in this mode. If this is intended, then it would probably be worth rephrasing the "Builtin Glyphs On/Off" option description, specifically indicating the need for "Direct3D".

// _p.s->font->builtinGlyphs is the setting which decides whether we should map box drawing glyphs to
// our own builtin versions. There's just one problem: BackendD2D doesn't have this functionality.
// But since AtlasEngine shapes the text before it's handed to the backends, it would need to know
// whether BackendD2D is in use, before BackendD2D even exists. These two flags solve the issue
// by triggering a complete, immediate redraw whenever the backend type changes.
//
// The proper solution is to move text shaping into the backends.
// Someone just needs to write a generic "TextBuffer to DWRITE_GLYPH_RUN" function.
bool _hackIsBackendD2D = false;
bool _hackWantsBuiltinGlyphs = true;
bool _hackTriggerRedrawAll = false;

switch (graphicsAPI)
{
case GraphicsAPI::Direct2D:
_b = std::make_unique<BackendD2D>();
_hackIsBackendD2D = true;
break;
default:
_b = std::make_unique<BackendD3D>(_p);
_hackIsBackendD2D = false;
break;
}
// This ensures that the backends redraw their entire viewports whenever a new swap chain is created,
// EVEN IF we got called when no actual settings changed (i.e. rendering failure, etc.).
_p.MarkAllAsDirty();
const auto hackWantsBuiltinGlyphs = _p.s->font->builtinGlyphs && !_hackIsBackendD2D;
_hackTriggerRedrawAll = _hackWantsBuiltinGlyphs != hackWantsBuiltinGlyphs;
_hackWantsBuiltinGlyphs = hackWantsBuiltinGlyphs;

@o-sdn-o o-sdn-o changed the title Gaps between block elements Gaps between block elements when using Direct2D May 9, 2024
@lhecker
Copy link
Member

lhecker commented May 10, 2024

Right... I wrote that "This feature only works when GPU Acceleration is available." text before I added the Graphics API setting (at which time that statement was mostly true), but now that doesn't work anymore of course. The question is, which one is best?

  • This feature is only available when the Direct3D Graphics API is used.
  • This feature is unavailable when the Direct2D Graphics API is used.
  • This feature is unavailable when Direct2D is used for text rendering.
  • something else?

@o-sdn-o
Copy link
Author

o-sdn-o commented May 10, 2024

It seems to me that since the "Builtin Glyphs" option is completely dependent on the "Graphics API" option, it makes sense to make it a sub-option of the "Graphics API" option that becomes available when "Direct3D 11" is selected.

I don't think the "Builtin Glyphs" option should necessarily be per-profile, as a global option it is quite appropriate for me.

@zadjii-msft
Copy link
Member

Oof that's a tricky one. Cause I'd expect it on the appearance page, but the renderer option doesn't really need to be on the appearance page. That's definitely an advance option.

Maybe when d2d is enabled, we could disable the builtin glyphs menu item, and add a message box with a deeplink to the advanced page... but that sounds overly complicated.

I guess making it a sub-item of the renderer entry is probably the most elegant solution

@zadjii-msft zadjii-msft added Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Product-Terminal The new Windows Terminal. Area-Settings UI Anything specific to the SUI labels May 10, 2024
@zadjii-msft zadjii-msft added this to the Terminal v1.22 milestone May 10, 2024
@lhecker
Copy link
Member

lhecker commented May 10, 2024

I suspect most people will keep it on the default option "Automatic", in which case Direct2D is only used if the system lacks a GPU (or it's a very old GPU, but that's fairly rare nowadays). This is commonly the case when using RDP to remote into a server or VM.

The reason I put the "Builtin Glyphs" option on the profile appearance page is because the font setting is per-profile and so I guess the builtin-glyph option (which overwrites glyphs in the font) should be per-profile too.

The problem is that the settings UI doesn't really have any idea about this "Automatic" API choice. It could be hooked up of course, but I think that's somewhat annoying to do (different DLLs, with different components, on different threads, and it may change at any given moment = needs event hooks). The upside on the other hand is potentially fairly small, because >95% of users are using the Direct3D renderer anyway.

Unless Dustin/Mike think about this differently, I think I'll only change the sub-text on the setting for now. If more people run into this issue (even just a few), I'll try to improve it by hooking up the render thread to the settings UI, or something similar.

@tusharsnx
Copy link
Contributor

tusharsnx commented May 10, 2024

I'd love those specific feature toggles to get grayed out when the feature is unavailable for any reason. We could give additional context as to why they're grayed out with a ⓘ button treatment to the grayed-out toggles.

image

@lhecker lhecker added the Needs-Discussion Something that requires a team discussion before we can proceed label May 13, 2024
@zadjii-msft
Copy link
Member

Unanimous team consent: It's just easier to add builtin glyph support to d2d rather than finagle the SUI to support this

@zadjii-msft zadjii-msft removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Discussion Something that requires a team discussion before we can proceed labels May 13, 2024
github-merge-queue bot pushed a commit that referenced this issue May 17, 2024
#17278)

This implements builtin glyphs for our Direct2D renderer, as well as
dashed and curly underlines. With this in place the only two features
it doesn't support are inverted cursors and VT soft fonts.
This allows us to remove the `_hack*` members introduced in a6a0e44.

The implementation of dashed underlines is trivial, while curly
underlines use quadratic bezier curves. Caching the curve as a sprite
is possible, however I feel like that can be done in the future.

Builtin glyphs on the other hand require a cache, because otherwise
filling the entire viewport with shaded glyphs would result in poor
performance. This is why it's built on top of `ID2D1SpriteBatch`.
Unfortunately the API causes an eager flush of other pending graphics
instructions, which is why there's still a decent perf hit.

Finally, as a little extra, this fixes the rounded powerline glyph
shapes being slightly cut off. The fix is to simply don't round the
position and radius of the ellipsis/semi-circle.

Closes #17224

## Validation Steps Performed
* RenderingTests.exe updated ✅
* All supported builtin glyphs look sorta right at different sizes ✅
@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Tag-Fix Doesn't match tag requirements label May 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Area-Settings UI Anything specific to the SUI Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants