Allow users to set font features and font axes #10525

PankajBhojwani · 2021-06-28T21:47:06Z

Adds support for users to be able to set font features and axes (see the spec for more details!)

Detailed Description

CustomTextLayout

Asks the DxFontRenderData for the font features when getting glyphs
If any features have been set/updated, we always skip the "isTextSimple" shortcut
Asks the _formatInUse for any font axes when mapping characters in _AnalyzeFontFallback

DxFontRenderData

Stores a map of font features (initialized to the standard feature list)
Stores a map of font axes
Has methods to add font features/axes to the map or update existing ones
Has methods to retrieve the font features/axes
Sets the font axes in the IDWriteTextFormat when creating it

Validation Steps Performed

It works!

Specified in #10457
Related to #1790
Closes #759
Closes #5828

src/cascadia/TerminalSettingsModel/TerminalSettings.cpp

skyline75489 · 2021-06-29T01:31:01Z

src/renderer/dx/DxFontRenderData.h

@@ -74,10 +77,15 @@ namespace Microsoft::Console::Render

        [[nodiscard]] static HRESULT STDMETHODCALLTYPE s_CalculateBoxEffect(IDWriteTextFormat* format, size_t widthPixels, IDWriteFontFace1* face, float fontScale, IBoxDrawingEffect** effect) noexcept;

+        bool DidUserSetFeatures() const noexcept;
+        void SetFeatures(std::unordered_map<std::wstring_view, uint32_t> features);


Can we just use UpdateFont as the single point of entry of font settings? In my understanding the features are basically like weight & style & etc, right? I'm OK with DidUserSetFeatures, because it's read-only. But for anything that can change the internal properties in DxFontRenderData, I'd like to do it through UpdateFont.

So the idea is roughly: move _features into FontInfoDesired &. Update _desiredFont->features in ControlCore. Pass the features inside _desiredFont through UpdateFont. Then continue.

This would be a less invasive way. BTW this also eliminates DxEngine::UpdateFontFeatures.

Is features something we need in both FontInfoBase & DxFontInfo? If it is, then it calls for the merging of these classes.

CC @miniksa for suggestions.

We cannot move _features into FontInfoDesired or FontInfo because that part of the code is shared with conhost. The features need to be specific to only the DxEngine (similar to the way we do pixel shader path)

No i mean technically all you need is a std::unordered_map, right? No DX structure related whatsoever. That is the low level representation. And you can convert that into DX structure in upper layer.

Unless there’s other regulations, then I am OK

Sorry it took a while to get back to you on this!

Even though its a low level representation, conhost will never use the map in font info, so it is very wasteful to always ask conhost to allocate space for an empty map especially considering that the majority use case is conhost

If it’s not used in conhost, I assume the compiler will maybe try to optimize them away, right? Even without optimization, I would personally trade the few bytes for architectural benefits without doubt.

I am gonna leave the choice to you guys, especially @miniksa since he created all the DX classes.

Nah. The compiler cannot optimize out members from a class. Memory layouts aren't malleable like that!

We could do it as a std::optional<std::unordered_map<std::wstring_view, uint32_t>> right? I don't think that will alloc unless it is actually used?

As before this parameter should be taken by const-reference, as you don't actually store it anywhere.

As discussed, we use UpdateFont now for the features and axes. _SetFeatures and _SetAxes are now just private helper functions inside DxFontRenderData

src/renderer/dx/CustomTextLayout.cpp

skyline75489 · 2021-06-29T22:25:52Z

src/renderer/dx/CustomTextLayout.cpp

@@ -351,7 +351,7 @@ CATCH_RETURN()
            _glyphIndices.resize(totalGlyphsArrayCount);
        }

-        if (_isEntireTextSimple)
+        if (_isEntireTextSimple && !_fontRenderData->DidUserSetFeatures())


I am a bi confused by this. Say without this PR, we have ligature enabled by default, and we think some text can be simple. But with this PR and people disabling ligature, suddenly all text is complicated now? If anything, isn’t text w/o ligature a “simple” one instead?

I mean code-wise, I get it. You have to pass the Feature structure in. But it seems wrong logically.

