Updating environment, additional dependencies, formatting, and optimizing imports#33
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 removes the kotlinx-coroutines-guava dependency, updates the material library version, and introduces skybox support in the EnvironmentController using ExrImage. Feedback was provided regarding the lifecycle management of pbrMaterial to ensure it is correctly tied to the xrSession, the performance impact of not caching ExrImage assets, and the implementation of safer casting when retrieving models from the asset cache to prevent potential exceptions.
| // Create and apply a custom PBR material to the model when the XR session or target node changes. | ||
| LaunchedEffect(xrSession, bugdroidNode) { | ||
| val material = KhronosPbrMaterial.create( | ||
| val material = pbrMaterial ?: KhronosPbrMaterial.create( |
There was a problem hiding this comment.
The pbrMaterial is cached in a state that is not keyed to the xrSession. If the xrSession changes, this LaunchedEffect will restart and reuse the pbrMaterial from the previous session. Since KhronosPbrMaterial objects are tied to a specific session, reusing them across sessions is invalid and can lead to crashes or rendering issues. Ensure the pbrMaterial state is reset when the session changes (e.g., by using remember(xrSession) for its definition).
| val lightingForSkybox = ExrImage.createFromZip( | ||
| xrSession, | ||
| Paths.get("environments/green_hills_ibl.zip") | ||
| ) |
There was a problem hiding this comment.
The ExrImage is recreated from the zip file every time the environment is changed. Unlike the GltfModel, this asset is not cached, which can lead to unnecessary I/O and processing overhead (unzipping and decoding) when switching between environments. Consider caching the ExrImage to improve performance.
| xrSession, | ||
| Paths.get("environments/green_hills_ibl.zip") | ||
| ) | ||
| val environmentModel = assetCache[environmentModelName] as GltfModel |
There was a problem hiding this comment.
This forced cast to GltfModel will throw a NullPointerException if the asset has not been loaded into the assetCache yet (e.g., if the user triggers the environment change before the asynchronous loadModelAsset completes). While this is caught by the surrounding try-catch block, relying on exceptions for expected control flow is suboptimal. Consider using a safe cast and handling the missing asset explicitly.
| val environmentModel = assetCache[environmentModelName] as GltfModel | |
| val environmentModel = assetCache[environmentModelName] as? GltfModel ?: return@launch |
1 participant
Updating environment, additional dependencies, formatting, and optimizing imports