-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[DRAFT] Re-add #9202, but with less crashes #10036
[DRAFT] Re-add #9202, but with less crashes #10036
Conversation
if (isTextSimple) | ||
{ | ||
// Only copy the indices when the run is simple. | ||
// Otherwise the glyph indices from GetTextComplexity are not meaningful. |
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.
This is from the documentation of IDWriteTextAnalyzer1::GetTextComplexity
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.
Even with the fix, the _glyphIndices
for simple runs will have wrong start index, if there's N-M runs previous to them. For example a simple line like this:
😀
There's two runs. First is complex, the second is simple.
Text | Length | Glyph Count |
---|---|---|
😀 | 2 | 1 |
(A lot of spaces | 118 | 118 |
The correct _glyphIndices
is this:
Index | Value |
---|---|
0 | 11685 |
1 | 3 |
2 | 3 |
3 | 3 |
... | ... |
But here we have:
Index | Value |
---|---|
0 | 11685 |
1 | 0 |
2 | 3 |
3 | 3 |
... | ... |
Notice there's an incorrect 0 indice here, because when we do GetTextComplexity
on the second run, we have no idea how many glyphs the previous run takes up, and assume the number is equal to the textLength (2) and fill up the _glyphIndices
from there.
while (indicesDiff > 0) | ||
{ | ||
// Left shift the indices. | ||
_glyphIndices.erase(_glyphIndices.begin() + glyphStart + actualGlyphCount); |
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.
This is me trying to fix the issue mentioned above by left-shifting the entire array to avoid vacancy in _glyphIndices
.
For now I'll revert 9502. Sorry @skyline75489 😄 |
Sorry, 9202. |
Should we repurpose this issue as "re-add #9202, but with less crashes", or should we open a new PR for that? I don't really care which ¯\_(ツ)_/¯ |
I'd like to repurpose it! |
Hey guys I've repurposed this. I realized that in order to fully solve this, we might need to rewrite the underlying run structure. Is the DxRenderer included in the TextBuffer rewrite of 2021? I think this is somewhat blocked by that. |
It's not expected that the text buffer change will cause or require any changes in the renderer :) The interfaces through which the rest of the code reads the buffer are staying unchanged |
Closed in favor of future work for #9156 |
Summary of the Pull Request
Introduced by #9202, I mistakenly used the
glyphStart
instead oftextStart
to calculate the cluster map of simple runs, causing the application to crash when it hits N-M cluster maps. This PR corrects the mistake.References
PR Checklist
Detailed Description of the Pull Request / Additional comments
Validation Steps Performed