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

Basic new line support for left to right scripts #712

Merged
merged 15 commits into from
Apr 15, 2024

Conversation

hmaarrfk
Copy link
Contributor

@hmaarrfk hmaarrfk commented Apr 7, 2024

So my team seemed to need to use newline to set text.

I gave it a go since it seemed easier than to try to always just guess the height of lines.

  • Add relevant tests that test the positions:
    • Tokenizing
    • Geometry alignment
    • Text with leading and lagging new line
    • Text with whitespace before the words
    • Text alignment with spaces before and after
  • Text alignment anchor center looks weird with the ability to have multiple lines. I would somewhat like to get the alignment correctly since I feel like it can be tricky for multi line cases.
    • Left
    • Center
    • Right
  • Text justificaiton text_justify as a new parameter. Options:
    • False
    • True
    • True but not the last line. <-- is this useful? maybe it should be "true but not the last line of my paragraph".
  • Test for different alignments of text when space is surrounding a new line
    • "hello \nworld" vs "hello\n world" vs "hello \nworld"
    • Should the above be different? Maybe in the regular font rendering, but maybe not for markdown rendering.
    • Yes they should be different.
  • Decide on how to handle
    • \r, (as before, mostly ignored)
    • \f, (who uses this for real?)
    • \t (just ignore)
outdated comment I'm not too proud of this, but it does most of the things we need.

I'm struggling to choose the best path for the implementation. I'm still trying to understand what things mean:

  1. "\n\n" means new paragraph.
  2. is "\n" a forced new line? So if text wraps.

I might have to put this on pause while we internally pursue a different approach, but leaving it here for a bit while I give it more thought. It was interesting to learn about fonts by reading your discussion.

xref: #391
Touches on the points:

  • Line breaking.

Performance stats:

  • Main: generate_glyph: 18.1 ms total 1.81 us per glyph
  • This branch: generate_glyph: 27.8 ms total 2.78 us per glyph

For future work:

@hmaarrfk hmaarrfk requested a review from Korijn as a code owner April 7, 2024 16:10
@hmaarrfk
Copy link
Contributor Author

hmaarrfk commented Apr 7, 2024

text_justify = False
image

text_justify = True
image

@hmaarrfk hmaarrfk changed the title Font new line support Basic new line support Apr 7, 2024
@hmaarrfk
Copy link
Contributor Author

hmaarrfk commented Apr 8, 2024

i made the example more interactive. Its pretty fun now. and added a few justification options that we can toggle or remove code for.

Screencast.from.2024-04-07.21-18-42.webm

pygfx/geometries/_text.py Outdated Show resolved Hide resolved
pygfx/geometries/_text.py Outdated Show resolved Hide resolved
@Korijn
Copy link
Collaborator

Korijn commented Apr 8, 2024

I think it is worthwhile to add a validation example (screenshot test) to cover the different cases you have supported.

Very cool and super useful! Awesome work.

Copy link
Collaborator

@almarklein almarklein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really nice work! Exciting to see the text rendering improving on this front 🚀

examples/feature_demo/text_justification.py Outdated Show resolved Hide resolved
pygfx/geometries/_text.py Outdated Show resolved Hide resolved
pygfx/geometries/_text.py Outdated Show resolved Hide resolved
pygfx/geometries/_text.py Show resolved Hide resolved
pygfx/geometries/_text.py Show resolved Hide resolved
pygfx/geometries/_text.py Outdated Show resolved Hide resolved
@hmaarrfk hmaarrfk force-pushed the font_new_line branch 3 times, most recently from 27bb637 to 5802c12 Compare April 10, 2024 01:00
@hmaarrfk hmaarrfk marked this pull request as draft April 10, 2024 01:00
@hmaarrfk hmaarrfk force-pushed the font_new_line branch 4 times, most recently from 6adb0ee to 1a0d89e Compare April 10, 2024 12:01
@hmaarrfk
Copy link
Contributor Author

I'm not sure how far I want to push this concept.

Text shaping is "hard" the concept of a new line in latin languages (left to right, top to bottom) is something I understand.

I cannot say that I really understand right to left languages, nor that I would be able to give meaningful inputs on top to bottom languages.

I'm also concerned about just the viability of me writing this for the first time as I am definitely not an expert in this.

To be clear, I think that a "new line" is really useful, but I just don't know if I can meaningfully setup good test case even for myself for other writing orientations.

@Korijn
Copy link
Collaborator

Korijn commented Apr 13, 2024

We can cut off the scope at this point and save the rest for the future. Let's wrap it up nicely and address any remaining code review comments.

@hmaarrfk hmaarrfk force-pushed the font_new_line branch 4 times, most recently from 405007f to 25d2968 Compare April 13, 2024 12:49
@hmaarrfk hmaarrfk marked this pull request as ready for review April 13, 2024 12:51
@hmaarrfk
Copy link
Contributor Author

I added:

  • tests for the use cases I considered
  • Test cases for questions raised during the review
  • A note about only supporting left to right text.
  • A note about performance in the top comment. On my machine 1.8us / glyph -> 2.7us / glyph on the existing benchmark

I noticed that you don't seem to care too much about the git history within the pull request.
I won't rebase my commits the next time.

@hmaarrfk hmaarrfk changed the title Basic new line support Basic new line support for left to right scripts Apr 13, 2024
@Korijn
Copy link
Collaborator

Korijn commented Apr 13, 2024

@almarklein can you review this once more in its final state? (I don't know enough about the text rendering subsystems to judge)

Copy link
Collaborator

@almarklein almarklein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really nice work. Great to see a validation example and unit tests!

@Korijn Korijn merged commit 637bef6 into pygfx:main Apr 15, 2024
12 checks passed
@almarklein
Copy link
Collaborator

🚀

@almarklein almarklein mentioned this pull request Apr 15, 2024
25 tasks
@hmaarrfk
Copy link
Contributor Author

Thank for you working on the scope and merging this!

We are super excited to try this out along in production to help our many aspects of our communication to end users!

@hmaarrfk
Copy link
Contributor Author

But most of all, thank you for helping teach me all about different aspects of text rendering!

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.

None yet

3 participants