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

Fix very long fragmentParts not wrapping in chat render #424

Merged
merged 17 commits into from Nov 20, 2022

Conversation

ScrubN
Copy link
Collaborator

@ScrubN ScrubN commented Nov 12, 2022

Fixes #421
Fixed a few memory leaks - i.e. after rendering about ~800 RTL messages, 3GB of memory had leaked
Fixes #229 - fixed most RTL outlines1

Footnotes

  1. RTL scripts with adapting characters (like Arabic) do not render outlines properly. This is an issue with SkiaSharp, not us. See code comment for more details

@ScrubN ScrubN marked this pull request as draft November 12, 2022 08:06
@ScrubN
Copy link
Collaborator Author

ScrubN commented Nov 13, 2022

I've sorted out all the issues and edge cases I can think of with LTR characters.

@HellbringerOnline also requested that I add wrapping on the last ? in a render line to aid in URL readability, like how Twitch wraps URLs. Though in 0.01% scenarios it's not completely faithful to Twitch.

@ScrubN
Copy link
Collaborator Author

ScrubN commented Nov 13, 2022

I wonder how I should go about adding RTL support. I believe in arabic script the shape of a character changes depending on what comes after it, meaning word wrap could alter the readability right?

@mohad12211
Copy link
Contributor

mohad12211 commented Nov 13, 2022

I hope that I can help with RTL (Arabic speaker)
yes that's right, it changes depending on the letter before it and after it.
twitch adds a Arabic Tatweel ـ character at the end of the breaking at the top line, and the beginning at the bottom line.
like this:
image
I'm not sure if that what you mean by word wrapping.
of course twitch will try to split words to their own lines before trying to split the word letters.
it's very rare tho and I wouldn't really focus on it that much.

@HellbringerOnline
Copy link

I've sorted out all the issues and edge cases I can think of with LTR characters.

@HellbringerOnline also requested that I add wrapping on the last ? in a render line to aid in URL readability, like how Twitch wraps URLs. Though in 0.01% scenarios it's not completely faithful to Twitch.

image
Also I see that links also break after the sign -

@ScrubN
Copy link
Collaborator Author

ScrubN commented Nov 13, 2022

of course twitch will try to split words to their own lines before trying to split the words themselves.

Yes it will stay that way, but in the case that a word needs to be split to multiple lines due to length it can be pretty complicated in select languages. I'm trying to work on those select languages, which are primarily RTL.

Thanks for the help with arabic

@ScrubN ScrubN changed the title Fix very long non-RTL fragmentParts not wrapping in chat render Fix very long fragmentParts not wrapping in chat render Nov 13, 2022
@ScrubN ScrubN marked this pull request as ready for review November 13, 2022 07:11
@ScrubN
Copy link
Collaborator Author

ScrubN commented Nov 13, 2022

I was able to confirm the RTL is wrapped correctly

@ScrubN
Copy link
Collaborator Author

ScrubN commented Nov 13, 2022

I was able to partially fix RTL outlines. For example Hebrew looks perfect, but Arabic's outlines are quite significantly wider than the drawn text. I tried scaling the outlines to the text width but the issue is the character kerning/fontface

@ScrubN
Copy link
Collaborator Author

ScrubN commented Nov 13, 2022

I've realized what the issue is now, and it's an issue with SkiaSharp.

SKPaint.GetTextPath() prints RTL strings in reverse order, so the easy solution was to just reverse the character order before calling it, but in the case of Arabic that changes how certain characters look, resulting in the wider outlines.

@mohad12211
Copy link
Contributor

I compiled your branch and tested a chat render.
you are right, skiasharp is not aware that the text is RTL for some reason, so you you first had to reverse the letters, but that's not enough, each letter has a different shape depending on its position in a word. skia is also not aware of that and its just displaying letters with the wrong shape.

@lay295 lay295 merged commit c651784 into lay295:master Nov 20, 2022
@ScrubN ScrubN deleted the chatrender-patch branch November 20, 2022 22:29
@ScrubN ScrubN restored the chatrender-patch branch December 20, 2022 06:57
@ScrubN ScrubN mentioned this pull request Dec 20, 2022
2 tasks
@ScrubN ScrubN deleted the chatrender-patch branch December 20, 2022 07:07
@ScrubN ScrubN restored the chatrender-patch branch December 20, 2022 07:22
@ScrubN ScrubN deleted the chatrender-patch branch December 20, 2022 09:09
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.

Word wrap problems (1.50.6) Incorrect outlines for RTL languages
4 participants