Shields loading optimization #6826

tomaszrybakiewicz · 2023-01-12T16:11:54Z

Closes NAVAND-483

Description

After reviewing RoadShiled loading logic, I've noticed that we were caching RoadShield image data in 3 different places.

ShieldResultCache - RouteShield in LruCache
ShieldByteArrayCache - ByteArray in LruCache
Native TileStore - ByteArray on disk

The ShieldResultCache already caches image data together with RouteShield; therefore, ShieldByteArrayCache is redundant.

In this PR, I optimized RoadShield loading by:

deleting duplicate image cache
separating download from caching logic - introduced local ResourceLoader and CacheResourceLoader decorator
renaming ShieldResultCache -> RoadShieldLoader and loosely coupling it with sprites and image loaders
loosely coupling RoadShieldContentManagerImpl with the RoadShieldLoader class

(new setup)

- deleted duplicate image cache - ShieldsCache and ShieldByteArrayCache were caching same data twice - separated download from caching logic by introducing ResourceLoader and CacheResourceLoader local interfaces - renamed ShieldResultCache -> RoadShieldLoader and loosely coupled it with sprites and image loaders - updated RoadShieldContentManagerImpl - loosely coupled it with loader class

github-actions · 2023-01-12T16:12:25Z

Changelog

Features

Introduced ViewStyleCustomization.infoPanelGuidelineMaxPosPercent that allows customization of the NavigationView InfoPanel bottom guideline maximum position. Increased default value to 50%. #6792

Bug fixes and improvements

Fixed an issue with NavigationView that caused info panel to shrink in landscape mode with a full screen theme. #6780

⚠️ Updated the NavigationView default navigation puck asset. #6678

Previous puck can be restored by injecting LocationPuck2D with the bearingImage set to com.mapbox.navigation.ui.maps.R.drawable.mapbox_navigation_puck_icon drawable:

navigationView.customizeViewStyles {
    locationPuckOptions = LocationPuckOptions.Builder(context)
        .defaultPuck(
            LocationPuck2D(
                bearingImage = ContextCompat.getDrawable(
                    context,
                    com.mapbox.navigation.ui.maps.R.drawable.mapbox_navigation_puck_icon,
                )
            )
        )
        .idlePuck(regularPuck(context))
        .build()
}

Fixed an issue where the first voice instruction might have been played twice. #6766
Added guarantees that route progress with RouteProgress#currentState == OFF_ROUTE arrives earlier than NavigationRerouteController#reroute is called. #6764
Introduced NavigationViewListener.onSpeedInfoClicked that would be triggered when MapboxSpeedInfoView is clicked upon. #6770
Each newly instantiated MapboxRouteArrowView class will initialize the layers with the provided options on the first render call. Previously this would only be done if the layers hadn't already been initialized. #6466
Fixed a rare java.lang.NullPointerException: Attempt to read from field 'SpeechAnnouncement PlayCallback.announcement' on a null object reference crash in PlayCallback.getAnnouncement. #6760
Fixed standalone MapboxManeuverView appearance when the app also integrates Drop-In UI. #6774

Known issues ⚠️

Other changes

Android Auto Changelog

Features

Bug fixes and improvements

dzinad · 2023-01-17T08:04:30Z

...i-shield/src/main/java/com/mapbox/navigation/ui/shield/internal/loader/ResourceDownloader.kt

+
+private typealias LoaderCallback<R> = (Expected<String, R>) -> Unit
+
+internal abstract class ResourceDownloader<I, R> : ResourceLoader<I, R> {


"I" stands for "input"?

dzinad · 2023-01-17T08:08:42Z

libnavui-shield/src/main/java/com/mapbox/navigation/ui/shield/internal/loader/ResourceLoader.kt

+
+import com.mapbox.bindgen.Expected
+
+internal fun interface ResourceLoader<Argument, Resource> {


There is already a ResourceLoader in com.mapbox.navigation.ui.utils.internal.resource. Can we reuse that one? I find 2 classes with the same quite an abstract name confusing.

No. They are not the same. The utils ResourceLoader is a global facade over the native TileStore, and it acts more like Downloader than a local loader (maybe ResourceDownloader would be a better name for it).
This ResourceLoader is an internal interface. It offers an abstraction for a generic, async loader and helps break tight coupling between the RoadShieldContentManager and concrete loaders.
I am aware that the name clashes with the utils version. We can shorten it to just a Loader if it helps. Furthermore, I would like to introduce this abstraction here first and later, if we find it useful elsewhere, move it to the utils module as well.

They should have different names. Especially considering the fact they are interfaces. So you can't even just look at the class code and understand the difference.
Maybe the utils one should be TileStoreResourcesLoader or sth like that. Since it uses Common classes in the API anyway.

Since they are internal interfaces they don't have to have different names.
Anyway, I've renamed it to just Loader.

Since they are internal interfaces they don't have to have different names.

Technically of course they don't, but for better understanding they should. Especially considering the fact that utils ResourceLoader is accessible from other modules.

Anyway, I've renamed it to just Loader.

It's not really helpful. Now it looks like the ResourceLoader should implement Loader and Loader should be in utils module as well since it's general enough. I think in this case specifying what utils ResourceLoader does is a better way. TileStoreResourceLoader, for example.

What exactly are you negating here? And why?
What don't you like about my suggestion and what are the advantages of yours?

dzinad · 2023-01-17T08:09:53Z

...i-shield/src/main/java/com/mapbox/navigation/ui/shield/internal/loader/ResourceDownloader.kt

+
+private typealias LoaderCallback<R> = (Expected<String, R>) -> Unit
+
+internal abstract class ResourceDownloader<I, R> : ResourceLoader<I, R> {


Why don't you use TileStore cache? Aren't your needs met by TileStore?

RoadShieldDownloader implicitly uses TIleStore via the utils ResourceLoader facade.

Yes, but isn't the cache TileStore provides enough? I can assume it's not because it was not enough for voice instructions. Just want to make sure that writing our own cache above TileStore is a conscious choice.

Just want to make sure that writing our own cache above TileStore is a conscious choice.

What's that supposed to mean? First, of course, it is a conscious choice! Second, loading voice instructions don't require parsing large JSON strings with ~400 entries. Third, shields require loading two resources: a) a sprites list; b) images. Finally, the in-memory cache is necessary to avoid redundant I/O requests and computationally heavy result processing.

It means that we are aware that TileStore provides us with a caching mechanism but we write our own because it doesn't meet our needs.

…pdated return type to return Expected<Error, V> instead of Expected<String, V>

codecov · 2023-01-17T17:32:11Z

Codecov Report

Merging #6826 (b8a6655) into main (61415ea) will decrease coverage by 0.04%.
The diff coverage is 85.29%.

@@             Coverage Diff              @@
##               main    #6826      +/-   ##
============================================
- Coverage     72.64%   72.61%   -0.04%     
- Complexity     5569     5600      +31     
============================================
  Files           780      783       +3     
  Lines         30104    30106       +2     
  Branches       3553     3555       +2     
============================================
- Hits          21869    21860       -9     
- Misses         6808     6815       +7     
- Partials       1427     1431       +4

Impacted Files	Coverage Δ
...ion/ui/shield/RoadShieldContentManagerContainer.kt	`8.33% <0.00%> (-16.67%)`	⬇️
...igation/ui/shield/internal/RoadShieldDownloader.kt	`64.28% <66.66%> (ø)`
...navigation/ui/shield/internal/loader/Downloader.kt	`70.00% <70.00%> (ø)`
...vigation/ui/shield/RoadShieldContentManagerImpl.kt	`91.30% <90.74%> (-2.39%)`	⬇️
.../shield/internal/loader/ShieldSpritesDownloader.kt	`94.11% <94.11%> (ø)`
...tion/ui/shield/internal/loader/RoadShieldLoader.kt	`96.29% <96.29%> (ø)`
...vigation/ui/shield/internal/loader/CachedLoader.kt	`100.00% <100.00%> (ø)`

tomaszrybakiewicz · 2023-02-17T21:49:11Z

@abhishek1508 Can you review and merge this?

abhishek1508 · 2023-02-21T15:08:10Z

@abhishek1508 Can you review and merge this?

cc @LukasPaczos

tomaszrybakiewicz added UI Work related to visual components, Android Auto, Camera, 3D, voice, etc. skip changelog Should not be added into version changelog. labels Jan 12, 2023

tomaszrybakiewicz requested a review from abhishek1508 January 12, 2023 16:11

github-actions bot requested a review from kmadsen January 12, 2023 16:12

dzinad reviewed Jan 17, 2023

View reviewed changes

Renamed ResourceLoader -> Loader, ResourceDownloader -> Downloader. U…

b8a6655

…pdated return type to return Expected<Error, V> instead of Expected<String, V>

kmadsen removed their request for review November 13, 2023 19:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Shields loading optimization #6826

Shields loading optimization #6826

tomaszrybakiewicz commented Jan 12, 2023 •

edited

github-actions bot commented Jan 12, 2023

Features

Bug fixes and improvements

Known issues ⚠️

Other changes

Features

Bug fixes and improvements

dzinad Jan 17, 2023

tomaszrybakiewicz Jan 17, 2023

dzinad Jan 17, 2023

tomaszrybakiewicz Jan 17, 2023

dzinad Jan 17, 2023

tomaszrybakiewicz Jan 17, 2023

dzinad Jan 18, 2023

tomaszrybakiewicz Jan 18, 2023

dzinad Jan 20, 2023

dzinad Jan 17, 2023

tomaszrybakiewicz Jan 17, 2023

dzinad Jan 17, 2023

tomaszrybakiewicz Jan 17, 2023

dzinad Jan 18, 2023

codecov bot commented Jan 17, 2023 •

edited

tomaszrybakiewicz commented Feb 17, 2023

abhishek1508 commented Feb 21, 2023


		private typealias LoaderCallback<R> = (Expected<String, R>) -> Unit

		internal abstract class ResourceDownloader<I, R> : ResourceLoader<I, R> {


		import com.mapbox.bindgen.Expected

		internal fun interface ResourceLoader<Argument, Resource> {

Shields loading optimization #6826

Are you sure you want to change the base?

Shields loading optimization #6826

Conversation

tomaszrybakiewicz commented Jan 12, 2023 • edited

Description

github-actions bot commented Jan 12, 2023

Features

Bug fixes and improvements

Known issues ⚠️

Other changes

Features

Bug fixes and improvements

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 17, 2023 • edited

Codecov Report

tomaszrybakiewicz commented Feb 17, 2023

abhishek1508 commented Feb 21, 2023

tomaszrybakiewicz commented Jan 12, 2023 •

edited

codecov bot commented Jan 17, 2023 •

edited