-
Notifications
You must be signed in to change notification settings - Fork 8.4k
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
Add support for custom box drawing and powerline glyphs #16729
Conversation
(Note: The look of 3 block shading glyphs was further improved in #16760) |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
I think it gets cut off. It's probably due to rounding errors. I'm also not 100% sure that my math is completely correct. |
081901c
to
ad2391c
Compare
@@ -115,6 +115,7 @@ Author(s): | |||
X(winrt::Windows::UI::Text::FontWeight, FontWeight, "weight", DEFAULT_FONT_WEIGHT) \ | |||
X(IFontAxesMap, FontAxes, "axes") \ | |||
X(IFontFeatureMap, FontFeatures, "features") \ | |||
X(bool, CustomGlyphs, "customGlyphs", true) \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may want to call this compatibility.integratedDrawingGlyphs
or something - IMO and of course open to discussion!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively we could call them "integrated pseudographics" or "builtin semigraphics". apparently those (pseudo/semigraphics) are names for these glyphs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feature label: Built-in Glyphs
Help text: "Use built-in glyphs for BoxDrawing, BlockElement and Powerline characters"
I think this fits well with the future plan where we would draw them even when the font itself may not come with any of those glyphs (Eg. unpatched fonts)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm mostly worried about the JSON name - where there is no help text and "glyphs" is awfully broad (there are a looooottt of glyphs in a font!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm opposed to renaming this setting for 2 reasons:
- wezterm calls it
custom_block_glyphs
- VS Code calls it
terminal.integrated.customGlyphs
So while I agree that we should call it "integrated glyphs" I think consistency with other applications is more important to make it easier for users to find related settings in related applications.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW that's for "vscode's integrated terminal", not for "integrated custom glyphs"
That's fair, however.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In case you're still considering other names, I think both Alacritty and Contour use something like builtin_box_drawing
for this option (as a sub setting of the font
configuration).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reviewed 27/28 (still have BackendD3D to go!)
{ | ||
// Ah, I love these bracketed C-style casts. I use them in C all the time. Yep. | ||
#pragma warning(suppress : 26493) // Don't use C-style casts (type.4). | ||
return (char32_t{ lead } << 10) - 0x35FDC00 + char32_t{ trail }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
horrifying
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but i love it as long as it works
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not too bad actually if you search for 0x35FDC00
on the interwebz. You'll find tons of results of this exact pattern. :)
const auto base = reinterpret_cast<const u16*>(_api.bufferLine.data()); | ||
const auto len = offEnd - offBeg; | ||
|
||
row.glyphIndices.insert(row.glyphIndices.end(), base + offBeg, base + offEnd); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't totally understand what this means for glyph indices. What are we storing for normal characters? What are we storing for box drawing ones?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add a comment. For "custom" glyphs we store the UTF16 code units in the glyph indices array. The reason I do this as opposed to using a 2nd array is so that the hashmap handling code is simpler in BackendD3D
.
At some point my hope is to move the DWrite handling into the 2 backends. That way there won't be any row
s and no glyphIndices
anymore because we'll write the QuadInstance
s directly into the buffer as we're shaping the text. That way this ugliness will also be gone.
// 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 _hackWantsCustomGlyphs = true; | ||
bool _hackTriggerRedrawAll = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry for yor beautiful architecture
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DYING
nit: reword the commit title to be in imperative form (e.g. it can complete the sentence, "This commit will ...") |
Hey @PhMajerus, you might be interested in this! The screenshot of all glyphs above is a bit outdated, since we've fixed the double/single intersections, but other than that this is ready to go in. Wondered if you had thoughts/concerns as an ANSI artist :) |
Hey @DHowett, thanks for including me in the discussion. I'm not really an ANSI artist, more of an 8-bit nostalgic, but I do have some comments about your approach. While I understand the reason to try to get away from font glyphs, I'm concerned about all the other block-sized characters that should be supported in the future. For the box drawing, there are at least 69 more connecting lines, angles, and diagonals. And even if you decided to custom-draw all those, if you ever plan to support really all legacy scenarios, such as less common characters ROM found in some terminals and 8-bit computers, you'll have even more characters that need to join with those and each other, think about MouseText elements from the Apple II for example. Now imagine you did the work to custom-draw all of those, then you'll still leave some more block-sized characters behind. Take the large type pieces, they don't need to join with pseudopixels and box drawing, but they do need to join seamlessly with each other, and while there are standard shapes for those provided by Unicode, they are ugly and actually don't join properly with each other, so they really are just enough to recognize them, but you need to improve them for large text to work. For example, here is my current work on large type pieces for Cascadia, with rounded corners: While the original design uses beveled corners, because it was modern in the 70s when HP originally designed them for its terminals: These cannot be custom-drawn without losing on consistency and font design. To me it seems there are way too many glyphs to handle them all, and picking a custom-drawn solution might mean you spend less effort on trying to reduce the artifacts of the renderer when joining full-block characters. I have even more concerns coming in an upcoming post, because Unicode only standardize glyphs intents, not their actual design, and they use the same codepoint for characters that have the same intent on several legacy systems, but which actually have incompatible designs if you want the terminal to be able to render their original semigraphics contents properly. In short, the same code point will have different pseudopixels patterns depending on the original system, and users may switch between a few fonts depending on what they are doing or which app they launch in different terminal profiles. And all of this is a real possibility, and one of the goals of the hundreds of semigraphics characters coming in Unicode 16. |
Are you aware of an approach that does this properly? I'd be happy to implement it! (BTW I'm not sure if it's relevant to your argument, but the PR makes this a setting. It'll be next to the font size setting.) |
@lhecker I don't have a perfect solution and I do appreciate the thought and work you've put into trying to improve the situation. One of the problem I'm concerned with is consistency, especially with pseudo-pixels mosaics, quadrants reuse existing half-block patterns, sextants reuse half-blocks as well, octants reuse half-blocks, quadrants, and eights fills, a new set of eights are providing horizontal and vertical lines, as well as horizontal and vertical fills from the top and right to complement existing ones from the bottom and left,... I'm not against an option to make the base box drawing and block elements look nicer, but I don't think it is the ultimate solution. Is there any way with the DirectWrite renderer to adjust the glyph size for rendering? If designed properly, all the hundreds of semigraphics of a font should be based on the Full Block Then you don't need to handle some characters in a special way, which is impossible since a font may include PUA characters that are also block-sized. For example, here is a version of my large type pieces with ligatures to further round the corners: It looks nice when the full block size is pretty much a round number of pixels, but at some sizes, it gets misaligned. If the renderer could find out the separate width and height scaling adjustments needed to snap the full block to its cell, and feed these adjustments back into the font renderer, I believe you could achieve perfect seamlessly connection of all the box drawing, pseudo-pixels, large font, and all semigraphics. No more seams or overlaps, no special table of which characters are block-sized, no special handling of "the most important" box drawing and block element characters at the expense of all the others,... Do you think something like that is possible? It is a very special case so maybe DirectWrite doesn't support that, but since you're lucky enough to be the same company, could it be possible if it isn't supported to get a customized version of DirectWrite that supports those adjustments scaling to render the terminal characters grid perfectly? |
If you want to test your rendering changes with those new glyphs, here are some more resources. This is a custom version of Cascadia with the characters I'm adding: Here are the charts, note I didn't do everything yet: Here is an explanation and test document for large type pieces: The Cascadia Code version includes ligatures for several of the corners combinations of large type pieces. The large type pieces are probably the trickiest semigraphics I'm encountered, if you can render those you can probably render everything. As usual, you can find more pseudo-pixels test files at https://github.com/PhMajerus/ANSI-art/tree/main/Unicode If you try it out, run the following: |
The old "DxRenderer" did this for the basic box drawing glyphs. If you currently have "AtlasEngine" enabled in the Rendering settings, you could try disabling it and seeing how it looks then. I think the old DxRenderer could still be fine tuned a bit, but it's probably about as good as it gets.
From my past experience triaging user feedback for Windows Terminal, I believe I need to disagree, even if I agree that you're fundamentally correct. There are two problems I need to prevent if I were to do what you suggest:
This PR solves both problems. Not in a way that I'd like since I fundamentally agree with you, but rather in a way that I think is proven to work, given that other popular terminals do the same thing. I'm open to brainstorm alternative approaches we can add to our text renderer, but I believe this PR is the most solid approach we can have and the one we should have if not at least as a fallback. For instance, given what you recommended, I think we should add another toggle that allows users to scale box drawing glyphs according to the size of the U+2588 glyph. This would make Cascadia work perfectly, while not breaking fonts such as TerminusTTF again. |
@lhecker I think many fonts have been designed by looking at their results in whichever terminal the font designer used daily. Now about the possibility to scale and snap-to-grid every glyph according to It seems like it would also please the users who want pixel-perfect old-school bitmap fonts. If you can scale width and height to snap glyphs to the grid, can you provide an alternative font size configuration in the settings to let users set the font size in pixels width and height instead of points? Finally, just to further make the point that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep this is dx code alright
{ _RegPropertyType::Boolean, CONSOLE_REGISTRY_COPYCOLOR, SET_FIELD_AND_SIZE(_fCopyColor) } | ||
{ _RegPropertyType::Boolean, CONSOLE_REGISTRY_COPYCOLOR, SET_FIELD_AND_SIZE(_fCopyColor) }, | ||
#if TIL_FEATURE_CONHOSTATLASENGINE_ENABLED | ||
{ _RegPropertyType::Boolean, L"EnableBuiltinGlyphs", SET_FIELD_AND_SIZE(_fEnableBuiltinGlyphs) }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice
it works: |
This PR works with any font and you can just use CaskaydiaCove. :) |
Just so we're on the same page... You are testing a font which is under development by PhMajerus, which you found on a pull request about changing how box/line/shade characters are rendered for the purposes of testing, and asking for PhMajerus to provide you a version of the under-development font that has also been patched to contain "Nerd Font" characters. Lhecker is telling you that the changes in this PR apply to any font; to be clear though: this PR will not make glyphs that exist only in PhMajerus' under-development font show up in all fonts. If you're looking for a font that contains sextants, octants, diagonals and large type pieces as well as Nerd Font glyphs, you will need to wait for Cascadia to officially release an update for those legacy computing symbols. |
Yep, that's what i meant with i wish there was a merge(remix?) between Cascadia Mono by PhMajerus and CaskaydiaCove Mono by NerdFonts. |
Not quite yet, but maybe soon 👀 |
@SHJordan As DHowett said, I'm just adding legacy computing characters, hoping for Cascadia to officially release an update with those included. But to clarify, I don't even intend to release a separate font unless DHowett or Aaron tell me to, I'm only submitting pull requests gradually for sets of legacy computing characters to be included in the Microsoft version: microsoft/cascadia-code#708. This is the reason the font attached in this thread contains much more characters than my public https://github.com/PhMajerus/cascadia-code/ fork currently does. It is my understanding that other fonts based off Cascadia can then later integrate those changes as well, so what you're hoping for would probably happen the other way around, with CaskaydiaCove including those through Cascadia, instead of me integrating CaskaydiaCove changes. I hope DHowett's "maybe soon 👀" mean they're considering accepting my pull requests, so we won't need different fonts to get those characters anymore. I feel like we're hijacking a thread with another topic here, sorry about that. So DHowett and lhecker, feel free to mark it all as off-topic. |
@PhMajerus understandable, and sorry for being a bit offtopic guys. |
This adds support for drawing our own box drawing, block element,
and basic Powerline (U+E0Bx) glyphs in AtlasEngine.
This PR consists of 4 parts:
_drawGlyph
because I'vewanted to do that for ~1 year now and never got a chance.
Well, now I'm doing it and you all will review it muahahaha.
The good news is that it removes a goto usage that even for my
standards was rather dangerous. Now it's gone and the risk with it.
we want to handle different from regular text. Previously, we only
did that for soft fonts, but now we want to do that for a lot more,
so a refactor was in order. The new code is still extremely
disgusting, because I now stuff
wchar_t
s into an array that'sintended for glyph indices, but that's the best way to make it fast
and not blow up the complexity of the code even further.
The LUT has 4 "drawing instruction" entries per glyph which describe
the shape (rectangle, circle, lines, etc.) and the start/end coord.
With a lot of bit packing each entry is only 4 bytes large.
builtinGlyphs
setting was added to the fontobject and it defaults to
true
.Closes #5897
Validation Steps Performed
customGlyphs
setting can be toggled on and off ✅