-
-
Notifications
You must be signed in to change notification settings - Fork 21.5k
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
[WIP] BiDi and shaping support for Font::draw/Font::get_string_size #21791
Conversation
Some visual comparison of 3.1 and this PR: 🔸 References to compare renderingNoto Nastaliq Urdu test page: Random Wikipedia articles: Update: Some additional examples of opentype features (Fonts: Kleymissky, Noto Sans, Fira Code) 🔹 3.1 alpha 1 🔹 this PR |
2349a44
to
7c79abe
Compare
@@ -173,6 +173,26 @@ Comment: The FreeType Project | |||
Copyright: 1996-2017, David Turner, Robert Wilhelm, and Werner Lemberg. | |||
License: FTL | |||
|
|||
Files: ./thirdparty/harfbuzz/ |
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.
Sections are ordered alphabetically based on the "Files" path, so this should go after ./thirdparty/glad/
.
COPYRIGHT.txt
Outdated
2005, David Turner | ||
2004, 2007, 2008, 2009, 2010, Red Hat, Inc. | ||
1998-2004, David Turner and Werner Lemberg | ||
License: MIT |
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.
The text of Harfbuzz's MIT variant should be included, as it's not the same as the "classical" MIT license Godot uses, or as Debian names it the Expat license.
Since there are dozens of MIT licenses with different wordings, I'd suggest to name this one MIT-HarfBuzz
for clarity.
For the reference, I checked https://www.debian.org/doc/packaging-manuals/copyright-format/1.0/#license-specification and https://spdx.org/licenses/ but it doesn't include any identifier for HarfBuzz's MIT variant.
Debian itself documents it as "MIT" in https://metadata.ftp-master.debian.org/changelogs/main/h/harfbuzz/harfbuzz_1.8.8-2_copyright, but since we might end up adding more thirdparty code under other MIT variants, I prefer to be more explicit.
COPYRIGHT.txt
Outdated
@@ -388,6 +408,41 @@ License: BSD-3-clause | |||
|
|||
|
|||
|
|||
License: Unicode |
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.
Licenses are listed alphabetically too, so this should go just above License: Zlib
.
SConstruct
Outdated
@@ -190,6 +190,9 @@ opts.Add(BoolVariable('builtin_squish', "Use the built-in squish library", True) | |||
opts.Add(BoolVariable('builtin_thekla_atlas', "Use the built-in thekla_altas library", True)) | |||
opts.Add(BoolVariable('builtin_zlib', "Use the built-in zlib library", True)) | |||
opts.Add(BoolVariable('builtin_zstd', "Use the built-in Zstd library", True)) | |||
opts.Add(BoolVariable('builtin_hb', "Use the built-in HarfBuzz library", True)) |
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.
I'd prefer builtin_harfbuzz
.
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.
Should also be listed alphabetically.
SConstruct
Outdated
@@ -190,6 +190,9 @@ opts.Add(BoolVariable('builtin_squish', "Use the built-in squish library", True) | |||
opts.Add(BoolVariable('builtin_thekla_atlas', "Use the built-in thekla_altas library", True)) | |||
opts.Add(BoolVariable('builtin_zlib', "Use the built-in zlib library", True)) | |||
opts.Add(BoolVariable('builtin_zstd', "Use the built-in Zstd library", True)) | |||
opts.Add(BoolVariable('builtin_hb', "Use the built-in HarfBuzz library", True)) | |||
opts.Add(BoolVariable('builtin_icu', "Use the built-in ICU library", True)) | |||
opts.Add(BoolVariable('use_sil_graphite2', "Use the external SIL Graphite library (LGPL licensed)", False)) |
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.
I don't see any LGPL text in src/hb-graphite2.{cc,h}
, it has the same copyright header as HarfBuzz.
# Don't use dynamic_cast, necessary with no-rtti. | ||
env.Append(CPPDEFINES=['NO_SAFE_CAST']) | ||
# These flags help keep the file size down | ||
env.Append(CPPFLAGS=["-fno-exceptions"]) #, '-fno-rtti' - dynamic_cast required by ICU |
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.
I don't think we can/should do that. CC @eska014
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.
Yeah, turning off runtime type information saves about 200KiB last I tested, so we should avoid re-enabling it. Does ICU have some build option that avoids usage of RTTI?
thirdparty/README.md
Outdated
@@ -124,6 +124,27 @@ Files extracted from upstream source: | |||
- the include/ folder | |||
- `docs/{FTL.TXT,LICENSE.TXT}` | |||
|
|||
## ICU4C |
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.
Should be listed alphabetically too (and with two lines between sections).
Hi Bruvzg, very glad that you are trying to get this to work, I think this is a lot of progress. How do you want to continue from here? I can imagine Label and RichTextlabel will need more complex code. Also, one thing I was never too clear about is how big is ICU. I suppose in the range of <1mb it could be compiled in with the option to compile out. If larger, it could be built-in for the editor, but loaded via gdnative on exported projects (or optionally compiled in on an export template). |
7b573fc
to
20f16f1
Compare
I have added separate functions to break lines and justify for Label, and separate
Currently total size impact of HarfBuzz and ICU on macOS release export template without LTO is 1.42 MB, some parts of ICU probably could be removed. ICU break iterator data is about 6 MB for all languages, full data is 26MB, but it is optional (only used for breaking lines and justification of some languages) and can be loaded as resource when needed (Can be set in
I didn't included actual library ( Not sure how to handle this best, graphite won't be useful for most users, it's only used for rare languages and require special smart fonts. |
a68b98c
to
57d9eda
Compare
FYI: I have uploaded standalone version of the same thing at - https://github.com/bruvzg/godot_tl
|
819be30
to
efed218
Compare
…(LineEdit, Label), line breaking and line justification using ICU and HarfBuzz libraries.
@bruvzg If I need to add a Label with arabic text, should I use this PR or use the standalone version? |
Hi, anyone can update us with the current state of this pull request? it's been a while since the last exchange. @bruvzg Are you still on it? how can I help? |
Ok, I did a little research and to answer my own question here is what I found:
Now my new questions @reduz @akien-mga @bruvzg
I'm willing to help either way so I appreciate it if someone could answer the above. |
The plan is to have all these features built-in in the engine, I think it fulfills an important enough use case to be made available without hassle in the base engine. But to do so, the implementation and API need to be designed to fit closely to Godot's design principles and architecture for rendering text - or Godot's architecture for text needs to evolve based on a consensus. At the time of this PR the consensus could not be reached, but later on @reduz and @bruvzg more or less reached a consensus on how to do it. Now what's needed is contributor time, but also the ability to refactor text APIs and probably break compatibility, which means waiting for Godot 4.0's development cycle. |
Great! Thank you @akien-mga for the clarification. Will someone post the requirements/vision somewhere? |
@bruvzg Any Update On This? |
Nothing is being done here. If you need a bit more maintained version - https://github.com/bruvzg/godot_tl |
@bruvzg What is the status of this PR? Is there any desire to continue this work and add it to Godot, or should users who want this feature simply be directed to your repository? If not, abandoned pull requests will be closed in the future as announced here. Feel free to close this PR if you don't want to rebase and continue this work. |
Is this the closest thing to getting Arabic support or is there something else being worked on? I saw this as a gdscript solution, but it seems like it would make more sense to support it natively in-engine: https://github.com/3akev/godot-arabic-text Also, the gdscript version uses regular expressions, which I believe cause problems with Switch ports. (Is that still a thing?) |
@jitspoe current version is here #41100 (for 4.0 / master), there's also https://github.com/bruvzg/godot_tl (for 3.2). |
AFAIK that project was intended as a band-aid solution (it's mostly a port of some Python bidi libraries/code) until we can get something better. I believe there is an issue in the godot-feature-requests repo for proper bidi support. |
Changes
Font::draw
,Font::draw_halign
,Font::get_string_size
functions andLabel
,LineEdit
controls to support bi-directional text and complex scripts using ICU and HarfBuzz libraries.Adds
Font::draw_paragraph
function, andShapedString
/ShapedAttributedString
classes for text display and input handling.🔹 Third-party libraries
🔸 Updates
7 Sep 2018 - Fixed build for ARMv7 Linux, added glyph offsets to
get_string_size
calculation.27 Sep 2018 - Added multiline and rich text shaping APIs.
12 Oct 2018 - Added Label, and LineEdit, text shaping moved from font to separate ShapedString (for plain text), ShapedAttributedString (for rich text)
🔹 TODO list
Related:
#10546, #3081, #9961, #982