Updated MGLScaleBar to use rendered UIImages instead of MGLScaleBarLabel #11921
Conversation
julianrex
added
iOS
|
Rebasing after release-boba merge... |
platform/ios/app/MBXViewController.m
Outdated
|
|
||
| // Now create a world coord | ||
| CLLocationDegrees heading = drand48()*360.0; | ||
| CLLocationDistance dist = drand48()*radius; |
platform/ios/app/MBXViewController.m
Outdated
| for (NSInteger i = 0; i<numAnnotations; i++) { | ||
|
|
||
| CLLocationDegrees heading = drand48()*360.0; | ||
| CLLocationDistance dist = drand48()*radius; |
platform/ios/app/MBXViewController.m
Outdated
| [self _randomWorldTourInternal]; | ||
| } | ||
|
|
||
| - (void)_randomWorldTourInternal { |
There was a problem hiding this comment.
There is no need for an underscored private method.
There was a problem hiding this comment.
I've noticed that we've used _ as a prefix for private methods in the SDK. Do we have a rule of thumb?
There was a problem hiding this comment.
We have some but mostly on SDK classes. This is part of the demo app. I'm OK if you want to leave it as it is.
platform/ios/src/MGLScaleBar.mm
Outdated
|
|
||
| [[NSNotificationCenter defaultCenter] addObserver:self | ||
| selector:@selector(currentLocaleDidChange:) | ||
| name:NSCurrentLocaleDidChangeNotification |
There was a problem hiding this comment.
I may be wrong but I think this notification is not triggered on iOS since changing the locale language requires an app restart.
There was a problem hiding this comment.
@fabian-guerra is correct — NSCurrentLocaleDidChangeNotification isn’t relevant on iOS, as the system does a soft-reboot whenever the language/locale/region is changed and apps reload all of their views.
(This notification is sent on macOS, but this code doesn’t support that platform.)
platform/ios/src/MGLScaleBar.mm
Outdated
| } | ||
|
|
||
| - (void)currentLocaleDidChange:(NSNotification*)notification { | ||
| // Clear the cache, so labels should be rebuilt. | ||
| [self.labelImageCache removeAllObjects]; |
There was a problem hiding this comment.
Would it be necessary freeing the cache when the app triggers a memory warning?
There was a problem hiding this comment.
IIRC - no, since the code uses NSCache rather than a dictionary. It has behaviour to evict objects in low memory situations (though seems unpredictable).
There was a problem hiding this comment.
Right and it may not free anything if it receives a memory warning. If you consider is not necessary then I'm OK.
Edit: No changelog entry yetEdit 2: Updated with screenshots of Instruments running the
randomWorldTourexample on an iPhone X, specifically looking at CPU and memory usage (not FPS).CPU / MGLScaleBarLabel
Before: In the following image, I'm filtering symbols for
drawTextInRectwhich was only used byMGLScaleBarLabel. You can see, in this test, taking up a total of 14% of the CPU - significant for such a small on-screen item. You can also see that allocations take up a good chunk of this too (and if we filter onmalloc- that actually goes up further).After: Filtering on
drawTextInRectand thenMGLScaleBar, the effect is essentially non-existent:Memory impact
Before: The following includes transient allocations i.e. allocations that have come and gone. The highlighted line, 33%, will include will include all allocations from
CA::Transaction::commit(), but notice on the right that the "Heaviest Stack Trace" is ourMGLScaleBarLabel.After: Hunting for the same
CA::Transaction::commit()as above:Transient allocations
Before: Looking at all allocations, sorted by transient, we see:
After: In these traces, the comparisons aren't exact, but it's worth noting how the top 3 small allocations compare with the next largest chunk,
mbgl::VectorTileFeature- this reduced much more than I was expecting (and why I included these traces).