-
Notifications
You must be signed in to change notification settings - Fork 59
Display a glTF of a bugdroid #11
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
Conversation
fuentespatrick
commented
May 20, 2025
- Display a glTF of a bugdroid
- Make the panels moveable and resizable
- Update the readme
- Update dependencies
- Add a comment to the manifest to reflect what the spatial feature entry is for
- Make the panels moveable and resizable - Update the readme - Update dependencies - Add a comment to the manifest to reflect what the spatial feature entry is for
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.
Hello @fuentespatrick, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
Hello there! Gemini here, providing a summary of this pull request. This PR, authored by fuentespatrick, introduces the capability to display a glTF model within the application, specifically a bugdroid model. It also enhances the user interface by making the existing spatial panels movable and resizable. Alongside these core feature additions, the PR includes necessary updates to project dependencies and documentation to reflect the changes and ensure compatibility with the latest development environment recommendations.
Highlights
- Display glTF Model: Adds functionality to load and display a glTF 3D model (a bugdroid) using the
androidx.xr.scenecorelibrary within a spatial Compose UI. - Movable and Resizable Panels: Applies
.movable()and.resizable()modifiers to theSpatialPanelcomponents, allowing users to interactively move and resize the UI panels in the spatial environment. - Dependency Updates: Updates various project dependencies, including Compose BOM, AGP, Kotlin, and other androidx libraries, to newer versions.
- Documentation and Manifest Updates: Updates the README with instructions for the required development environment (Canary build) and adds a clarifying comment to the Android Manifest regarding the spatial API feature declaration.
Changelog
Click here to see the changelog
- README.md
- Updated development environment instructions to recommend using the latest Canary version of Android Studio and updating the XR emulator image.
- app/src/main/AndroidManifest.xml
- Added a comment to the
uses-feature android:name="android.software.xr.api.spatial"tag to clarify its purpose and non-required status.
- Added a comment to the
- app/src/main/java/com/example/helloandroidxr/ui/HelloAndroidXRApp.kt
- Imported necessary Compose and XR Compose layout components for Box, Button, Text, state management, movable, and resizable modifiers.
- Applied
.movable()and.resizable()modifiers to theSpatialPanelcomposables within theSpatialLayoutfunction. - Refactored the
PrimaryContentcomposable to conditionally display either the original text pane or a button that triggers the display of the Bugdroid model, depending on whether spatial UI is enabled. - Removed an unnecessary
Rowwrapper aroundprimaryContent. - Minor formatting adjustments.
- app/src/main/java/com/example/helloandroidxr/ui/components/BugdroidModel.kt
- Added a new file containing the
BugdroidModelcomposable function. - This composable loads the
bugdroid_animated_wave.gltfasset usingGltfModel.createandGltfModelEntity.createfromandroidx.xr.scenecore. - It places the model within a
SubspaceandVolumewith a relative offset. - The model's scale is reduced, and a specific animation is started on loop.
- The model is only displayed when the
showBugdroidparameter is true.
- Added a new file containing the
- app/src/main/res/values/strings.xml
- Added a new string resource
show_bugdroidfor the button text.
- Added a new string resource
- gradle/libs.versions.toml
- Updated versions for
androidx-runtime,agp,kotlinxCoroutinesGuava,kotlin,activityCompose,composeBom,screenshot, andadaptiveAndroid. - Added
kotlinAndroidversion reference.
- Updated versions for
- gradle/wrapper/gradle-wrapper.properties
- Updated the Gradle distribution URL to version 8.11.1.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
A bugdroid takes the stage,
In spatial space, it's the rage.
Move panels with ease,
Resize as you please,
Turning a new digital page.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
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.
Code Review
This pull request introduces some exciting new features, including the display of a 3D Bugdroid model using glTF and making UI panels movable and resizable. The updates to the README for development setup and dependency versions are also welcome. The code is generally well-structured.
I've identified a couple of high-severity issues that should be addressed before merging, primarily related to resource handling in BugdroidModel.kt and modifier usage in PrimaryContent. Addressing these will improve the robustness and correctness of the application.
No specific style guide was provided, so my review is based on common Android Kotlin best practices and the Google Java Style Guide principles where applicable.
Summary of Findings
- Modifier Reuse in PrimaryContent: The
modifierparameter inPrimaryContent(app/src/main/java/com/example/helloandroidxr/ui/HelloAndroidXRApp.kt) is applied to multiple nested Composables. This can lead to incorrect layouts and modifier conflicts. Modifiers should generally be applied to the outermost relevant Composable or new Modifier instances should be used for children. - InputStream Handling and Error Management in BugdroidModel: In
BugdroidModel.kt, theInputStreamfor the glTF model might be read after being closed on subsequent recompositions, leading to a crash. Additionally, potential errors during model loading are not handled. UsingLaunchedEffectwith proper stream handling and try-catch blocks is recommended.
Merge Readiness
The pull request introduces valuable features and updates. However, there are two high-severity issues identified related to modifier usage and resource handling/error management that could lead to runtime crashes or incorrect behavior. I recommend addressing these issues before merging the pull request to ensure code quality and stability. I am not authorized to approve pull requests, so please ensure these changes are reviewed and approved by a maintainer.
| Surface(modifier.fillMaxSize()) { | ||
| Box(modifier.padding(48.dp), contentAlignment = Alignment.Center) { | ||
| Button( | ||
| onClick = { | ||
| showBugdroid = true | ||
| }, | ||
| modifier = modifier |
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.
The modifier parameter passed to PrimaryContent is being reused for multiple Composables (Surface, Box, and Button). This is generally not recommended as a Modifier instance should ideally be applied to a single UI element in a composition branch. Reusing it can lead to unexpected layout behaviors or conflicts, especially if the passed-in modifier has properties like size, padding, or click handlers that are not intended for all nested elements.
Could you refactor this to apply the incoming modifier to the root element within PrimaryContent (e.g., the Surface or TextPane) and use new Modifier instances for specific styling of internal elements like the Box or Button?
| Surface(modifier.fillMaxSize()) { | |
| Box(modifier.padding(48.dp), contentAlignment = Alignment.Center) { | |
| Button( | |
| onClick = { | |
| showBugdroid = true | |
| }, | |
| modifier = modifier | |
| Surface(modifier = modifier.fillMaxSize()) { // Apply the incoming 'modifier' to the root Surface | |
| Box( | |
| // Use a new Modifier for Box's specific padding, not the one passed to PrimaryContent | |
| modifier = Modifier.padding(48.dp), | |
| contentAlignment = Alignment.Center | |
| ) { | |
| Button( | |
| onClick = { | |
| showBugdroid = true | |
| } | |
| // Removed 'modifier = modifier'. Add specific Button modifiers if necessary. | |
| ) { |
| val inputStream: InputStream = | ||
| context.resources.openRawResource(R.raw.bugdroid_animated_wave) | ||
| Volume( | ||
| SubspaceModifier.offset(z = 400.dp) // Relative position | ||
| ) { parent -> | ||
| scope.launch { | ||
| val gltfModel = GltfModel.create( | ||
| session = xrSession, | ||
| assetData = inputStream.readBytes(), | ||
| assetKey = "BUGDROID" | ||
| ).await() | ||
| val gltfEntity = GltfModelEntity.create(xrSession, gltfModel) | ||
| // Make this glTF a child of the Volume | ||
| gltfEntity.setParent(parent) | ||
| // Change the size of the large glTF to 10% | ||
| gltfEntity.setScale(0.1f) | ||
| gltfEntity.startAnimation( | ||
| loop = true, | ||
| animationName = "Armature|Take 001|BaseLayer" | ||
| ) | ||
| } |
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.
There are a couple of concerns with the model loading logic:
- InputStream Handling: The
inputStreamis obtained at line 43. If thescope.launchblock (lines 48-63) is executed multiple times due to recompositions of theVolume's content lambda,inputStream.readBytes()would be called on the same stream instance. SincereadBytes()closes the stream after reading, subsequent calls on the closed stream will fail, likely causing a crash. - Error Handling: The model loading process (
GltfModel.create(...).await()) can throw exceptions (e.g., file not found, network issues, parsing errors). These are not currently caught, which could lead to unhandled exceptions. - Execution Context: Launching the coroutine directly within the
Volume's content lambda means it could re-execute more often than necessary. WhileGltfModel.createcaches the model data byassetKey, the entity creation and parenting logic would still run.
Could you consider refactoring this to use LaunchedEffect? This would provide better control over when the loading logic executes (e.g., when xrSession or the parent entity from Volume changes), allow for safer InputStream handling by opening it inside the effect, and provide a natural place for try-catch error handling.
// It's generally safer to open and use the InputStream within the scope where it's consumed,
// especially with effects, to ensure it's fresh if the effect re-runs.
Volume(
SubspaceModifier.offset(z = 400.dp) // Relative position
) { parentNode -> // Renamed from 'parent' for clarity
LaunchedEffect(xrSession, parentNode, context) { // Add keys that define when this effect should run
val modelInputStream: InputStream = try {
context.resources.openRawResource(R.raw.bugdroid_animated_wave)
} catch (e: Exception) {
// Log.e("BugdroidModel", "Failed to open resource stream", e)
return@LaunchedEffect // Stop if stream can't be opened
}
try {
val gltfModel = GltfModel.create(
session = xrSession,
assetData = modelInputStream.readBytes(), // readBytes() closes the stream
assetKey = "BUGDROID"
).await()
val gltfEntity = GltfModelEntity.create(xrSession, gltfModel)
// Make this glTF a child of the Volume
gltfEntity.setParent(parentNode)
// Change the size of the large glTF to 10%
gltfEntity.setScale(0.1f)
gltfEntity.startAnimation(
loop = true,
animationName = "Armature|Take 001|BaseLayer"
)
} catch (e: Exception) {
// Log.e("BugdroidModel", "Error loading or displaying glTF model", e)
// Handle or log the exception appropriately
}
}
}