[ios] Add glyphs rasterization options #517

lloydsheng · 2020-10-22T12:24:53Z

Add MGLGlyphsRasterizationMode indicates how the map view load glyphs.

julianrex

Looks good! thank you

alexshalamov · 2020-10-22T18:25:25Z

platform/darwin/src/MGLMapSnapshotter.mm

@@ -726,9 +726,23 @@ - (void)configureWithOptions:(MGLMapSnapshotOptions *)options {

    // Create the snapshotter
    mbgl::optional<std::string> localFontFamilyName = config.localFontFamilyName ? mbgl::optional<std::string>(std::string(config.localFontFamilyName.UTF8String)) : mbgl::nullopt;
+    mbgl::GlyphsRasterizationOptions glyphsRasterizationOptions;


If MGLIdeographicFontFamilyName is provided and it is set to NO, and MGLGlyphsRasterizationMode is set to something other than MGLNoGlyphsRasterizedLocally, maybe we should print error and fallback to NoGlyphsRasterizedLocally option.

Maybe worth adding test for that combination?

Definitely.

alexshalamov · 2020-10-22T18:26:19Z

platform/darwin/src/MGLMapSnapshotter.mm

@@ -726,9 +726,23 @@ - (void)configureWithOptions:(MGLMapSnapshotOptions *)options {

    // Create the snapshotter
    mbgl::optional<std::string> localFontFamilyName = config.localFontFamilyName ? mbgl::optional<std::string>(std::string(config.localFontFamilyName.UTF8String)) : mbgl::nullopt;
+    mbgl::GlyphsRasterizationOptions glyphsRasterizationOptions;
+    glyphsRasterizationOptions.fontFamily = localFontFamilyName;


if localFontFamilyName is nullopt, we should fallback to MGLNoGlyphsRasterizedLocally

alexshalamov · 2020-10-22T18:48:39Z

platform/darwin/src/MGLRendererConfiguration.m

+
+- (MGLGlyphsRasterizationMode)glyphsRasterizationModeWithInfoDictionaryObject:(id)infoDictionaryObject {
+    if (!infoDictionaryObject || ![infoDictionaryObject isKindOfClass:[NSString class]]) {
+        return MGLNoGlyphsRasterizedLocally;


I think it should return something that represents 'unset' value, nil cc/ @julianrex?

With this PR, if user don't set MGLIdeographicFontFamilyName and MGLGlyphsRasterizationMode, glyphsRasterizationModeWithInfoDictionaryObject would return MGLNoGlyphsRasterizedLocally, thus, we will not render CJK characters locally. That will be a breaking change.

Excellent point. Rather than nil (which would make this return type a bit weird), what if we add MGLGlyphsRasterizationModeNone?

alexshalamov · 2020-10-22T18:55:29Z

platform/ios/src/MGLMapView.mm

    mbgl::optional<std::string> localFontFamilyName = config.localFontFamilyName ? mbgl::optional<std::string>(std::string(config.localFontFamilyName.UTF8String)) : mbgl::nullopt;
-    auto renderer = std::make_unique<mbgl::Renderer>(_mbglView->getRendererBackend(), config.scaleFactor, localFontFamilyName);
+    mbgl::GlyphsRasterizationOptions glyphsRasterizationOptions;
+    glyphsRasterizationOptions.fontFamily = localFontFamilyName;
+    switch (config.glyphsRasterizationMode) {
+        case MGLNoGlyphsRasterizedLocally:
+            glyphsRasterizationOptions.rasterizationMode = mbgl::GlyphsRasterizationMode::NoGlyphsRasterizedLocally;
+            break;
+        case MGLIdeographsRasterizedLocally:
+            glyphsRasterizationOptions.rasterizationMode = mbgl::GlyphsRasterizationMode::IdeographsRasterizedLocally;
+            break;
+        case MGLAllGlyphsRasterizedLocally:
+            glyphsRasterizationOptions.rasterizationMode = mbgl::GlyphsRasterizationMode::AllGlyphsRasterizedLocally;
+            break;
+    }


I think this block could be shared between snapshotter and mapview

julianrex · 2020-10-22T19:47:23Z

platform/darwin/src/MGLMapSnapshotter.mm

@@ -726,9 +726,23 @@ - (void)configureWithOptions:(MGLMapSnapshotOptions *)options {

    // Create the snapshotter
    mbgl::optional<std::string> localFontFamilyName = config.localFontFamilyName ? mbgl::optional<std::string>(std::string(config.localFontFamilyName.UTF8String)) : mbgl::nullopt;
+    mbgl::GlyphsRasterizationOptions glyphsRasterizationOptions;


Definitely.

julianrex · 2020-10-22T19:48:33Z

platform/darwin/src/MGLMapSnapshotter.mm

@@ -726,9 +726,23 @@ - (void)configureWithOptions:(MGLMapSnapshotOptions *)options {

    // Create the snapshotter
    mbgl::optional<std::string> localFontFamilyName = config.localFontFamilyName ? mbgl::optional<std::string>(std::string(config.localFontFamilyName.UTF8String)) : mbgl::nullopt;
+    mbgl::GlyphsRasterizationOptions glyphsRasterizationOptions;
+    glyphsRasterizationOptions.fontFamily = localFontFamilyName;


julianrex · 2020-10-22T19:50:58Z

platform/darwin/src/MGLRendererConfiguration.m

+
+- (MGLGlyphsRasterizationMode)glyphsRasterizationModeWithInfoDictionaryObject:(id)infoDictionaryObject {
+    if (!infoDictionaryObject || ![infoDictionaryObject isKindOfClass:[NSString class]]) {
+        return MGLNoGlyphsRasterizedLocally;


Excellent point. Rather than nil (which would make this return type a bit weird), what if we add MGLGlyphsRasterizationModeNone?

julianrex · 2020-10-22T19:53:04Z

platform/darwin/src/MGLRendererConfiguration.h

+typedef NS_ENUM(NSUInteger, MGLGlyphsRasterizationMode) {
+    /// No glyphs are rasterized locally. All glyphs are loaded from the server.
+    MGLNoGlyphsRasterizedLocally = 0,
+    /// Ideographs are rasterized locally, and they are not loaded from the server.
+    MGLIdeographsRasterizedLocally,
+    /// All glyphs are rasterized locally. No glyphs are loaded from the server.
+    MGLAllGlyphsRasterizedLocally


These enums should have the same prefix, e.g. for Swift consumption. Although it's a mouthful, how about:

MGLGlyphsRasterizationModeNone MGLGlyphsRasterizationModeIdeographsRasterizedLocally MGLGlyphsRasterizationModeNoGlyphsRasterizedLocally MGLGlyphsRasterizationModeAllGlyphsRasterizedLocally

lloydsheng · 2020-10-23T10:45:57Z

@alexshalamov @julianrex Thanks for your review 🙇 .

julianrex · 2020-10-26T15:20:52Z

platform/darwin/src/MGLRendererConfiguration.mm

+                                    @"MGLIdeographsRasterizedLocally":@(MGLGlyphsRasterizationModeNoGlyphsRasterizedLocally),
+                                    @"MGLAllGlyphsRasterizedLocally":@(MGLGlyphsRasterizationModeAllGlyphsRasterizedLocally)};
+
+    return (MGLGlyphsRasterizationMode)[nameOptionMap[infoDictionaryObject] integerValue];


For other readers: this should result in MGLGlyphsRasterizationModeNone if infoDictionaryObject is not expected.

See: https://github.com/mapbox/mapbox-gl-native-ios/pull/517/files#diff-944ba4f88de8974791fb7cf7e890a87157c51513eb6e31179983619a8312c6a6R176

zmiao · 2020-10-26T15:49:59Z

platform/darwin/src/MGLRendererConfiguration.h

+    /// No glyphs are rasterized locally. All glyphs are loaded from the server.
+    MGLGlyphsRasterizationModeIdeographsRasterizedLocally,
+    /// Ideographs are rasterized locally, and they are not loaded from the server.
+    MGLGlyphsRasterizationModeNoGlyphsRasterizedLocally,


The comments for these two enum values are incorrectly mixed.

zmiao · 2020-10-26T15:54:54Z

platform/darwin/src/MGLRendererConfiguration.mm

+    NSDictionary *nameOptionMap = @{@"MGLNoGlyphsRasterizedLocally":@(MGLGlyphsRasterizationModeIdeographsRasterizedLocally),
+                                    @"MGLIdeographsRasterizedLocally":@(MGLGlyphsRasterizationModeNoGlyphsRasterizedLocally),


Mode values are wrongly matched. I am wondering why the tests are passed 🤔

Nice catch.

zmiao · 2020-10-26T15:58:47Z

platform/darwin/src/MGLRendererConfiguration.mm

+    mbgl::GlyphsRasterizationOptions options;
+    if (fontFamilyName == nil) {
+        if (rasterizationMode != MGLGlyphsRasterizationModeNoGlyphsRasterizedLocally) {
+            MGLLogError(@"The `MGLIdeographicFontFamilyName` is set to `NO`, this will make `MGLGlyphsRasterizationMode` always be `MGLNoGlyphsRasterizedLocally`.");


I think MGLIdeographicFontFamilyName can not fully reflect the font family names anymore, as it will also be used for mbgl::GlyphsRasterizationMode::AllGlyphsRasterizedLocally

We'll ticket this out and address after beta has landed.

zmiao · 2020-10-26T16:14:57Z

platform/darwin/src/MGLRendererConfiguration.mm

+                                                                    rasterizationMode:(MGLGlyphsRasterizationMode)rasterizationMode {
+    mbgl::GlyphsRasterizationOptions options;
+    if (fontFamilyName == nil) {
+        if (rasterizationMode != MGLGlyphsRasterizationModeNoGlyphsRasterizedLocally) {


Is it possible that rasterizationMode == MGLGlyphsRasterizationModeNone?

zmiao · 2020-10-26T16:18:35Z

platform/darwin/src/MGLRendererConfiguration.mm

+            options.rasterizationMode = mbgl::GlyphsRasterizationMode::AllGlyphsRasterizedLocally;
+            break;
+        default:
+            options.rasterizationMode = mbgl::GlyphsRasterizationMode::NoGlyphsRasterizedLocally;


How about mode MGLGlyphsRasterizationModeNone?
And should we print an error log in case that the mode fallback to default?

Yes, we should always fallback to default. If fontFamilyName is not nil and user sets MGLNoGlyphsRasterizedLocally or MGLGlyphsRasterizationModeNone, I think we should warn and fallback to MGLIdeographsRasterizedLocally

We may want to revisit and test the behavior here once beta is released.

alexshalamov · 2020-10-26T18:01:40Z

platform/darwin/src/MGLRendererConfiguration.mm

+        return options;
+    }
+
+    options.fontFamily =  mbgl::optional<std::string>(std::string(fontFamilyName.UTF8String));


nit: options.fontFamily = std::string(fontFamilyName.UTF8String); should be enough

lloydsheng requested a review from 1ec5 as a code owner October 22, 2020 12:24

lloydsheng requested review from a team and julianrex October 22, 2020 12:24

julianrex requested review from alexshalamov and jmkiley and removed request for a team and 1ec5 October 22, 2020 15:43

julianrex approved these changes Oct 22, 2020

View reviewed changes

julianrex added the ⚠️ DO NOT MERGE label Oct 22, 2020

julianrex assigned lloydsheng Oct 22, 2020

alexshalamov reviewed Oct 22, 2020

View reviewed changes

julianrex suggested changes Oct 22, 2020

View reviewed changes

lloydsheng requested review from alexshalamov and julianrex October 23, 2020 10:46

julianrex approved these changes Oct 26, 2020

View reviewed changes

zmiao reviewed Oct 26, 2020

View reviewed changes

alexshalamov reviewed Oct 26, 2020

View reviewed changes

lloydsheng and others added 6 commits October 26, 2020 13:22

add glyphs rasterization options

4788fab

fix issues

aee1e57

fix issue

1094165

fix issue

90b2a17

Address some feedback

44eeea4

Switch to closed enum, remove default

cca743c

jmkiley force-pushed the lloyd-glyphs-rasterization-options branch from 2505280 to cca743c Compare October 26, 2020 20:23

julianrex removed the ⚠️ DO NOT MERGE label Oct 26, 2020

Update tests

0b82fed

jmkiley merged commit 5b239aa into main Oct 26, 2020

jmkiley deleted the lloyd-glyphs-rasterization-options branch October 26, 2020 21:08

jmkiley mentioned this pull request Oct 26, 2020

Review and Update Customizing Fonts Guide #524

Open

ChrisLoer mentioned this pull request Feb 12, 2021

CJK Glyph rasterization disabled by default in 6.3 release #589

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ios] Add glyphs rasterization options #517

[ios] Add glyphs rasterization options #517

lloydsheng commented Oct 22, 2020

julianrex left a comment

alexshalamov Oct 22, 2020

julianrex Oct 22, 2020

alexshalamov Oct 22, 2020

julianrex Oct 22, 2020

alexshalamov Oct 22, 2020

julianrex Oct 22, 2020

alexshalamov Oct 22, 2020

julianrex Oct 22, 2020

julianrex Oct 22, 2020

julianrex Oct 22, 2020

julianrex Oct 22, 2020

lloydsheng commented Oct 23, 2020 •

edited by julianrex

Loading

julianrex Oct 26, 2020

zmiao Oct 26, 2020

zmiao Oct 26, 2020

julianrex Oct 26, 2020

zmiao Oct 26, 2020

jmkiley Oct 26, 2020

zmiao Oct 26, 2020

zmiao Oct 26, 2020

alexshalamov Oct 26, 2020

jmkiley Oct 26, 2020

alexshalamov Oct 26, 2020

		NSDictionary *nameOptionMap = @{@"MGLNoGlyphsRasterizedLocally":@(MGLGlyphsRasterizationModeIdeographsRasterizedLocally),
		@"MGLIdeographsRasterizedLocally":@(MGLGlyphsRasterizationModeNoGlyphsRasterizedLocally),

[ios] Add glyphs rasterization options #517

[ios] Add glyphs rasterization options #517

Conversation

lloydsheng commented Oct 22, 2020

julianrex left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lloydsheng commented Oct 23, 2020 • edited by julianrex Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lloydsheng commented Oct 23, 2020 •

edited by julianrex

Loading