The thing is we cannot detect what glyphs get affected/don't get affected based on how the user changed the default features (there's just way too many features to track). So as long as the user changed something, we cannot take the shortcut and need to call GetGlyphs instead

src/cascadia/TerminalSettingsModel/TerminalSettings.cpp

src/renderer/dx/CustomTextLayout.cpp

src/renderer/dx/CustomTextLayout.h

…/pabhoj/font_feature

src/cascadia/TerminalSettingsModel/FontConfig.cpp

src/cascadia/TerminalSettingsModel/FontConfig.h

src/renderer/dx/DxFontRenderData.cpp

src/renderer/dx/DxRenderer.cpp

src/renderer/dx/DxFontRenderData.cpp

src/cascadia/TerminalControl/ControlCore.cpp

src/renderer/dx/DxFontRenderData.cpp

src/renderer/dx/DxFontRenderData.h

zadjii-msft

Okay, i'm clearing off my block, since it was pretty trivially fixable. I think everyone else on this thread's got a handle for what's up, I'll leave it to them to ✔️

src/cascadia/TerminalSettingsModel/FontConfig.idl

I literally just said why

DHowett

I'm cool with this, and pretty excited to boot. Let's get a second signoff and gogogo!

DHowett · 2021-07-21T20:28:01Z

You can mark this as closing #5828 as well!

DHowett · 2021-07-21T20:29:14Z

I edited your link to the standard feature list to be a commit-based permalink so that if fdwr ever changes that code it still points to the same ole place

miniksa

You made it! Thank you for grinding this out. I know it's been a journey. Looks good; let's ship it!

ghost · 2021-07-22T23:14:54Z

Hello @PankajBhojwani!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

ghost · 2021-08-31T17:14:51Z

🎉Windows Terminal Preview v1.11.2421.0 has been released which incorporates this pull request.:tada:

Handy links:

PankajBhojwani added 5 commits June 23, 2021 13:11

hm

d0c37fd

hmm

e3bba2b

did user set features

575a2d6

remove this

7a40553

spell

ae9e1b5

PankajBhojwani commented Jun 28, 2021

View reviewed changes

src/cascadia/TerminalSettingsModel/TerminalSettings.cpp Outdated Show resolved Hide resolved

format

afd89ed

skyline75489 reviewed Jun 29, 2021

View reviewed changes

src/renderer/dx/CustomTextLayout.cpp Show resolved Hide resolved

don't involve dxfontinfo

acc03bb

skyline75489 reviewed Jun 29, 2021

View reviewed changes

PankajBhojwani added 2 commits June 29, 2021 16:28

pipe axes through

797bae5

works, needs cleaning up

a23254c

PankajBhojwani changed the title ~~Allow users to set font features~~ Allow users to set font features and font axes Jun 30, 2021

PankajBhojwani commented Jun 30, 2021

View reviewed changes

src/cascadia/TerminalSettingsModel/TerminalSettings.cpp Outdated Show resolved Hide resolved

PankajBhojwani commented Jun 30, 2021

View reviewed changes

src/renderer/dx/CustomTextLayout.cpp Outdated Show resolved Hide resolved

PankajBhojwani commented Jun 30, 2021

View reviewed changes

src/renderer/dx/CustomTextLayout.h Outdated Show resolved Hide resolved

PankajBhojwani added 9 commits June 30, 2021 14:08

fix some failings

64e42bf

noexcept

5f23633

comments

0a9b1eb

Merge branch 'main' of https://github.com/microsoft/terminal into dev…

bd469bd

…/pabhoj/font_feature

start json parsing

8ec55ad

json parsing

9d08385

mark todos, consistent naming

87fca5c

fix errors?

b14360c

more errors

5c1be4f

PankajBhojwani commented Jul 2, 2021

View reviewed changes

src/cascadia/TerminalSettingsModel/FontConfig.cpp Outdated Show resolved Hide resolved

DHowett mentioned this pull request Jul 4, 2021

Make it possible to choose between a font's stylistic sets #10557

Closed

WSLUser mentioned this pull request Jul 6, 2021

