-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
split extremely wide ligature glyphs to fit atlas #4386
base: master
Are you sure you want to change the base?
Conversation
@meganrogge @Tyriar can you please review this and let me know if you have any suggestions for webgl renderer? |
1f1e9e3
to
9de040c
Compare
long ligatures render wider than the cells
9de040c
to
a3321a6
Compare
got webgl to also work a bit. It's working only when there's something else at the end e.g. '=====================a' but the last char disappears. Needs some more work I guess, but still not fully clear on how the webgl renderer works. @Tyriar any suggestions? |
// split the image into multiple images to fit into the texture | ||
const images: ImageData[] = []; |
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.
Returning an array of images here will complicate things a lot, can we instead force glyphs to have a maximum width of _textureSize
? 512px is a lot even on a high device pixel ratio and seems like a reasonable place to stop
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.
for most glyphs, it'd be an array with a single item, we will have splits only when the image is bigger than texture size. Limiting glyphs to texture size would work mostly, but what to do with the huge ligatures, then?
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.
To do one step back - is the potentially endless stacking into a bigger and bigger glyph the expected behavior from ligatures, is that transporting the right sematics? How do other output systems (TEs or editors) deal with that?
If its indeed intended, then we have only one "natural" upper bound - the current width of the viewport. Not sure how much hassles it imports - could those big glyphs be rendered from a slow path (eg. skipping the texture atlas logic, instead doing on-the-fly composition)?
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.
Considering how unimportant a ligature for ================================================>
seems to be, I think we should just not support that case and revert to disabling ligatures when it would exceed the width of a texture atlas page. Too much complexity would be introduced (particularly dealing with the special texture in webgl) having a special path that creates a new larger special texture just for this obscure case.
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, was caught up with things, I will try to update the pr this week to limit ligature size instead.
Thanks
Fixes #4362
Have fixed it for the canvas renderer, got confused with the webgl code, can try it again if someone can give few pointers on how to go about it.