Skip to content
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

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

LabhanshAgrawal
Copy link
Contributor

@LabhanshAgrawal LabhanshAgrawal commented Jan 28, 2023

Fixes #4362

Screenshot 2023-01-29 at 01 57 47

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.

@LabhanshAgrawal LabhanshAgrawal changed the title split extremely wide ligatures to fit atlas split extremely wide ligature glyphs to fit atlas Jan 29, 2023
@LabhanshAgrawal
Copy link
Contributor Author

@meganrogge @Tyriar can you please review this and let me know if you have any suggestions for webgl renderer?

@LabhanshAgrawal
Copy link
Contributor Author

LabhanshAgrawal commented Feb 25, 2023

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?

Comment on lines +685 to +686
// split the image into multiple images to fit into the texture
const images: ImageData[] = [];
Copy link
Member

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

Copy link
Contributor Author

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?

Copy link
Member

@jerch jerch Feb 28, 2023

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)?

Copy link
Member

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.

Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Infinite loop when drawing large ligatures to char atlas
3 participants