Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

[darwin, android, core] Expose "local ideograph font family" in map snapshotters #13427

Merged
merged 4 commits into from
Nov 24, 2018

Conversation

ChrisLoer
Copy link
Contributor

The core part of this PR exposes a "local ideograph font family" option on MapSnapshotter and passes it through to the underlying renderer.

The darwin implementation picks up the localFontFamilyName that's already read out of the application bundle by MGLRendererConfiguration. I tested this manually in the macosapp, using a breakpoint to ensure that local fonts were being used during snapshot generation.

The Android implementation is more of a guess: I added localIdeographFontFamily as an option in MapSnapshotter::Options and hooked it up to the underlying core snapshotter. This seemed closest to the way the rest of the configuration works, but it is unfortunate that it requires the user to pay attention and set the value every time they create a MapSnapshotter. This might be less of an issue if we change the default value to be enabled. @LukasPaczos or @tobrun I could use input on what you think is the right way to do this. Also I haven't tested at all yet because my Android debug environment is messed up.

This continues to be an area with poor test coverage. 😞 The problem is that the behavior is inherently inconsistent between machines.

cc @lloydsheng @chriswu42 @LukasPaczos @julianrex

@ChrisLoer
Copy link
Contributor Author

Oh, I almost forgot there's one other change in this PR for macos/iOS -- we also pass the programCacheDir to the MapSnapshotter from the global options. If set, this path is used to cache the compiled binary version of the shader programs, to avoid compilation time after the first time the map runs. This definitely seems like something we want enabled for the MapSnapshotter (and we already use it on the Android side). cc @kkaefer

@kkaefer
Copy link
Member

kkaefer commented Nov 21, 2018

We’re not using program caching on iOS because the platform is pretty good at caching the compiled programs internally already. This means subsequent shader compiles will reuse the already compiled version even if we use the regular OpenGL APIs to compile shaders from source.

@ChrisLoer
Copy link
Contributor Author

We’re not using program caching on iOS because the platform is pretty good at caching the compiled programs internally already.

Ah, I had forgotten that, thanks. Still probably makes sense to pass through the setting if it's there, just for consistency with MGLMapView?

Copy link
Contributor

@julianrex julianrex left a comment

Choose a reason for hiding this comment

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

iOS/macOS piece looks good.

Copy link
Contributor

@LukasPaczos LukasPaczos left a comment

Choose a reason for hiding this comment

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

LGTM 👍

The example you modified currently crashes on startup for unrelated reasons, and after applying a fix from #13444 the value is pushed correctly to the renderer.

return this;
}


Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: new line

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants