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

Optionally exclude CJK glyphs from offline downloads #13607

Merged
merged 5 commits into from
Dec 21, 2018
Merged

Conversation

ChrisLoer
Copy link
Contributor

@ChrisLoer ChrisLoer commented Dec 19, 2018

This is a proof-of-concept for a fix to issue #11561 -- for small offline packs, the size of the pack is dominated by the space taken up by CJK glyphs, even though many or most people won't use them:

  • For maps intended for CJK audiences, we typically recommend using local CJK glyph generation, so it's not necessary to cache the glyphs in an offline pack.
  • For offline maps intended for non-CJK audiences, it is likely that the offline region covers an area in which there are no or very few CJK glyphs, so there's a good chance it's OK to exclude them.

For a z1/z2 offline region of Satellite Streets, it looks like excluding CJK glyphs brings the bundle size down from ~90MB to ~12MB.

screenshot 2018-12-18 17 27 18

Currently the code defaults to the existing behavior of including the glyphs. We should consider defaulting to excluding the glyphs.

TODO:

  • Android implementation
  • Automated tests
  • Documentation

cc @1ec5 @lloydsheng @kkaefer

@ChrisLoer ChrisLoer added the ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold label Dec 19, 2018
@ChrisLoer ChrisLoer requested a review from 1ec5 as a code owner December 19, 2018 01:35
@ChrisLoer ChrisLoer requested a review from a team December 19, 2018 01:35
@ChrisLoer
Copy link
Contributor Author

@LukasPaczos For the Android implementation I didn't see any automated tests (outside of the core tests), but I just manually tested changing the value in OfflineActivity.

bin/offline.cpp Show resolved Hide resolved
platform/darwin/src/MGLTilePyramidOfflineRegion.h Outdated Show resolved Hide resolved
platform/darwin/src/MGLShapeOfflineRegion.h Outdated Show resolved Hide resolved
platform/darwin/src/MGLShapeOfflineRegion.h Outdated Show resolved Hide resolved
platform/darwin/src/MGLShapeOfflineRegion.h Outdated Show resolved Hide resolved
platform/darwin/src/MGLShapeOfflineRegion.h Outdated Show resolved Hide resolved
platform/macos/app/Base.lproj/MapDocument.xib Outdated Show resolved Hide resolved
platform/macos/app/MapDocument.m Outdated Show resolved Hide resolved
@ChrisLoer
Copy link
Contributor Author

@LukasPaczos My initial Android implementation has the same issue @1ec5 noted in the iOS implementation: the addition of an extra parameter to the OfflineTilePyramidRegionDefinition and the OfflineGeometryRegionDefinition constructors isn't backwards compatible, and I don't think Java supports default argument values.

What do you think is the idiomatic way to handle this? Create a second constructor that takes the extra argument?

Copy link
Contributor

@1ec5 1ec5 left a comment

Choose a reason for hiding this comment

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

Looks good on the iOS/macOS front. Suggested iOS/macOS changelog blurb:

Added the MGLOfflineRegion.includesIdeographicGlyphs property, which you can set to NO to exclude CJK glyphs and save space. (#13607)

platform/ios/app/MBXOfflinePacksTableViewController.m Outdated Show resolved Hide resolved
@1ec5
Copy link
Contributor

1ec5 commented Dec 21, 2018

eb46bd8c7f6b982d308d47dd2e76be15b65816d7 fixes some Auto Layout misplaced view warnings by adding constraints to the checkbox and updating the buttons’ constraints.

Copy link
Member

@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.

What do you think is the idiomatic way to handle this? Create a second constructor that takes the extra argument?

Yes, the best way would be to create a second constructor with the extra argument which is called by the original constructor with the default value as the extra argument.

@ChrisLoer
Copy link
Contributor Author

OK, I think we're good to go, thanks for the help, everybody!

~450 lines touched to add a boolean flag. 😬

@ChrisLoer ChrisLoer added offline and removed ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold labels Dec 21, 2018
@ChrisLoer ChrisLoer changed the title [WIP] Optionally exclude CJK glyphs from offline downloads Optionally exclude CJK glyphs from offline downloads Dec 21, 2018
@ChrisLoer ChrisLoer merged commit d3bb518 into master Dec 21, 2018
@kkaefer kkaefer deleted the no-offline-cjk branch January 2, 2019 11:02
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants