Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(Android): drop zero frames in release mode #607

Merged
merged 9 commits into from
Jan 3, 2025
2 changes: 2 additions & 0 deletions .github/pull_request_template.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,10 @@ What is this PR for?
iOS
- [ ] If you added any user-facing strings on iOS, are they included in Localizable.xcstrings?
- [ ] Add temporary machine translations, marked "Needs Review"

android
- [ ] All user-facing strings added to strings resource
- [ ] Expensive calculations are run in `withContext(Dispatchers.Default)` where possible

### Testing

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import androidx.compose.runtime.collectAsState
import androidx.compose.runtime.getValue
import androidx.compose.runtime.mutableStateOf
import androidx.compose.runtime.remember
import androidx.compose.runtime.rememberCoroutineScope
import androidx.compose.runtime.setValue
import androidx.compose.ui.Alignment
import androidx.compose.ui.Modifier
Expand Down Expand Up @@ -63,6 +64,7 @@ import com.mbta.tid.mbta_app.model.response.StopMapResponse
import io.github.dellisd.spatialk.geojson.Position
import kotlin.time.Duration.Companion.seconds
import kotlinx.coroutines.flow.map
import kotlinx.coroutines.launch

@OptIn(MapboxExperimental::class, ExperimentalPermissionsApi::class)
@Composable
Expand All @@ -82,6 +84,7 @@ fun HomeMapView(
var nearbyTransitSelectingLocation by nearbyTransitSelectingLocationState
val previousNavEntry: NavBackStackEntry? = rememberPrevious(current = currentNavEntry)

val coroutineScope = rememberCoroutineScope()
val layerManager = remember { LazyObjectQueue<MapLayerManager>() }
var selectedStop by remember { mutableStateOf<Stop?>(null) }

Expand Down Expand Up @@ -128,7 +131,7 @@ fun HomeMapView(
viewportProvider.animateTo(stop.position.toMapbox())
}

fun updateDisplayedRoutesBasedOnStop() {
suspend fun updateDisplayedRoutesBasedOnStop() {
val globalResponse = globalResponse ?: return
val railRouteShapes = railRouteShapes ?: return
val stopMapData = stopMapData ?: return
Expand Down Expand Up @@ -159,19 +162,19 @@ fun HomeMapView(
}
}

fun refreshRouteLineSource() {
suspend fun refreshRouteLineSource() {
val routeData = railRouteLineData ?: return
layerManager.run {
updateRouteSourceData(RouteFeaturesBuilder.buildCollection(routeData).toMapbox())
}
}

fun refreshStopSource() {
suspend fun refreshStopSource() {
val sourceData = stopSourceData ?: return
layerManager.run { updateStopSourceData(sourceData) }
}

fun handleNearbyNavRestoration() {
suspend fun handleNearbyNavRestoration() {
if (
previousNavEntry?.destination?.route?.contains("NearbyTransit") == true &&
currentNavEntry?.destination?.route?.contains("StopDetails") == true
Expand All @@ -186,7 +189,7 @@ fun HomeMapView(
}
}

fun handleNavChange() {
suspend fun handleNavChange() {
handleNearbyNavRestoration()
val stopId = currentNavEntry?.arguments?.getString("stopId")
if (stopId == null) {
Expand All @@ -211,8 +214,10 @@ fun HomeMapView(
mapEvents =
MapEvents(
onStyleLoaded = {
layerManager.run {
addLayers(if (isDarkMode) ColorPalette.dark else ColorPalette.light)
coroutineScope.launch {
layerManager.run {
addLayers(if (isDarkMode) ColorPalette.dark else ColorPalette.light)
}
}
},
onCameraChanged = { viewportProvider.updateCameraState(it.cameraState) }
Expand Down Expand Up @@ -299,8 +304,9 @@ fun HomeMapView(
}

MapEffect { map ->
if (layerManager.`object` == null)
layerManager.`object` = MapLayerManager(map.mapboxMap, context)
if (layerManager.`object` == null) {
layerManager.setObject(MapLayerManager(map.mapboxMap, context))
}
}

if (
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ package com.mbta.tid.mbta_app.android.map

import android.content.Context
import android.graphics.drawable.BitmapDrawable
import androidx.annotation.AnyThread
import androidx.annotation.MainThread
import com.mapbox.geojson.FeatureCollection
import com.mapbox.maps.GeoJSONSourceData
import com.mapbox.maps.MapboxMap
Expand All @@ -18,6 +20,8 @@ import com.mbta.tid.mbta_app.map.RouteLayerGenerator
import com.mbta.tid.mbta_app.map.StopFeaturesBuilder
import com.mbta.tid.mbta_app.map.StopIcons
import com.mbta.tid.mbta_app.map.StopLayerGenerator
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.withContext

class MapLayerManager(val map: MapboxMap, context: Context) {
init {
Expand All @@ -28,51 +32,62 @@ class MapLayerManager(val map: MapboxMap, context: Context) {
}
}

fun addSource(source: GeoJsonSource) {
this.map.addSource(source)
@AnyThread
suspend fun addSource(source: GeoJsonSource) {
withContext(Dispatchers.Main) { map.addSource(source) }
}

fun addLayers(colorPalette: ColorPalette) {
@AnyThread
suspend fun addLayers(colorPalette: ColorPalette) {
val layers: List<MapboxLayer> =
RouteLayerGenerator.createAllRouteLayers(colorPalette).map { it.toMapbox() } +
StopLayerGenerator.createStopLayers(colorPalette).map { it.toMapbox() }
for (layer in layers) {
if (map.styleLayerExists(checkNotNull(layer.layerId))) {
// Skip attempting to add layer if it already exists
continue
withContext(Dispatchers.Default) {
RouteLayerGenerator.createAllRouteLayers(colorPalette).map { it.toMapbox() } +
StopLayerGenerator.createStopLayers(colorPalette).map { it.toMapbox() }
}
if (map.styleLayerExists("puck")) {
map.addLayerBelow(layer, below = "puck")
} else {
map.addLayer(layer)
withContext(Dispatchers.Main) {
for (layer in layers) {
if (map.styleLayerExists(checkNotNull(layer.layerId))) {
// Skip attempting to add layer if it already exists
continue
}
if (map.styleLayerExists("puck")) {
map.addLayerBelow(layer, below = "puck")
} else {
map.addLayer(layer)
}
}
}
}

@MainThread
boringcactus marked this conversation as resolved.
Show resolved Hide resolved
fun resetPuckPosition() {
if (map.styleLayerExists("puck")) {
map.moveStyleLayer("puck", null)
}
}

private fun updateSourceData(sourceId: String, data: FeatureCollection) {
if (map.styleSourceExists(sourceId)) {
map.setStyleGeoJSONSourceData(
sourceId,
"",
GeoJSONSourceData(checkNotNull(data.features()))
)
@AnyThread
private suspend fun updateSourceData(sourceId: String, data: FeatureCollection) {
// styleSourceExists is not thread safe, but setStyleGeoJSONSourceData is
if (withContext(Dispatchers.Main) { map.styleSourceExists(sourceId) }) {
withContext(Dispatchers.Default) {
map.setStyleGeoJSONSourceData(
sourceId,
"",
GeoJSONSourceData(checkNotNull(data.features()))
)
}
} else {
val source = GeoJsonSource.Builder(sourceId).featureCollection(data).build()
addSource(source)
}
}

fun updateRouteSourceData(routeData: FeatureCollection) {
suspend fun updateRouteSourceData(routeData: FeatureCollection) {
updateSourceData(RouteFeaturesBuilder.routeSourceId, routeData)
}

fun updateStopSourceData(stopData: FeatureCollection) {
suspend fun updateStopSourceData(stopData: FeatureCollection) {
updateSourceData(StopFeaturesBuilder.stopSourceId, stopData)
}
}
Original file line number Diff line number Diff line change
@@ -1,18 +1,17 @@
package com.mbta.tid.mbta_app.android.map

import androidx.compose.runtime.Composable
import androidx.compose.runtime.LaunchedEffect
import androidx.compose.runtime.collectAsState
import androidx.compose.runtime.getValue
import androidx.compose.runtime.mutableStateOf
import androidx.compose.runtime.remember
import androidx.compose.runtime.setValue
import androidx.lifecycle.ViewModel
import androidx.lifecycle.ViewModelProvider
import androidx.lifecycle.viewModelScope
import com.mapbox.common.HttpServiceFactory
import com.mapbox.common.MapboxOptions
import com.mapbox.geojson.FeatureCollection
import com.mbta.tid.mbta_app.android.util.rememberSuspend
import com.mbta.tid.mbta_app.dependencyInjection.UsecaseDI
import com.mbta.tid.mbta_app.map.RouteFeaturesBuilder
import com.mbta.tid.mbta_app.map.RouteLineData
Expand All @@ -35,6 +34,7 @@ import kotlinx.coroutines.flow.StateFlow
import kotlinx.coroutines.flow.debounce
import kotlinx.coroutines.flow.first
import kotlinx.coroutines.launch
import kotlinx.coroutines.withContext
import kotlinx.datetime.Clock
import kotlinx.datetime.Instant
import org.koin.core.component.KoinComponent
Expand Down Expand Up @@ -103,18 +103,16 @@ open class MapViewModel(
}

override suspend fun globalMapData(now: Instant): GlobalMapData? =
globalResponse.first()?.let {
GlobalMapData(it, GlobalMapData.getAlertsByStop(it, alertsData, now))
withContext(Dispatchers.Default) {
globalResponse.first()?.let {
GlobalMapData(it, GlobalMapData.getAlertsByStop(it, alertsData, now))
}
}

@Composable
override fun rememberGlobalMapData(now: Instant): GlobalMapData? {
var result by remember { mutableStateOf<GlobalMapData?>(null) }
val globalResponse by this.globalResponse.collectAsState(initial = null)
LaunchedEffect(this.alertsData, globalResponse, now) {
launch(Dispatchers.IO) { result = globalMapData(now) }
}
return result
return rememberSuspend(this.alertsData, globalResponse, now) { globalMapData(now) }
}

override suspend fun refreshRouteLineData(now: Instant) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import com.mbta.tid.mbta_app.android.state.getNearby
import com.mbta.tid.mbta_app.android.state.getSchedule
import com.mbta.tid.mbta_app.android.state.subscribeToPredictions
import com.mbta.tid.mbta_app.android.util.managePinnedRoutes
import com.mbta.tid.mbta_app.android.util.rememberSuspend
import com.mbta.tid.mbta_app.android.util.timer
import com.mbta.tid.mbta_app.model.Coordinate
import com.mbta.tid.mbta_app.model.NearbyStaticData
Expand All @@ -32,6 +33,8 @@ import com.mbta.tid.mbta_app.model.response.GlobalResponse
import com.mbta.tid.mbta_app.model.withRealtimeInfo
import io.github.dellisd.spatialk.geojson.Position
import kotlin.time.Duration.Companion.seconds
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.withContext

@Composable
fun NearbyTransitView(
Expand All @@ -58,7 +61,7 @@ fun NearbyTransitView(
val (pinnedRoutes, togglePinnedRoute) = managePinnedRoutes()

val nearbyWithRealtimeInfo =
remember(
rememberSuspend(
nearby,
globalResponse,
targetLocation,
Expand All @@ -68,19 +71,21 @@ fun NearbyTransitView(
now,
pinnedRoutes
) {
if (targetLocation != null) {
nearby?.withRealtimeInfo(
globalData = globalResponse,
sortByDistanceFrom = targetLocation,
schedules,
predictions,
alertData,
now,
pinnedRoutes.orEmpty(),
useTripHeadsigns = false,
)
} else {
null
withContext(Dispatchers.Default) {
if (targetLocation != null) {
nearby?.withRealtimeInfo(
globalData = globalResponse,
sortByDistanceFrom = targetLocation,
schedules,
predictions,
alertData,
now,
pinnedRoutes.orEmpty(),
useTripHeadsigns = false,
)
} else {
null
}
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,20 +3,22 @@ package com.mbta.tid.mbta_app.android.pages
import androidx.compose.material3.ExperimentalMaterial3Api
import androidx.compose.runtime.Composable
import androidx.compose.runtime.LaunchedEffect
import androidx.compose.runtime.remember
import androidx.compose.ui.Modifier
import com.mapbox.maps.MapboxExperimental
import com.mbta.tid.mbta_app.android.state.getGlobalData
import com.mbta.tid.mbta_app.android.state.getSchedule
import com.mbta.tid.mbta_app.android.state.subscribeToPredictions
import com.mbta.tid.mbta_app.android.stopDetails.StopDetailsView
import com.mbta.tid.mbta_app.android.util.managePinnedRoutes
import com.mbta.tid.mbta_app.android.util.rememberSuspend
import com.mbta.tid.mbta_app.android.util.timer
import com.mbta.tid.mbta_app.model.Stop
import com.mbta.tid.mbta_app.model.StopDetailsDepartures
import com.mbta.tid.mbta_app.model.StopDetailsFilter
import com.mbta.tid.mbta_app.model.response.AlertsStreamDataResponse
import kotlin.time.Duration.Companion.seconds
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.withContext

@Composable
@ExperimentalMaterial3Api
Expand All @@ -41,7 +43,7 @@ fun StopDetailsPage(
val (pinnedRoutes, togglePinnedRoute) = managePinnedRoutes()

val departures =
remember(
rememberSuspend(
stop,
globalResponse,
schedulesResponse,
Expand All @@ -50,18 +52,20 @@ fun StopDetailsPage(
pinnedRoutes,
now
) {
if (globalResponse != null) {
StopDetailsDepartures.fromData(
stop,
globalResponse,
schedulesResponse,
predictionsResponse,
alertData,
pinnedRoutes.orEmpty(),
now,
useTripHeadsigns = false,
)
} else null
withContext(Dispatchers.Default) {
if (globalResponse != null) {
StopDetailsDepartures.fromData(
stop,
globalResponse,
schedulesResponse,
predictionsResponse,
alertData,
pinnedRoutes.orEmpty(),
now,
useTripHeadsigns = false,
)
} else null
}
}

LaunchedEffect(departures) { updateDepartures(departures) }
Expand Down
Loading
Loading