-
Notifications
You must be signed in to change notification settings - Fork 318
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
Create text shaper sample project #568
Create text shaper sample project #568
Conversation
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.
Thanks a lot for this, this looks great! I only have some small comments here, overall it looks very good.
I haven't delved deeply into the font engine code. I'll leave the ownership of this code, so to say, to you for now, and rather fix things up as we go. It won't impact any current users, so it should be safe to merge.
When we eventually merge the cmake
branch, then we could consider to move this into Core, and add it as another font engine selection option. Or perhaps we instead integrate it into our current font engine, but I'm a bit worried about adding another dependency for all users.
CMakeLists.txt
Outdated
@@ -905,6 +905,23 @@ if(BUILD_SAMPLES) | |||
BUNDLE DESTINATION ${SAMPLES_DIR}) | |||
endforeach() | |||
|
|||
# Build and install textshaper sample |
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 think we should nest this under a new ENABLE_HARFBUZZ
option, like we do with the plugins.
CMakeLists.txt
Outdated
@@ -905,6 +905,23 @@ if(BUILD_SAMPLES) | |||
BUNDLE DESTINATION ${SAMPLES_DIR}) | |||
endforeach() | |||
|
|||
# Build and install textshaper sample | |||
find_package(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.
It would be nice if we made this compatible with config mode, including the vcpkg package.
To achieve that:
- The find module should return the imported target
harfbuzz::harfbuzz
. - First, run
find_package(harfbuzz CONFIG)
and test withif(TARGET harfbuzz::harfbuzz)
. - If not found, run it with the find module like now, maybe rename it to lowercase for consistency.
- Then simply add
target_link_libraries(textshaper PRIVATE harfbuzz::harfbuzz)
If all of this is a bit much to deal with, let me know and I'll help out.
CMakeLists.txt
Outdated
# Build and install textshaper sample | ||
find_package(HarfBuzz) | ||
if(${HARFBUZZ_FOUND}) | ||
bl_sample(textshaper basic/textshaper ${sample_LIBRARIES} ) |
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 wonder if the sample name textshaper
is not great for discoverability. Maybe we could simply name it harfbuzz?
{ | ||
if (value == "set-english") | ||
{ | ||
lorem_ipsum_element->SetAttribute("lang", "en"); |
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 believe it would be cleaner to add these language samples and attributes to the RML source. Then we can simply add and remove a display: none
class as appropriate to show the one we want.
That all sounds good to me! I'll have a go at applying the CMake suggestions. I'll let you know if I need assistance therewith. A while back, you mentioned here about the idea of having two font engines: one based on stb_truetype and the other based on FreeType and HarfBuzz. The user could then choose which font engine to use when building the project with CMake. This would be the best way to go forward about this, since stb_truetype is very lightweight and upgrading the current font engine to be compatible with HarfBuzz wouldn't require that much more work (although this font engine overhaul as a whole would be a huge undertaking). |
Yeah, having two font engines like that is definitely something to consider, certainly very tempting. I appreciate your input here. |
Alright, I've applied the suggestions from your code review. I've renamed the sample to I think I've done it correctly, but CMake isn't my forte, so let me know if there are any alterations needed. |
- Rename 'harfbuzz' sample to 'harfbuzzshaping' due to conflict with target harfbuzz - Create imported target for harfbuzz, and use that for linking - Smaller fixups and error messages
Hey, thanks, I like the changes here. I tested them a bit locally, and found that it had some issues. In particular, it didn't like that the sample was named the same as the target. So I changed the sample name again, not sure if I like it too much, let me know if you have better suggestions. I also made some various changes, like creating an imported target. Could you test how this works out on your local setup? And generally, let me know if you have any feedback on these changes. One issue here is that we rely on internal headers and functions from the default font engine. This is understandable, but not entirely ideal. This won't really work without the default font engine interface compiled. And it won't really work when symbols need to be manually exported (like with dynamic linking on Windows). I added some warnings for these cases. I think it's okay for now, but I hope we're able to improve it in the future. When merging the cmake branch, we could either make this into a separate font engine selection, or we could of course merge this shaping code with the default font engine. |
I've just pulled your changes locally, and I can confirm that they work perfectly. Thanks for the help with that! The rest of the changes look good to me. Regarding the name of the sample, I think I had another look at what internal files this sample requires, and it seems it only needs Alternatively, if you feel that the issues that you mentioned are too glaring, we could forego merging this sample and instead refer back to this pull request when we decide to implement text shaping in the core library. When I have some extended free time, I might try fully implementing text shaping in the default font engine. I'll try to make it so that text shaping can be turned off with a CMake option/preprocessor symbol, since HarfBuzz is a sizeable binary and some users might not want or need it (especially if they only wish to do simple text rendering). |
Good to hear the changes work for you. And thanks a lot for the pull request! I think this is good as it is for merging now, we'll consider it a stepping stone until we move things into the font engine. I think it could be useful to still have such a sample to show off text shaping, even after we've moved all the code into Core.
Very cool, that sounds great! I think instead of a preprocessor symbol, we should consider structuring it as a separate font engine, and pull out everything that is common between freetype/harfbuzz engines so that they are mostly shared. Of course, it could turn out that this is too cumbersome compared to preprocessor logic, but at least something to consider. |
No problem! A separate font engine sounds good too; this sample has shown that there isn't much to change in order to make the default font engine support text shaping—so it shouldn't be too difficult. |
) Co-authored-by: Michael Ragazzon <[email protected]>
This pull request is a follow-up from #563.
It adds a new sample project that uses a custom font engine with HarfBuzz to render properly shaped text. It displays a block of lorem ipsum text and has three buttons (English, Arabic, and Hindi) that change the language of the text (to demonstrate the text shaping in each language).
Most of the font engine code in this PR was copied from the existing default FreeType engine but modified to support HarfBuzz text shaping. Some of these files are almost identical, but due to the fact that some functions I needed were private and/or internally linked, I had to copy the file entirely to the sample project in order to use the functions or modify a single parameter.
I used the default font engine as a template because I wanted this sample project to determine how feasible it would be to upgrade it to support text shaping.
Here is a summarised list of what was needed to support text shaping:
hb_font
member, which is initialised from the existing FreeType face.auto
). If the language of the text isn't registered, the font-face handle will attempt to determine the best-fitting script and direction (if set toauto
, otherwise it will use whatever thedir
attribute was).0
). One possible way to implement this could be to create another lookup table inside the font-face handle that maps unsupported characters to rendered fallback glyphs, and then use those glyphs when necessary.Screenshots:
Font rendering without text shaping.
Font rendering with text shaping.
(The screenshots in #563 contained a minor kerning error, which has since been fixed).