-
Notifications
You must be signed in to change notification settings - Fork 4
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
Tabletop AR: scene placement #602
base: feature-branches/tabletop-ar
Are you sure you want to change the base?
Conversation
* `Forms`: Add `TextFormElement` (#542) * `Forms`: Add `TextFormElement` tests (#551) * add tests * updated feature form doc * bump sdk version * `Forms` : Add SubTypeFeatureLayer support (#559) * `Forms`: Fix stale `LaunchedEffect`s (#563) * fix stale launched effects * use rememberupdatedstate * update feature form doc (#565)
* prototype design options * prototype tabletopSceneView * remove unused implementations * create readme * add tabletopsceneviewproxy * update microapp to use tabletop proxy * add TableTopSceneViewScope * apply Compose gradle plugin * fix warnings * rename microapp * delete tests for microapp * add copyright * fix since years * use swift doc on tabletopsceneview * add proxy doc * add scope doc * fix references in proxy doc * fix references in scope doc * fix references in scope doc * newlines * rm unit test * fix doc and imports * fix doc and imports * revert authentication changes --------- Co-authored-by: Gunther Heppner <[email protected]>
* import necessary files for rendering camera feed * copyright * rm obj dependency * make kotlin classes internal * make java classes non-public * centralize logging and tag * add doc links to hello ar * make planerenderer companion internal * Fix texture name * add newline
* bring in existing implementation of ArSurfaceView * bring in changes to TabletopSceneView and TableTopSceneViewState * pare down changes to just implement ARSurfaceView and add lifecycle management wrappers for AR Session and GlSurfaceView * rm camera controller line * mv call to box * mv ArSessionWrapper to internal * fix since tags * rename ARSurfaceView -> ArCameraFeed * rename localLifecycleOwner -> lifecycleOwner * mv initialization of sceneViewProxy to TableTopSceneViewProxy
* mv assets folder to correct location * out of the box permissions request * rename camera permission function * make request permission optional * add initialization status * add availability check * update microapp * extract string resources * rm unnecessary changes * don't use stateflow for microapp * rename string res * rename string resource * don't require camera permission before checking arcore visibility * mv box call * add local function for update status * simplify availability check * rename status * make status constructors internal * use updateState for initial status * mv status doc to member objects * add remaining param doc * fix remaining param doc * add parameter for callback to lambda * revert whitespace in manifest * remove debug delay * add factory rememberTableTopSceneViewStatus * add extension fun to mutable state * add extension fun to mutable state * rename camera feed file * use launchedeffect to ensure initializing status is only sent once * Don't send callback on first status * use side effects for callback * use collectAsStateWithLifecycle * add doc to rememberTableTopSceneViewStatus()
…i/arcgis-maps-sdk-kotlin-toolkit into hud10837/scene-placement
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hud10837 - looks good, some minor comments.
@@ -56,6 +81,9 @@ fun MainScreen() { | |||
Box(modifier = Modifier.fillMaxSize()) { | |||
TableTopSceneView( | |||
arcGISScene = arcGISScene, | |||
arcGISSceneAnchor = Point(-74.0, 40.72, 0.0, SpatialReference.wgs84()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better initialize this with the spatial reference of the ArcGISScene:
arcGISSceneAnchor = Point(-74.0, 40.72, 0.0, SpatialReference.wgs84()), | |
arcGISSceneAnchor = Point(-74.0, 40.72, 0.0, arcGISScene.spatialReference), |
} | ||
} | ||
} | ||
initializationStatus.let { status -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the let
necessary here? Could be...
initializationStatus.let { status -> | |
when (initializationStatus) { | |
... |
) { | ||
CircularProgressIndicator() | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could handle the FailedToLoad case as well here:
} | |
} else if (sceneLoadStatus is LoadStatus.FailedToLoad) { | |
// Tell the user that the ArcGISScene failed to load | |
TextWithScrim(text = stringResource(R.string.arcgisscene_failed)) | |
} |
Note, a when statement would be more appropriate.
this.clippingDistance = clippingDistance | ||
} | ||
} | ||
var arCoreAnchor: Anchor? by remember { mutableStateOf(null) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find it more readable if cameraController
and arCoreAnchor
were defined outside of the Box scope. The Box scope is really just for arranging the ArCameraFeed
and the SceneView
on top of it.
@@ -317,3 +387,16 @@ private fun MutableState<TableTopSceneViewStatus>.update( | |||
this.value = newStatus | |||
callback?.invoke(newStatus) | |||
} | |||
|
|||
private val Pose.transformationMatrix: TransformationMatrix |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing doc
Related to issue: https://devtopia.esri.com/runtime/kotlin/issues/4585
Description:
Implements behavior to place the scene when the user taps a plane on the camera feed
Summary of changes:
arcGISAnchorPoint
,translationFactor
, andclippingDistance
parametersTableTopSceneView
to respond to the first detected plane (to update the status), to respond to the first user tap (to place the scene), and to render the scene manually while updating the camera positionDetectingPlanes
status toTableTopSceneViewStatus
ArCameraFeed
andCameraFeedRenderer
with callbacks for the new behavior ofTableTopSceneView
Pre-merge Checklist