A proper Unicode Font setting #10510

Closed

json utils works, todo complete missing axes

1bb1d10

DHowett requested changes Jul 20, 2021

View reviewed changes

ghost 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 Jul 20, 2021

fixes

753c00c

DHowett reviewed Jul 20, 2021

View reviewed changes

src/renderer/dx/DxFontRenderData.cpp Outdated Show resolved Hide resolved

DHowett reviewed Jul 20, 2021

View reviewed changes

src/renderer/dx/DxFontRenderData.cpp Outdated Show resolved Hide resolved

miniksa reviewed Jul 20, 2021

View reviewed changes

src/cascadia/TerminalControl/ControlCore.cpp Show resolved Hide resolved

DHowett reviewed Jul 20, 2021

View reviewed changes

src/renderer/dx/DxFontRenderData.cpp Outdated Show resolved Hide resolved

rename, real function

b2db07e

skyline75489 reviewed Jul 21, 2021

View reviewed changes

src/renderer/dx/DxFontRenderData.h Outdated Show resolved Hide resolved

zadjii-msft reviewed Jul 21, 2021

View reviewed changes

src/cascadia/TerminalSettingsModel/FontConfig.idl Show resolved Hide resolved

PankajBhojwani added 2 commits July 21, 2021 11:14

remove opsz generation

319e30a

private members

04d6f00

DHowett approved these changes Jul 21, 2021

View reviewed changes

ghost added Area-Settings Issues related to settings and customizability, for console or terminal Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) labels Jul 21, 2021

miniksa reviewed Jul 22, 2021

View reviewed changes

miniksa approved these changes Jul 22, 2021

View reviewed changes

PankajBhojwani added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jul 22, 2021

ghost merged commit 4c16cb2 into main Jul 22, 2021

ghost deleted the dev/pabhoj/font_feature branch July 22, 2021 23:15

cinnamon-msft mentioned this pull request Aug 26, 2021

Font features and axes missing from JSON schema #11058

Closed

This was referenced Aug 31, 2021

Request: Support for Font Width Variants #5828

Closed

Investigate/implement renderer setting to disable ligatures #759

Closed

KalleOlaviNiemitalo mentioned this pull request Nov 4, 2021

How to disable character rendering like from '>=' to '≥', from '!=' to '≠' #11683

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow users to set font features and font axes #10525

Allow users to set font features and font axes #10525

PankajBhojwani commented Jun 28, 2021 •

edited by DHowett

Loading

skyline75489 Jun 29, 2021 •

edited

Loading

skyline75489 Jun 29, 2021

PankajBhojwani Jun 29, 2021

skyline75489 Jun 29, 2021

PankajBhojwani Jul 9, 2021

skyline75489 Jul 9, 2021

DHowett Jul 9, 2021

miniksa Jul 13, 2021

lhecker Jul 14, 2021

PankajBhojwani Jul 16, 2021

skyline75489 Jun 29, 2021

PankajBhojwani Jun 30, 2021 •

edited

Loading

zadjii-msft left a comment

DHowett left a comment

DHowett commented Jul 21, 2021

DHowett commented Jul 21, 2021

miniksa left a comment

ghost commented Jul 22, 2021

ghost commented Aug 31, 2021

Allow users to set font features and font axes #10525

Allow users to set font features and font axes #10525

Conversation

PankajBhojwani commented Jun 28, 2021 • edited by DHowett Loading

Detailed Description

Validation Steps Performed

skyline75489 Jun 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PankajBhojwani Jun 30, 2021 • edited Loading

Choose a reason for hiding this comment

zadjii-msft left a comment

Choose a reason for hiding this comment

DHowett left a comment

Choose a reason for hiding this comment

DHowett commented Jul 21, 2021

DHowett commented Jul 21, 2021

miniksa left a comment

Choose a reason for hiding this comment

ghost commented Jul 22, 2021

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

ghost commented Aug 31, 2021

PankajBhojwani commented Jun 28, 2021 •

edited by DHowett

Loading

skyline75489 Jun 29, 2021 •

edited

Loading

PankajBhojwani Jun 30, 2021 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.