Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the Android XR samples to align with recent API changes, including renaming 'DepthMap' to 'Depth' and 'ExrImage' to 'ImageBasedLightingAsset', and transitioning to 'SPATIAL' tracking modes. It also introduces a comprehensive set of Glimmer component samples and replaces the '.movable()' modifier with '.transformingMovable()'. Feedback identifies several issues in the 'SpatialGltfModel' sample, such as a potential null pointer exception, redundant imports, and conflicting 'LaunchedEffect' blocks that overwrite material overrides. Additionally, the reviewer noted a performance regression where material caching was removed and a duplicated documentation tag in the focus samples.
| node?.localPose = | ||
| Pose(node.localPose.translation, rotation) |
There was a problem hiding this comment.
Accessing node.localPose directly is unsafe because node is nullable (returned by find). This will cause a NullPointerException if the node is not found. Use a null-safe block to perform the update.
node?.let {
it.localPose = Pose(it.localPose.translation, rotation)
}| import androidx.compose.runtime.getValue | ||
| import androidx.compose.runtime.mutableStateOf | ||
| import androidx.compose.runtime.remember | ||
| import androidx.compose.runtime.setValue |
There was a problem hiding this comment.
These imports are redundant as they are already present in the file (lines 27-30). Please remove the duplicates to keep the import section clean.
| // Create and apply the custom material once the session is ready and the target node is available. | ||
| LaunchedEffect(node) { | ||
| val material = pbrMaterial ?: KhronosPbrMaterial.create( | ||
| val material = KhronosPbrMaterial.create( |
There was a problem hiding this comment.
The material is being re-created every time this LaunchedEffect runs, bypassing the pbrMaterial cache defined at line 78. This is less efficient than the previous implementation. Consider restoring the caching logic.
| val material = KhronosPbrMaterial.create( | |
| val material = pbrMaterial ?: KhronosPbrMaterial.create( |
| } | ||
|
|
||
| // [START androidxr_compose_SpatialGltfModelTexture] | ||
| LaunchedEffect(node) { |
There was a problem hiding this comment.
This LaunchedEffect conflicts with the one starting at line 81. Both effects use node as a key and call setMaterialOverride. Since they run concurrently, the second one will overwrite the first, making the base color factor set in the first block ineffective. Consider merging these snippets or clarifying their usage.
|
|
||
| // [START xr_glimmer_focus_activity] | ||
|
|
||
| // [START xr_glimmer_focus_activity] |
There was a problem hiding this comment.
The snippet tag [START xr_glimmer_focus_activity] is duplicated (see line 32). Please remove the redundant tag to ensure correct documentation generation.
2 participants
No description provided.