Integrate Harffbuzz freetype to render Khmer Burmese and hindi

[alanchenboy]

refactor for #1289

This is a demo of how to use harfbuzz to render complex text.

Example style:
https://alanchenboy.github.io/harfbuzzResource/hindi.json.

Screenshots:
.

https://github.com/maplibre/maplibre-native/assets/5135389/f8f294ec-1d20-4106-b745-51d5f4a1af1d.

https://github.com/maplibre/maplibre-native/assets/5135389/7e3857e9-74c4-405e-b12c-e5722cf76919.

[codecov]

Codecov Report

Attention: Patch coverage is 93.22382% with 33 lines in your changes are missing coverage. Please review.

Project coverage is 85.78%. Comparing base (a1bb983) to head (6b02cba).

❗ Current head 6b02cba differs from pull request most recent head ca19d05. Consider uploading reports for the commit ca19d05 to get more accurate results

Files	Patch %	Lines
src/mbgl/text/shaping.cpp	85.56%	14 Missing ⚠️
src/mbgl/text/harfbuzz.cpp	78.94%	8 Missing ⚠️
src/mbgl/text/glyph_manager.cpp	92.59%	4 Missing ⚠️
src/mbgl/text/glyph.cpp	91.66%	2 Missing ⚠️
src/mbgl/layout/layout.hpp	50.00%	1 Missing ⚠️
src/mbgl/layout/symbol_layout.cpp	98.90%	1 Missing ⚠️
src/mbgl/storage/resource.cpp	87.50%	1 Missing ⚠️
src/mbgl/tile/geometry_tile_worker.cpp	96.77%	1 Missing ⚠️
src/mbgl/util/i18n.cpp	91.66%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #1439       +/-   ##
===========================================
+ Coverage   59.94%   85.78%   +25.84%     
===========================================
  Files         580      566       -14     
  Lines       28529    28188      -341     
  Branches    11093        0    -11093     
===========================================
+ Hits        17102    24182     +7080     
+ Misses       4137     4006      -131     
+ Partials     7290        0     -7290     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[alanchenboy]

Swipe out macos end is such a pain for me, I have to use my personal computer to fix unit tests

[ntadej]

I wonder if we want to make this an optional feature? This would allow us to fix Qt later for example.

Also what is the binary size increase? (@louwers, didn't we have a CI test for that in the past? I guess the rest of the actions need to pass.)

[alanchenboy]

https://github.com/maplibre/maplibre-native/actions/runs/5747452827/job/15578480347 does anybody known what is the reason for this failure?

[alanchenboy]

I didn't have linux end, will fix linux CI in next weeks.

[louwers]

@alanchenboy That means the iOS render tests are failing. Did you run them locally?

[ramSeraph]

Hi @alanchenboy thank you for working on this.

I want to get this merged, but due to the +26% binary size increase I think it makes sense to make using Harfbuzz opt-in (or at least offer an easy opt-out). The easy way to make it opt-in would be to sprinkle #ifdefs throughout all the touched files, but this would not make the code very readable. Especially since more features that will be opt-in are expected in the future.

So to merge this we need to find a good pattern to make functionality opt-in. Perhaps it is possible to make a few derived classes where only at instantiation the right class is instantiated (with an #ifdef).

Hey @louwers What is the acceptable limit for increase in binary size? Would pulling in data during runtime be an acceptable solution if it helps reduce binary size?

[louwers]

@ramSeraph We don't really have a policy for it at the moment, but if we don't make a feature that has a large binary size increase opt-in or opt-out then we risk losing contributions from large corporations for whom binary size is important. They might decide to maintain an internal fork.

Each binary size increate should always be looked at in the context of the value the contribution brings.

Personally I think we should release the 'kitchen sink' with all features enabled, users that care about binary size need to make their own builds. Although we can also consider making minimal builds in adition to full builds.

Would pulling in data during runtime be an acceptable solution if it helps reduce binary size?

No that is not acceptable, at least not for iOS because Apple forbids it.

[ramSeraph]

@ramSeraph We don't really have a policy for it at the moment, but if we don't make a feature that has a large binary size increase opt-in or opt-out then we risk losing contributions from large corporations for whom binary size is important. They might decide to maintain an internal fork.

Each binary size increate should always be looked at in the context of the value the contribution brings.

The problem with this argument is that you are indeed encouraging the people who need this feature to fork maplibre( as Grab has probably already done ).

Anyway, I do understand where you are coming from.

Personally I think we should release the 'kitchen sink' with all features enabled, users that care about binary size need to make their own builds. Although we can also consider making minimal builds in addition to full builds.

I am thinking a modular architecture with feature flags would be more appropriate, but in the end that will cause a test matrix overload.

No that is not acceptable, at least not for iOS because Apple forbids it.

This is surprising.. I do mean pulling in data, not code. I was asking this because a lot of harfbuzz binary size might be coming from unicode data it loads, which can be pulled in lazily.

[ramSeraph]

The problem with this argument is that you are indeed encouraging the people who need this feature to fork maplibre( as Grab has probably already done ).

Actually ignore this.. I can see how the build flag would be good enough, without the need for a fork.

I am thinking a modular architecture with feature flags would be more appropriate, but in the end that will cause a test matrix overload.

Doesn't maplibre-native already have a plugin architecture? Can this particular feature be moved to a plugin?

[alanchenboy]

Pipelines fixed， lucky me. let me add some complex text render tests to finish this PR.

[louwers]

@ramSeraph

This is surprising.. I do mean pulling in data, not code. I was asking this because a lot of harfbuzz binary size might be coming from unicode data it loads, which can be pulled in lazily.

That is an interesting idea.

Doesn't maplibre-native already have a plugin architecture? Can this particular feature be moved to a plugin?

Not for the C++ Core at the moment.

@alanchenboy Congrats! 💯 Can you merge main in?

[wipfli]

If this is ready for review then I would propose to have a discussion in the style spec repo about the changes needed to the style spec.

[robinhood245]

@alanchenboy Thank you so much for the effort you are putting in. I was hoping to try it out, but whenever I am trying to build from the code for Android, it's failing. Can you please help me build from the code?

[alanchenboy]

@alanchenboy Thank you so much for the effort you are putting in. I was hoping to try it out, but whenever I am trying to build from the code for Android, it's failing. Can you please help me build from the code?

Hi @robinhood245 the pipeline is passed so I think the CMake change is correct, but the new vendor requires higher CMake version, so if you installed cmake 3.18 in your Android studio, it will use that and cause the build failure. I update cmakeVersion in dependencies.gradle to from 3.18.1+ to 3.19.1+. And the app can build by android studio.
So maybe we should also update this file, or we build our own cmake file to build freetype and harfbuzz, cause most vendors are using Maplibe cmake instead of vendors' cmake file.

I roll back cmake upgrade, and I can compile the branch in my Android studio (it failed last time because of VPN setting)

[alanchenboy]

https://app.codecov.io/gh/maplibre/maplibre-native/commit/2f76cee32dff34d10f43f8abc4985f77a9578680 How can I fix this pipeline, is it a requirement for approve?

[louwers]

It means there are some code paths that are not reached during testing.

It's not a hard requirement at the moment.