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

[android] POC: implementation of LocalGlyphRasterizer #10608

Closed
wants to merge 17 commits into from

Conversation

ChrisLoer
Copy link
Contributor

@ChrisLoer ChrisLoer commented Nov 30, 2017

This is an initial implementation of local glyph CJK glyph generation for Android. The namespaces/class names are weird because this is my first time using the JNI and I was figuring everything out through aggressive copy-pasting. 😅

Basic rendering seems to work, although the glyph metrics could use some tweaking.

TODOs:

  • Implement configurable font family (right now it's hard-wired to "Noto Sans")
  • Implement FontStack-based font-weight heuristics?
  • Automated testing?
  • Expose font family configuration in SDK interface

My biggest concern is that the glyphs do not look as good as they do on the iOS/macOS side. Since we're pretty sure that's not TinySDF's fault, I'll try looking into whether it's something in the "Noto Sans" that's getting loaded, or maybe there's some kind of lossy down-sampling in drawing to a 35x35px bitmap?

Android using "Noto Sans":
screenshot 2017-11-30 12 43 44

vs. mbgl-glfw using "Noto Sans":
screenshot 2017-11-30 12 55 20

/cc @zugaldia @tobrun @ivovandongen @chriswu42 @lilykaiser

… drawn area, better align to baseline.

Includes code to extract glyph metrics, but the results don't match anything I'd expect so I'm not using them.
…y" string, plumb it out as far as the Renderer constructor.
Add pragma once
Move instantiations of CFHandle down to where they're used (although a few are duplicated).
In my tests so far, I have yet to get CoreText to render anything differently based on the font weight changes.
@ChrisLoer
Copy link
Contributor Author

That was an easy fix -- the glyphs were ugly because I hadn't turned on anti-aliasing for the Paint class. It looks pretty good to me now:

screenshot 2017-11-30 13 38 35

@kkaefer
Copy link
Member

kkaefer commented Dec 1, 2017

🔥

@tobrun tobrun added the Android Mapbox Maps SDK for Android label Dec 1, 2017
@tobrun tobrun added this to the android-v6.0.0 milestone Dec 1, 2017
@zugaldia
Copy link
Member

zugaldia commented Dec 1, 2017

@ChrisLoer this is looking amazing, thank you for running with it.

The namespaces/class names are weird because this is my first time using the JNI and I was figuring everything out through aggressive copy-pasting.

Per chat with @ivovandongen, he can give the current approach a pass next week (if he still has the bandwidth).

cc: @LukasPaczos fyi.

Add support for "bold" font stacks
Somewhat rationalize class names, add comments.
@ChrisLoer
Copy link
Contributor Author

OK, latest implementation has support for "Bold" (as far as I know, Typeface doesn't expose a general font-weight interface), and the font family can be configured. We still have to decide how to hook it up externally. On the iOS side, @akitchen proposed a minimal initial approach in which the configuration option is specified as part of the app bundle (see #10612) -- there's probably some close Android analog of that change?

@zugaldia
Copy link
Member

zugaldia commented Dec 1, 2017

there's probably some close Android analog of that change?

I believe that the direction translation would be adding a new key to the Android Manifest file, but I think that in this case it'd be better to introduce instead an XML attribute that could look something like:

mapbox:mapbox_fontWeight="bold"

This'd also be exposed through the MapboxMapOptions object so that the same result could be achieved programmatically. The advantage of this approach is that this is a per-view approach, which addresses the suggestion from @1ec5 on #10612 (review).

@ChrisLoer
Copy link
Contributor Author

ChrisLoer commented Dec 1, 2017

@zugaldia Sorry, I think I introduced some confusion talking about two separate issues in that comment. We derive the font weight from the font stack that's being used, so it doesn't need to be part of the manifest (although this is a hack, and a per layer configuration might be better).

The second part of my comment was about choosing the font family to use locally (i.e. the main entry point for turning this feature on).

I squashed/re-wrote #10522 and as part of that cherry picked these changes onto the new base -- we can continue discussion on that PR (and try to keep Android and iOS in sync).

@ChrisLoer ChrisLoer closed this Dec 1, 2017
@jfirebaugh jfirebaugh deleted the tinysdf-android branch July 27, 2018 22:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Android Mapbox Maps SDK for Android
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants