WorldScaleSceneView: Handle permissions and implement world tracking #709

hud10837 · 2025-01-20T13:55:10Z

Related to issue: https://devtopia.esri.com/runtime/kotlin/issues/5175

Description:

Adds world tracking and fixes the sequence of initialization, including handling multiple permissions, in the WorldScaleSceneView

Summary of changes:

Wraps the LocationDataSource in a new wrapper class to ensure it responds to lifecycle events
Requests location permissions automatically and fixes issues related to permission requests
Removes the heading change listener -- Swift wasn't doing this and I don't think it was necessary
Filters out locations based on the Swift model and resets the camera if it deviates from the LDS position more than 2 meters.

Pre-merge Checklist

a vTest Job for this PR has been run
- link: ~~https://runtime-kotlin.esri.com/view/all/job/vtest/job/toolkit/298/~~
- https://runtime-kotlin.esri.com/view/all/job/vtest/job/toolkit/299/
Unit and/or integration tests have been added to exercise this PR's logic, and the tests are passing:
- Yes
- No

hud10837 · 2025-01-21T09:12:53Z

...WorldScaleApp/app/src/main/java/com/arcgismaps/toolkit/arworldscaleapp/screens/MainScreen.kt

@@ -39,7 +39,6 @@ fun MainScreen() {
    }
    WorldScaleSceneView(
        arcGISScene = arcGISScene,
-        locationDataSource = SystemLocationDataSource(),


This will no longer be passed in by the user

hud10837 · 2025-01-21T09:14:30Z

toolkit/ar/src/main/java/com/arcgismaps/toolkit/ar/WorldScaleSceneView.kt

-        launch {
-            locationDataSource.status.collect {
+    var isLocationDataSourceStarted by remember { mutableStateOf(false) }
+    if (hasCompletedPermissionRequest && context.checkPermissionsGranted()) {


context.checkPermissionsGranted() will re-check the permissions with the system every time it is called, rather than storing that in a mutable state. I found this was much more predictable and easy to understand than using state, and it should only run once when hasCompletedPermissionRequest is true

When hasCompletedPermissionRequest is true, doesn't this mean that all required permissions have been granted? Why do we need to then check again with checkPermissionsGranted()?

In the version you reviewed, hasCompletedPermissionRequest could be true even if all permissions were not granted. However given your question I thought about it more and reworked it so that the mutable state encodes whether all permissions were granted.

hud10837 · 2025-01-21T09:14:50Z

toolkit/ar/src/main/java/com/arcgismaps/toolkit/ar/WorldScaleSceneView.kt

    }

    Box(modifier = modifier) {
-        if (cameraPermissionGranted && arCoreInstalled) {
+        if (isLocationDataSourceStarted && arCoreInstalled) {


isLocationDataSourceStarted will only be true if permissions have already been granted

hud10837 · 2025-01-21T09:16:39Z

toolkit/ar/src/main/java/com/arcgismaps/toolkit/ar/WorldScaleSceneView.kt

+            hasCompletedRequest.value = true
+        }
+
+    if (!hasLaunchedRequest) {
+        SideEffect {
+            hasLaunchedRequest = true
+            launcher.launch(permissionsToRequest.toTypedArray())
+        }
+    }
+
+    return hasCompletedRequest


Note that hasLaunchedRequest is used internally to this function to ensure the request is only launched once. hasCompletedRequest is used for the WorldScaleSceneView to know it's okay to continue with the startup sequence

…hub.com/Esri/arcgis-maps-sdk-kotlin-toolkit into hud10837/implement-world-tracking

01smito01

Nothing obviously wrong to me, only a few queries & suggestions

toolkit/ar/src/androidTest/java/com/arcgismaps/toolkit/ar/LocationFilteringTests.kt

toolkit/ar/src/main/java/com/arcgismaps/toolkit/ar/WorldScaleSceneView.kt

01smito01 · 2025-01-21T16:33:17Z

toolkit/ar/src/main/java/com/arcgismaps/toolkit/ar/WorldScaleSceneView.kt

+    requestCameraPermissionAutomatically: Boolean,
+    requestLocationPermissionAutomatically: Boolean,
+    initializationStatus: MutableState<WorldScaleSceneViewStatus>,
+    onInitializationStatusChanged: ((WorldScaleSceneViewStatus) -> Unit)?


What's the thinking behind this (and ultimately MutableState<T>.update from ARHelpers) being nullable rather than defaulting to {}?

These are both meant to be set to the callback parameter that's set on the WorldScaleSceneView, and the convention we've used there for the GeoViews and TableTopSceneView is to make them nullable. IIRC this was because by making them nullable we can easily check if they are set to something meaningful, and if not, we can avoid doing calculation (like listening to the state flows of the underlying geoview). I'm not sure how big an improvement that was but given the convention it makes sense to keep it going. In the case of these utility functions, I think it's easier to read the calling code if it just passes through the value from the WSSV argument, rather than having to do a null check inline

toolkit/ar/src/main/java/com/arcgismaps/toolkit/ar/WorldScaleSceneView.kt

gunt0001 · 2025-01-22T13:17:13Z

toolkit/ar/src/androidTest/java/com/arcgismaps/toolkit/ar/LocationFilteringTests.kt

+        val oldLocationFarFarAway = Location.create(farFarAway, 1.0, 1.0, 0.0, 0.0, true, Instant.now().minusMillis(30000))
+        val recentLocationWithNoAccuracy = Location.create(nullIsland, Double.NaN, Double.NaN, 0.0, 0.0, false, Instant.now().minusMillis(50))
+
+        assert(!shouldUpdateCamera(recentLocationAtNullIsland, cameraAtNullIsland))


Better use Truth instead of junit asserts? We have Truth already in the version catalog, just need to add it to the gradle file.

gunt0001 · 2025-01-22T13:39:15Z

toolkit/ar/src/main/java/com/arcgismaps/toolkit/ar/WorldScaleSceneView.kt

+ * @since 200.7.0
+ */
+@Composable
+private fun TrackLocationWithLocationDataSource(


Based on naming conventions, since this composable function doesn't return anything, it should be pascal cased and a noun. Also could make the name a bit shorter? How about:

Suggested change

private fun TrackLocationWithLocationDataSource(

private fun LocationTracker(

gunt0001 · 2025-01-22T13:46:43Z

toolkit/ar/src/main/java/com/arcgismaps/toolkit/ar/WorldScaleSceneView.kt

+                        Camera(
+                            location.position.y,
+                            location.position.x,
+                            if (location.position.hasZ) location.position.z!! else 1.0,


Better set z to 0.0 if we don't have a z value? This is what Swift is doing.

Suggested change

if (location.position.hasZ) location.position.z!! else 1.0,

if (location.position.hasZ) location.position.z!! else 0.0,

toolkit/ar/src/main/java/com/arcgismaps/toolkit/ar/WorldScaleSceneView.kt

+
+                    // We have to do this or the error gets bigger and bigger.
+                    cameraController.transformationMatrix =
+                        TransformationMatrix.createIdentityMatrix()


gunt0001 · 2025-01-22T13:53:13Z

toolkit/ar/src/main/java/com/arcgismaps/toolkit/ar/WorldScaleSceneView.kt

+): Boolean {
+    // filter out old locations
+    if (Instant.now()
+            .toEpochMilli() - location.timestamp.toEpochMilli() > 10.0.seconds.inWholeMilliseconds


Let's keep thresholds centralized. How about some internal object data WorldScaleParameters class?

gunt0001 · 2025-01-22T14:13:22Z

toolkit/ar/src/main/java/com/arcgismaps/toolkit/ar/WorldScaleSceneView.kt

+ * @since 200.7.0
+ */
+@Composable
+private fun requestPermissionsOrFail(


Since this function returns a remembered state variable, I think we should prefix it with "remember", based on convention. How about:

Suggested change

private fun requestPermissionsOrFail(

private fun rememberPermissionsGranted(

gunt0001 · 2025-01-22T14:15:59Z

toolkit/ar/src/main/java/com/arcgismaps/toolkit/ar/WorldScaleSceneView.kt

    modifier: Modifier = Modifier,
    onInitializationStatusChanged: ((WorldScaleSceneViewStatus) -> Unit)? = null,
    requestCameraPermissionAutomatically: Boolean = true,
+    requestLocationPermissionAutomatically: Boolean = true,


I wonder if we should have just a single requestPermissionsAutomatically parameter? What's the use case for having separate ones? We can solve this separate from this PR though.

Let's discuss this. I think there may be use cases for having them separate, but I don't know if it's critical

gunt0001 · 2025-01-22T14:18:13Z

toolkit/ar/src/main/java/com/arcgismaps/toolkit/ar/WorldScaleSceneView.kt

-        launch {
-            locationDataSource.status.collect {
+    var isLocationDataSourceStarted by remember { mutableStateOf(false) }
+    if (hasCompletedPermissionRequest && context.checkPermissionsGranted()) {


When hasCompletedPermissionRequest is true, doesn't this mean that all required permissions have been granted? Why do we need to then check again with checkPermissionsGranted()?

gunt0001 · 2025-01-22T14:24:49Z

toolkit/ar/src/main/java/com/arcgismaps/toolkit/ar/internal/LocationDataSourceWrapper.kt

+        scope.launch {
+            locationDataSource.stop()
+        }


When the wrapper is destroyed, we should just cancel the scope:

Suggested change

scope.launch {

locationDataSource.stop()

}

scope.cancel()

I put this inside the launch, since otherwise I think we defeat the purpose of the wrapper. Let's discuss.

gunt0001 · 2025-01-22T14:26:20Z

toolkit/ar/src/main/java/com/arcgismaps/toolkit/ar/internal/LocationDataSourceWrapper.kt

+        }
+    }
+
+    fun startLocationDataSource() {


Could do this instead in onResume?

Suggested change

fun startLocationDataSource() {

override fun onResume(owner: LifecycleOwner) {

gunt0001 · 2025-01-23T13:16:08Z

toolkit/ar/src/main/java/com/arcgismaps/toolkit/ar/WorldScaleSceneView.kt

+
+/**
+ * Checks the permissions required for the [WorldScaleSceneView] to function and requests them if necessary.
+ * If [requestCameraPermissionAutomatically] or [requestLocationPermissionAutomatically] are set to false,


This doc is out of date

gunt0001 · 2025-01-23T13:18:29Z

toolkit/ar/src/main/java/com/arcgismaps/toolkit/ar/WorldScaleSceneView.kt

+            ActivityResultContracts.RequestMultiplePermissions()
+        ) { grantedState: Map<String, Boolean> ->
+            // This callback will only be relevant to the permissions that were added to the request.
+            // If the user has set [requestCameraPermissionAutomatically] to false, for example,


This comment is out of date

gunt0001 · 2025-01-23T13:19:31Z

toolkit/ar/src/main/java/com/arcgismaps/toolkit/ar/WorldScaleSceneView.kt

+    onInitializationStatusChanged: ((WorldScaleSceneViewStatus) -> Unit)?
+): State<Boolean> {
+    val allPermissionsGranted = remember { mutableStateOf(false) }
+    val permissionsToRequest = mutableListOf<String>()


Where does this list get populated?

gunt0001 · 2025-01-23T13:22:28Z

toolkit/ar/src/main/java/com/arcgismaps/toolkit/ar/WorldScaleSceneView.kt

+ */
+@Composable
+private fun LocationTracker(
+    locationDataSource: LocationDataSource,


I think we can remove this parameter and keep the LDS as an internal remembered variable of this function. The LDS is not used y the client code.

gunt0001 · 2025-01-23T13:28:57Z

toolkit/ar/src/main/java/com/arcgismaps/toolkit/ar/internal/LocationDataSourceWrapper.kt

+ * @since 200.7.0
+ */
+internal class LocationDataSourceWrapper(val locationDataSource: LocationDataSource) : DefaultLifecycleObserver {
+    // This ensures that we can stop the location data source on a scope that won't be cancelled when the composition is destroyed


Suggested change

// This ensures that we can stop the location data source on a scope that won't be cancelled when the composition is destroyed

// coroutine scope that is tied to the lifecycle of this LocationDataSourceWrapper

gunt0001 · 2025-01-23T13:31:52Z

toolkit/ar/src/main/java/com/arcgismaps/toolkit/ar/WorldScaleSceneView.kt

+            wrapper.onDestroy(lifecycleOwner)
+            lifecycleOwner.lifecycle.removeObserver(wrapper)


I think it' safer to do it in the opposite order:

Suggested change

wrapper.onDestroy(lifecycleOwner)

lifecycleOwner.lifecycle.removeObserver(wrapper)

lifecycleOwner.lifecycle.removeObserver(wrapper)

wrapper.onDestroy(lifecycleOwner)

… to session starting

gunt0001 · 2025-01-24T15:38:35Z

toolkit/ar/src/main/java/com/arcgismaps/toolkit/ar/internal/ArHelpers.kt

+    }
+
+    return allPermissionsGranted
+}


missing newline

gunt0001 · 2025-01-24T16:53:12Z

toolkit/ar/src/main/java/com/arcgismaps/toolkit/ar/TableTopSceneView.kt

-import android.content.Context
-import android.content.pm.PackageManager
-import androidx.activity.compose.rememberLauncherForActivityResult
-import androidx.activity.result.contract.ActivityResultContracts
 import androidx.compose.foundation.layout.Box
 import androidx.compose.foundation.layout.fillMaxSize
 import androidx.compose.runtime.Composable
 import androidx.compose.runtime.DisposableEffect


Unused import

gunt0001 · 2025-01-24T16:55:08Z

toolkit/ar/src/main/java/com/arcgismaps/toolkit/ar/WorldScaleSceneView.kt

 import com.arcgismaps.geometry.SpatialReference
+import com.arcgismaps.location.Location
 import com.arcgismaps.location.LocationDataSource


Unused import

gunt0001 · 2025-01-24T16:58:39Z

toolkit/ar/src/main/java/com/arcgismaps/toolkit/ar/WorldScaleSceneView.kt

+}
+
+/**
+ * Starts the [locationDataSource] and periodically updates the [cameraController] with new locations


locationDataSource parameter has been removed...

Suggested change

* Starts the [locationDataSource] and periodically updates the [cameraController] with new locations

* Starts a SystemLocationDataSource and periodically updates the [cameraController] with new locations

gunt0001 · 2025-01-24T17:01:04Z

toolkit/ar/src/main/java/com/arcgismaps/toolkit/ar/WorldScaleSceneView.kt

+    val lifecycleOwner = LocalLifecycleOwner.current
+    DisposableEffect(Unit) {
+        // This will start the LocationDataSource
+        val wrapper = LocationDataSourceWrapper(locationDataSource)


This LocationDataSourceWrapper needs to be kept in a remember, otherwise it could be garbage collected. See related comment below on rememberSystemLocationDataSource.

gunt0001 · 2025-01-24T17:02:24Z

toolkit/ar/src/main/java/com/arcgismaps/toolkit/ar/WorldScaleSceneView.kt

+}
+
+@Composable
+private fun rememberSystemLocationDataSource(): SystemLocationDataSource {


You could have this return the LocationDataSourceWrapper and also do the DisposableEffect in this remember function.

gunt0001

@hud10837 - looks good. There are some refinements we can do regarding initialization. For example I noticed that the SceneView may render the globe at full extent (as if looking at it from space) on startup, before we have the initial location. This is something we should be able to avoid. But we can refine that in a separate PR.

toolkit/ar/src/main/java/com/arcgismaps/toolkit/ar/WorldScaleSceneView.kt

…ceneView.kt Co-authored-by: Gunther Heppner <[email protected]>

hud10837 added 12 commits January 14, 2025 10:02

add tracking modes

067a58a

update WorldScaleSceneView to use LocationDataSource from TrackingMode

9a2ea2c

add todos

96c49ac

rm tracking mode

bac047a

make initialization of LDS and Camera Feed sequential

2190ccc

add permission state backup

252338c

fix permission state

1395f78

add doc

12c9a13

fix usage of PermissionState in WSSV

717b0d9

remove permissionstate from wssv

d997122

handle automatic permission request for wssv

c99b940

revert tabletop code

c06edcb

hud10837 changed the title ~~Hud10837/implement world tracking~~ WorldScaleSceneView: Handle permissions Jan 21, 2025

filter locations for world tracking

7c42932

hud10837 changed the title ~~WorldScaleSceneView: Handle permissions~~ WorldScaleSceneView: Handle permissions and implement world tracking Jan 21, 2025

hud10837 added 4 commits January 21, 2025 11:12

use nan for accuracy filtering and test

b1ecc58

eof newline

80d8e71

add doc to TrackLocations fun and rm initialHeading

0721ff1

add doc to lds wrapper and copyrights

97258fd

hud10837 commented Jan 21, 2025

View reviewed changes

hud10837 added 2 commits January 21, 2025 11:22

Merge branch 'feature-branches/world-scale-scene-view' of https://git…

5ed0dc6

…hub.com/Esri/arcgis-maps-sdk-kotlin-toolkit into hud10837/implement-world-tracking

update api

79ccd2c

hud10837 marked this pull request as ready for review January 21, 2025 09:24

use correct lifecycle

75a423a

01smito01 self-requested a review January 21, 2025 12:27

01smito01 self-assigned this Jan 21, 2025

01smito01 approved these changes Jan 21, 2025

View reviewed changes

hud10837 added 2 commits January 22, 2025 10:41

fix formatting

e7f68b1

use dleegated state

a6708dd

gunt0001 reviewed Jan 22, 2025

View reviewed changes

hud10837 added 8 commits January 23, 2025 09:31

use truth asserts

d5ef304

rename TrackLocationWithLocationDataSource -> LocationTracker

57beacc

centralize default values

b21ab92

hasSetOriginCamera and hasLaunchedRequest to non State

55851f9

rework permission request code to return state of permissions

d2f1dcb

start lds in onresume

00a923c

convert rememberPermissionsGranted to return State

3224082

rm requestPermissionsAutomatically params

12e83bb

gunt0001 reviewed Jan 23, 2025

View reviewed changes

hud10837 added 8 commits January 23, 2025 15:50

update comments on LocationDataSourceWrapper

00093f3

update permission doc

7eaccd8

reorder onDispose

5c56467

simplify flow of code with early exits and make lds starting parallel…

a57a634

… to session starting

subscribe to lifecycle for ArSessionWrapper inside remember function

472bd42

move ArCore installed check to function

af811ed

mv funs to ArHelpers

4d0ec01

mv lds to LocationTracker

9bba4d9

gunt0001 reviewed Jan 24, 2025

View reviewed changes

hud10837 added 3 commits January 27, 2025 10:28

format

70c3bb1

refactor LDS wrapper creation

57e3bce

refactor LDS wrapper creation

b960c01

hud10837 requested a review from gunt0001 January 27, 2025 08:43

gunt0001 approved these changes Jan 27, 2025

View reviewed changes

toolkit/ar/src/main/java/com/arcgismaps/toolkit/ar/WorldScaleSceneView.kt Outdated Show resolved Hide resolved

Update toolkit/ar/src/main/java/com/arcgismaps/toolkit/ar/WorldScaleS…

04a4af7

…ceneView.kt Co-authored-by: Gunther Heppner <[email protected]>

hud10837 merged commit 0f6aa1d into feature-branches/world-scale-scene-view Jan 27, 2025

hud10837 deleted the hud10837/implement-world-tracking branch January 27, 2025 14:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WorldScaleSceneView: Handle permissions and implement world tracking #709

WorldScaleSceneView: Handle permissions and implement world tracking #709

hud10837 commented Jan 20, 2025 •

edited

Loading

hud10837 Jan 21, 2025

hud10837 Jan 21, 2025

gunt0001 Jan 22, 2025

hud10837 Jan 23, 2025

hud10837 Jan 21, 2025

hud10837 Jan 21, 2025

01smito01 left a comment

01smito01 Jan 21, 2025

hud10837 Jan 22, 2025

gunt0001 Jan 22, 2025

gunt0001 Jan 22, 2025

gunt0001 Jan 22, 2025

This comment was marked as resolved.

gunt0001 Jan 22, 2025

gunt0001 Jan 22, 2025

gunt0001 Jan 22, 2025

hud10837 Jan 23, 2025

gunt0001 Jan 22, 2025

gunt0001 Jan 22, 2025

hud10837 Jan 23, 2025

gunt0001 Jan 22, 2025

gunt0001 Jan 23, 2025

gunt0001 Jan 23, 2025

gunt0001 Jan 23, 2025

gunt0001 Jan 23, 2025

gunt0001 Jan 23, 2025

gunt0001 Jan 23, 2025

gunt0001 Jan 24, 2025

gunt0001 Jan 24, 2025

gunt0001 Jan 24, 2025

gunt0001 Jan 24, 2025

gunt0001 Jan 24, 2025

gunt0001 Jan 24, 2025

gunt0001 left a comment

	private fun TrackLocationWithLocationDataSource(
	private fun LocationTracker(

	if (location.position.hasZ) location.position.z!! else 1.0,
	if (location.position.hasZ) location.position.z!! else 0.0,

	private fun requestPermissionsOrFail(
	private fun rememberPermissionsGranted(

	fun startLocationDataSource() {
	override fun onResume(owner: LifecycleOwner) {

	// This ensures that we can stop the location data source on a scope that won't be cancelled when the composition is destroyed
	// coroutine scope that is tied to the lifecycle of this LocationDataSourceWrapper

		wrapper.onDestroy(lifecycleOwner)
		lifecycleOwner.lifecycle.removeObserver(wrapper)

	* Starts the [locationDataSource] and periodically updates the [cameraController] with new locations
	* Starts a SystemLocationDataSource and periodically updates the [cameraController] with new locations

WorldScaleSceneView: Handle permissions and implement world tracking #709

WorldScaleSceneView: Handle permissions and implement world tracking #709

Conversation

hud10837 commented Jan 20, 2025 • edited Loading

Description:

Summary of changes:

Pre-merge Checklist

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

01smito01 left a 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

This comment was marked as resolved.

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

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

gunt0001 left a comment

Choose a reason for hiding this comment

hud10837 commented Jan 20, 2025 •

edited

Loading