Bevy Rewrite, attempt 2 #46
Reference in New Issue
Block a user
Delete Branch "bevy_for_real_this_time"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
This will not have a working wayland impl for now, thats the next goal
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores
📝 Walkthrough
Walkthrough
This update transitions the codebase from a custom StereoKit-based rendering and input loop to a Bevy ECS-driven architecture with deep OpenXR integration. Major subsystems—including audio, 3D models, lines, and text rendering—are refactored to leverage Bevy's plugins, assets, and ECS. Input handling for VR controllers and hands now uses Bevy and OpenXR, with new plugins and systems for lifecycle and state management. The play space and spatial node management are also reworked for Bevy compatibility. Supporting infrastructure such as async channels, color conversion, and entity lifecycle management are introduced to facilitate this migration. Cargo configuration and dependencies are updated to reflect these architectural changes.
Changes
BevyChannelandBevyChannelReader.save_state; used Rust named format arguments.ColorConverttrait and implementation for color conversion to Bevy'sColor.EntityHandlePlugin,EntityHandlewrapper, and channel-based entity destruction.color,entity_handle,bevy_channel.stereokit_loopwithbevy_loop; refactored main control flow to Bevy ECS and OpenXR integration.AudioNodePlugin, changedSoundto use entity handles.LinesNodePlugin, mesh generation, and entity management.ModelNodePlugin, async loading, new material management.bevy_mesh_text_3d; addedTextNodePlugin, event-driven spawning.shadersmodule.CameraItem.Aabband transform types; addedSpatialNodePluginand ECS transform sync system.ControllerPlugin, ECS systems, and new action/state handling.HandPlugin, ECS systems, new OpenXR joint/material handling.ServerObjectsstruct and related input management; removed stereokit imports; addedRwLockimport.PlaySpacePluginfor ECS-managed play space; refactored D-Bus bounds to useRwLockand ECS resources.StereoKitErrorvariant fromServerErrorenum.ItemTypeenum.PanelItem::createmethod.updatemethod fromEyePointer.FlatscreenInputPluginintegrating mouse/keyboard input with Bevy ECS; refactored input update to use Bevy input events; updated key mapping.Sequence Diagram(s)
Main Application Flow (Old vs. New)
Old: StereoKit Loop
New: Bevy/OpenXR Loop
Audio Playback (New Flow)
Model Loading and Material Application (New Flow)
XR Controller Update (New Flow)
Text Spawning (New Flow)
Play Space Update and D-Bus Exposure (New Flow)
📜 Recent review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between
9347072249andaec875a1bf.⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock📒 Files selected for processing (19)
Cargo.toml(4 hunks)src/core/client.rs(2 hunks)src/core/error.rs(0 hunks)src/main.rs(4 hunks)src/nodes/drawable/lines.rs(2 hunks)src/nodes/drawable/mod.rs(1 hunks)src/nodes/drawable/model.rs(4 hunks)src/nodes/drawable/text.rs(2 hunks)src/nodes/input/mod.rs(2 hunks)src/nodes/items/camera.rs(2 hunks)src/nodes/items/mod.rs(1 hunks)src/nodes/items/panel.rs(1 hunks)src/nodes/spatial/mod.rs(5 hunks)src/objects/input/eye_pointer.rs(0 hunks)src/objects/input/mouse_pointer.rs(8 hunks)src/objects/input/sk_controller.rs(2 hunks)src/objects/input/sk_hand.rs(2 hunks)src/objects/mod.rs(1 hunks)src/objects/play_space.rs(2 hunks)✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
❤️ Share
🪧 Tips
Chat
There are 3 ways to chat with CodeRabbit:
I pushed a fix in commit <commit_id>, please review it.Explain this complex logic.Open a follow-up GitHub issue for this discussion.@coderabbitaiin a new review comment at the desired location with your query. Examples:@coderabbitai explain this code block.@coderabbitai modularize this function.@coderabbitaiin a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:@coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.@coderabbitai read src/utils.ts and explain its main purpose.@coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.@coderabbitai help me debug CodeRabbit configuration file.Support
Need help? Create a ticket on our support page for assistance with any issues or questions.
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.
CodeRabbit Commands (Invoked using PR comments)
@coderabbitai pauseto pause the reviews on a PR.@coderabbitai resumeto resume the paused reviews.@coderabbitai reviewto trigger an incremental review. This is useful when automatic reviews are disabled for the repository.@coderabbitai full reviewto do a full review from scratch and review all the files again.@coderabbitai summaryto regenerate the summary of the PR.@coderabbitai generate docstringsto generate docstrings for this PR.@coderabbitai generate sequence diagramto generate a sequence diagram of the changes in this PR.@coderabbitai resolveresolve all the CodeRabbit review comments.@coderabbitai configurationto show the current CodeRabbit configuration for the repository.@coderabbitai helpto get help.Other keywords and placeholders
@coderabbitai ignoreanywhere in the PR description to prevent this PR from being reviewed.@coderabbitai summaryto generate the high-level summary at a specific location in the PR description.@coderabbitaianywhere in the PR title to generate the title automatically.CodeRabbit Configuration File (
.coderabbit.yaml).coderabbit.yamlfile to the root of your repository.# yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.jsonDocumentation and Community
@coderabbitai review
✅ Actions performed
Review triggered.
Actionable comments posted: 21
🧹 Nitpick comments (15)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between
fd1c6ed0cfand9347072249.⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock📒 Files selected for processing (19)
Cargo.toml(4 hunks)src/core/bevy_channel.rs(1 hunks)src/core/client.rs(2 hunks)src/core/color.rs(1 hunks)src/core/entity_handle.rs(1 hunks)src/core/mod.rs(1 hunks)src/main.rs(6 hunks)src/nodes/audio.rs(2 hunks)src/nodes/drawable/lines.rs(2 hunks)src/nodes/drawable/mod.rs(0 hunks)src/nodes/drawable/model.rs(4 hunks)src/nodes/drawable/text.rs(2 hunks)src/nodes/input/mod.rs(4 hunks)src/nodes/items/camera.rs(2 hunks)src/nodes/spatial/mod.rs(6 hunks)src/objects/input/sk_controller.rs(2 hunks)src/objects/input/sk_hand.rs(3 hunks)src/objects/mod.rs(6 hunks)src/objects/play_space.rs(2 hunks)💤 Files with no reviewable changes (1)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/core/entity_handle.rs (1)
src/nodes/drawable/model.rs (12)
🔇 Additional comments (20)
@@ -23,3 +23,3 @@[features]default = ["wayland"]default = []wayland = ["dep:smithay", "dep:wayland-scanner", "dep:wayland-backend"]⚠️ Potential issue
Breaking change: Wayland no longer in default features.
Removing
waylandfrom default features might break existing users. Consider documenting this change prominently or keeping it as default if it's commonly used.🤖 Prompt for AI Agents
@@ -73,3 +67,4 @@tracing = { version = "0.1.40", features = ["release_max_level_warn"] }tracing-subscriber = { version = "0.3.18", features = ["env-filter"] }tracing-tracy = { version = "0.11.1", optional = true }🛠️ Refactor suggestion
Pin git dependencies to specific commits for reproducible builds.
Using branch names without commit hashes can lead to non-reproducible builds as branches can be updated.
Add specific commit hashes or tags to ensure reproducible builds:
🤖 Prompt for AI Agents
@@ -83,6 +78,17 @@ glam = { version = "0.29.0", features = ["mint", "serde"] }mint = "0.5.9"⚠️ Potential issue
Local path dependency will cause issues for other developers.
The local path dependency
bevy_sk.path = "../bevy_sk"won't work for other developers or in CI/CD.Consider using:
🤖 Prompt for AI Agents
@@ -0,0 +14,4 @@let (tx, rx) = mpsc::unbounded_channel();self.0.set(tx).unwrap();app.insert_resource(BevyChannelReader(rx));}🛠️ Refactor suggestion
Handle multiple initialization attempts gracefully.
The
unwrap()will panic ifinitis called multiple times. Consider handling this case explicitly.📝 Committable suggestion
🤖 Prompt for AI Agents
@@ -0,0 +27,4 @@Err(TryRecvError::Disconnected) => panic!("bevy channel should never disconnect"),Err(TryRecvError::Empty) => None,}}🛠️ Refactor suggestion
Consider graceful handling of disconnected channels.
Panicking on channel disconnection might be too aggressive, especially during shutdown scenarios.
📝 Committable suggestion
🤖 Prompt for AI Agents
🛠️ Refactor suggestion
Replace
println!with structured logging.Use proper tracing macros instead of
println!for consistency with the rest of the codebase's logging approach.Apply this diff to use proper structured logging:
🤖 Prompt for AI Agents
@@ -226,0 +257,4 @@let mut app = App::new();app.insert_resource(DbusConnection(dbus_connection));app.add_plugins(AssetPlugin {meta_check: AssetMetaCheck::Never,⚠️ Potential issue
Fix undefined
waylandvariable in xr_step function.The
waylandvariable is referenced but not defined in this function scope. This will cause compilation errors when the wayland feature is enabled.The wayland instance needs to be made accessible to the xr_step function. Consider these options:
🤖 Prompt for AI Agents
✅ Addressed in commits
d2afa00toaec875a💡 Verification agent
🧩 Analysis chain
Verify the extremely small near plane value.
The near plane is set to 0.003 meters (3mm), which is unusually small and may cause z-fighting or depth precision issues.
Consider using a more typical value like 0.01 or 0.05 meters for VR applications. The small value might be intentional for specific use cases, but it's worth verifying.
🌐 Web query:
💡 Result:
In VR/XR applications, the recommended near plane distance is typically set to a value that balances depth precision and minimizes z-fighting.
A common guideline is to set the near plane distance to around 0.1 meters (10 centimeters) or slightly above. This is because:
For VR, which often requires accurate depth perception at close range, 0.1 meters is a good starting point. Some developers go as close as 0.05 meters if their engine and hardware support higher precision depth buffers (like using 24 or 32-bit depth buffers), but going below that usually causes depth fighting.
To sum up:
If you want, I can also look up the latest best practices or guidelines from specific VR platforms or engines. Would you like me to do that?
Re-evaluate the near‐plane setting (0.003 m)
A 3 mm near plane is well below common VR/XR guidelines (typically ≥ 0.05–0.1 m) and can severely degrade depth‐buffer precision, leading to z-fighting. Please confirm whether such an ultra-close near plane is required for your use case, or consider increasing it to a more typical value (e.g., 0.05–0.1 m).
• Location:
src/main.rs, lines 415–431• All three branches set
near = 0.003(Perspective, Orthographic, andXrProjection)Suggested change example:
📝 Committable suggestion
🤖 Prompt for AI Agents
✅ Addressed in commits
d2afa00toaec875a⚠️ Potential issue
Address the memory leak in tokio runtime context handling.
The current implementation leaks memory by forgetting the enter guard. Consider storing the guard and properly dropping it on application shutdown.
Consider this approach to fix the memory leak:
📝 Committable suggestion
🤖 Prompt for AI Agents
@@ -17,0 +36,4 @@fn spawn_hmd_audio_listener(mut cmds: Commands, session: Res<OxrSession>) {let space = session.create_reference_space(openxr::ReferenceSpaceType::VIEW, BevyTransform::IDENTITY)⚠️ Potential issue
Handle the error from destroy_space instead of ignoring it.
The error from
session.destroy_space()is being ignored, which could hide important cleanup failures.📝 Committable suggestion
🤖 Prompt for AI Agents
@@ -1,314 +1,498 @@use super::{MODEL_PART_ASPECT_ALIAS_INFO, MaterialParameter, ModelAspect, ModelPartAspect};⚠️ Potential issue
Fix early return in iterator that prevents proper cleanup.
Using
returninside an iterator skips remaining entities. Should usecontinueinstead.📝 Committable suggestion
🤖 Prompt for AI Agents
@@ -76,1 +91,4 @@{let Ok(mut mesh_mat) = query.get_mut(**model_part.mesh_entity.get().unwrap()) else {continue;};🛠️ Refactor suggestion
Handle GLTF files with multiple scenes.
The code assumes scene index 0 exists, which may not be true for all GLTF files. Consider making this configurable or loading the default scene.
Alternatively, load the GLTF asset first to inspect available scenes:
🤖 Prompt for AI Agents
🛠️ Refactor suggestion
Address the potential DoS vector in model part creation.
The comment mentions this could be a DoS vector. Consider adding limits or validation.
🤖 Prompt for AI Agents
@@ -45,0 +111,4 @@font_size: style.character_height,line_height: style.character_height,alignment,max_width,⚠️ Potential issue
Fix incorrect height bounds calculation.
The max_height is incorrectly using bounds.x instead of bounds.y.
📝 Committable suggestion
🤖 Prompt for AI Agents
@@ -14,4 +17,4 @@use mint::Vector3;use parking_lot::Mutex;use rustc_hash::FxHashMap;use std::fmt::Debug;🛠️ Refactor suggestion
Consider implementing entity cleanup for dead spatial references.
The comment raises a valid concern. When the spatial weak reference can't be upgraded, the entity is orphaned and should be despawned to prevent resource leaks.
Note: This would require adding
Commandsto the system parameters.🤖 Prompt for AI Agents
⚠️ Potential issue
Fix incorrect AABB transformation for rotated children.
Transforming only the min and max corners of an AABB produces incorrect results when the child is rotated. You need to transform all 8 corners to get the correct enclosing AABB.
📝 Committable suggestion
🤖 Prompt for AI Agents
⚠️ Potential issue
Fix incorrect AABB transformation in relative bounding box calculation.
This has the same issue as the previous AABB transformation - only min/max corners are transformed, which produces incorrect results for rotated transforms.
📝 Committable suggestion
🤖 Prompt for AI Agents
@@ -25,0 +62,4 @@app.add_systems(Startup, setup.run_if(resource_exists::<OxrInstance>));app.add_systems(PreFrameWait, update);}}🛠️ Refactor suggestion
Use platform-appropriate paths and avoid redundant file writes.
The current implementation has portability issues and inefficiently writes the file on every startup.
Then use
get_cursor_model_path()instead ofCURSOR_MODEL_PATH.🤖 Prompt for AI Agents
@@ -25,0 +130,4 @@cmds.entity(entity).insert(MeshMaterial3d(handle)).insert(CorrectHandMaterial);}⚠️ Potential issue
Fix typo in constant name.
The constant name has a typo.
🤖 Prompt for AI Agents
@@ -5,0 +64,4 @@}}fn destroy_stage_space(session: Res<OxrSession>, mut cmds: Commands, stage: Res<StageSpace>) {session.destroy_space(stage.0);🛠️ Refactor suggestion
Consider handling stage space creation failure more robustly.
When stage space creation fails, the system continues without it, which might cause issues in the update system.
Consider either:
🤖 Prompt for AI Agents
@@ -5,0 +70,4 @@/// TODO: impl thisfn update(session: Option<Res<OxrSession>>,⚠️ Potential issue
Clarify or remove misleading TODO comment.
The function is implemented despite the TODO comment. The commented code suggests bounds calculation is not yet implemented.
Either:
Would you like me to help implement the bounds calculation based on the OpenXR stage reference space?
Also applies to: 120-133
🤖 Prompt for AI Agents
text align is still slightly broken/unimplemented but its def good enough
merged, thank you for your patronage :)
was my bad btw, forgot that merging a PR here in github is different than applying commits other ways