Adding primitive support for glyph clusters and n-to-m mapping. #4438
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
reli-msft
reviewed
Feb 4, 2020
| // cluster is representing and add column counts for all the characters together. | ||
| // Inside a glyph cluster, we should try our best to respect the advances and offsets to keep glyphs properly | ||
| // positioned. This means the glyph offset adjustment should be applied to all glyphs in cluster, and the | ||
| // column-based advance adjustment should be applied on the last glyph of the cluster. | ||
|
|
||
| // Find the range for current glyph run in _glyphClusters. | ||
| const auto runStartIterator = _glyphClusters.begin() + run.textStart; | ||
| const auto runEndIterator = _glyphClusters.begin() + run.textStart + run.textLength; | ||
|
|
||
| // Find the range of characters that the current glyph is representing. | ||
| const auto firstIterator = std::find(runStartIterator, runEndIterator, i - run.glyphStart); |
There was a problem hiding this comment.
These finds may impact performance.
Could we iterate through characters instead?
There was a problem hiding this comment.
I think considering that we are only searching within a single run, I personally think the impact should be pretty close to ignorable.
|
@milizhang sorry we haven't gotten to reviewing this one yet. We're working on the 0.9 release. To that end, I think I'm going to knowingly regress #696 by reverting #4081 and ask you to bring 4081 back in as part of this PR -- I'd prefer the issue we know (ligatures causing rendering issues) than the unknown unknowns. As an example, I've seen glyphs rendering outside of their cells in addition to #4375's stated issue. We're tackling rendering things in the coming month or two, so this should be driven to a fairly quick resolution either way. |
|
@milizhang, I'm currently working this code snippet in combination with #4708. I've figured out what's up and am using your code as a basis to fix the issues. |
miniksa
added a commit
that referenced
this pull request
Feb 28, 2020
10 tasks
ghost
pushed a commit
that referenced
this pull request
Mar 2, 2020
## Summary of the Pull Request - Improves the correction of the scaling and spacing that is applied to glyphs if they are too large or too small for the number of columns that the text buffer is expecting ## References - Supersedes #4438 Co-authored-by: Mili (Yi) Zhang <milizhang@gmail.com> - Related to #4704 (#4731) ## PR Checklist * [x] Closes #696 * [x] Closes #4375 * [x] Closes #4708 * [x] Closes a crash that @DHowett-MSFT complained about with `"x" * ($Host.UI.RawUI.BufferSize.Width - 1) + "`u{241b}"` * [x] Eliminates an exception getting thrown with the U+1F3E0 emoji in `_CorrectGlyphRun` * [x] Corrects several graphical issues that occurred after #4731 was merged to master (weird repeats and splits of runs) * [x] I work here. * [x] Tested manually versus given scenarios. * [x] Documentation written into comments in the code. * [x] I'm a core contributor. ## Detailed Description of the Pull Request / Additional comments - The `_CorrectGlyphRun` function now walks through and uses the `_glyphClusters` map to determine the text span and glyph span for each cluster so it can be considered as a single unit for scaling. - The total number of columns expected across the entire cluster text/glyph unit is considered for the available spacing for drawing - The total glyph advances are summed to see how much space they will take - If more space than necessary to draw, all glyphs in the cluster are offset into the center and the extra space is padded onto the advance of the last glyph in the range. - If less space than necessary to draw, the entire cluster is marked for shrinking as a single unit by providing the initial text index and length (that is back-mapped out of the glyph run) up to the parent function so it can use the `_SetCurrentRun` and `_SplitCurrentRun` existing functions (which operate on text) to split the run into pieces and only scale the one glyph cluster, not things next to it as well. - The scale factor chosen for shrinking is now based on the proportion of the advances instead of going through some font math wizardry - The parent that calls the run splitting functions now checks to not attempt to split off text after the cluster if it's already at the end. This was @DHowett-MSFT's crash. - The split run function has been corrected to fix the `glyphStart` position of the back half (it failed to `+=` instead of `=` which resulted in duplicated text, sometimes). - Surrogate pair emoji were not allocating an appropriate number of `_textClusterColumns`. The constructor has been updated such that the trailing half of surrogate pairs gets a 0 column width (as the lead is marked appropriately by the `GetColumns()` function). This was the exception thrown. - The `_glyphScaleCorrections` array stored up over the calls to `_CorrectGlyphRun` now uses a struct `ScaleCorrection` as we're up to 3 values. - The `ScaleCorrection` values are named to clearly indicate they're in relation to the original text span, not the glyph spans. - The values that are used to construct `ScaleCorrection`s within `_CorrectGlyphRun` have been double checked and corrected to not accidentally use glyph index/counts when text index/counts are what's required. ## Validation Steps Performed - Tested the utf82.txt file from one of the linked bugs. Looked specifically at Burmese through Thai to ensure restoration (for the most part) of the behavior - Ensured that U+1f3e0 emoji (🏠) continues to draw correctly - Checked Fixedsys Excelsior font to ensure it's not shrinking the line with its ligatures - Checked ligatureness of Cascadia Code font - Checked combining characters U+0300-U+0304 with a capital A
|
Closing. This was pulled into #4747. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary of the Pull Request
This changes now makes handles column width-based position correction and advance adjustment by glyph clusters instead of individual glyphs, and thus provide primitive support for n-to-m glyph-character cluster mapping, which is necessary to display some complex scripts in a readable manner.
References
This should provide a solution to issue #4375.
PR Checklist
Detailed Description of the Pull Request / Additional comments
Due to the restriction of the column grid system, this change cannot yield a perfectly layouted result, as it could add additional spacing between glyph clusters and therefore might break writing system features such as Shirorekha (Top Line) in Devanagari.
Just like #4081, I personally consider this to be a stop gap solution before we have the opportunity to design and implement a better alignment and buffer system in the future.
Validation Steps Performed
Manually tested against the test file in #4375.