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 @@ -18,6 +18,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,23 +30,27 @@ class MapLayerManager(val map: MapboxMap, context: Context) {
}
}

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

fun addLayers(colorPalette: ColorPalette) {
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)
}
}
}
}
Expand All @@ -55,24 +61,27 @@ class MapLayerManager(val map: MapboxMap, context: Context) {
}
}

private fun updateSourceData(sourceId: String, data: FeatureCollection) {
if (map.styleSourceExists(sourceId)) {
map.setStyleGeoJSONSourceData(
sourceId,
"",
GeoJSONSourceData(checkNotNull(data.features()))
)
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
Original file line number Diff line number Diff line change
Expand Up @@ -6,20 +6,22 @@ package com.mbta.tid.mbta_app.android.util
*/
class LazyObjectQueue<T> {
var `object`: T? = null
set(value) {
field = value
if (value != null) {
val oldQueue = queue
queue = mutableListOf()
for (op in oldQueue) {
value.op()
}
private set

suspend fun setObject(value: T?) {
`object` = value
if (value != null) {
val oldQueue = queue
queue = mutableListOf()
for (op in oldQueue) {
value.op()
}
}
}

private var queue: MutableList<T.() -> Unit> = mutableListOf()
private var queue: MutableList<suspend T.() -> Unit> = mutableListOf()

fun run(op: T.() -> Unit) {
suspend fun run(op: suspend T.() -> Unit) {
val `object` = this.`object`
if (`object` != null) {
`object`.op()
Expand Down
Loading
Loading