-
Notifications
You must be signed in to change notification settings - Fork 3
Update to bevy 0.17.3 and bevy_rapier3d 0.32.0 #16
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
Changed Files
|
Co-authored-by: cleder <1218010+cleder@users.noreply.github.com>
…move outdated entries
|
@copilot run |
Co-authored-by: cleder <1218010+cleder@users.noreply.github.com>
All tests pass. Fixed Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
@copilot run |
|
@copilot fix these errors error: use of deprecated type alias error: use of deprecated type alias error: use of deprecated type alias error: use of deprecated type alias error: use of deprecated type alias error: use of deprecated type alias error: could not compile |
Co-authored-by: cleder <1218010+cleder@users.noreply.github.com>
Fixed clippy warnings for deprecated bevy 0.17 APIs in commit Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
@copilot fix: cargo fmt --all -- --check
Diff in /home/runner/work/brkrs/brkrs/src/systems/../../tests/common/respawn_visual.rs:22:
Diff in /home/runner/work/brkrs/brkrs/src/systems/textures/loader.rs:3: use bevy::asset::io::Reader; -use bevy::prelude::; use super::loader::{TextureManifest, VisualAssetProfile};
Diff in /home/runner/work/brkrs/brkrs/tests/respawn_visual.rs:46: fn finish_overlay_timer(app: &mut App) {
fn overlay_exists(app: &mut App) -> bool {
Diff in /home/runner/work/brkrs/brkrs/tests/respawn_visual.rs:75:
|
Co-authored-by: cleder <1218010+cleder@users.noreply.github.com>
Fixed in commit Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Co-authored-by: cleder <1218010+cleder@users.noreply.github.com>
Fixed in commit |
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
||||||||||||||||||||||||||||||||||||
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.
Important
Looks good to me! 👍
Reviewed everything up to 9bc4675 in 52 seconds. Click for details.
- Reviewed
4259lines of code in19files - Skipped
0files when reviewing. - Skipped posting
0draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
Workflow ID: wflow_jP9CjN6W888ZAvvp
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
PR Code Suggestions ✨Explore these optional code suggestions:
|
|||||||||
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.
AI Code Review by LlamaPReview
🎯 TL;DR & Recommendation
Recommendation: Approve with suggestions
This PR successfully updates Bevy and bevy_rapier3d dependencies with comprehensive API migrations, but introduces minor maintainability and performance considerations in test patterns and system architecture.
🌟 Strengths
- Comprehensive update to Bevy 0.17.3 and bevy_rapier3d 0.32.0 with all breaking API changes addressed.
- All tests pass and code compiles without warnings, ensuring reliability.
| Priority | File | Category | Impact Summary | Anchors |
|---|---|---|---|---|
| P2 | src/systems/respawn.rs | Testing | Verbose test code reduces readability across files | tests/respawn_timer.rs, tests/respawn_visual.rs |
| P2 | src/systems/respawn.rs | Testing | Unwrap in tests risks masking entity errors | tests/respawn_visual.rs, tests/common/multi_respawn.rs |
| P2 | src/level_loader.rs | Architecture | Artificial system grouping obscures execution order | |
| P2 | Cargo.toml | Architecture | Removed SIMD feature may degrade physics performance | |
| P2 | src/lib.rs | Architecture | CursorOptions changes risk synchronization issues | src/pause.rs |
🔍 Notable Themes
- Testing Verbosity: Migration to
query_filtered()introduces more verbose patterns in multiple test files, impacting readability and maintainability. - Architectural Shifts: Changes in system grouping and cursor management require careful ordering to prevent synchronization issues.
💡 Have feedback? We'd love to hear it in our GitHub Discussions.
✨ This review was generated by LlamaPReview Advanced, which is free for all open-source projects. Learn more.
| let ball_count = { | ||
| let mut query = app | ||
| .world_mut() | ||
| .query_filtered::<Entity, (With<Ball>, With<BallFrozen>)>(); | ||
| query.iter(app.world()).count() | ||
| }; |
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.
P2 | Confidence: High
Speculative: The migration from iter_entities() to query_filtered() introduces more verbose test code patterns across multiple test files. While functionally equivalent, the new pattern requires more lines and explicit query construction, potentially reducing test readability. The related_context shows this pattern repeated in 4+ test files, suggesting a maintainability impact for future test modifications.
| let mut schedule = app.world_mut().resource_mut::<RespawnSchedule>(); | ||
| schedule.pending = Some(RespawnRequest { | ||
| lost_ball: Entity::from_raw(999), | ||
| lost_ball: Entity::from_row(EntityRow::from_raw_u32(999).unwrap()), |
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.
P2 | Confidence: High
Speculative: The migration from Entity::from_raw() to Entity::from_row(EntityRow::from_raw_u32().unwrap()) introduces potential panic points in tests via unwrap(). While acceptable in test contexts, this pattern could mask deeper issues if entity creation fails. The related_context shows this pattern used in multiple test files, creating consistency risks.
| @@ -595,22 +594,23 @@ fn toggle_wireframe( | |||
| } | |||
|
|
|||
| fn grab_mouse( | |||
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.
P2 | Confidence: High
The migration from direct window cursor control to separate CursorOptions component creates a potential synchronization issue. The related_context shows similar changes in src/pause.rs where cursor visibility is managed independently. This architectural change means cursor state is now managed via component rather than direct window access, which could lead to race conditions if multiple systems modify CursorOptions in the same frame without explicit ordering.
Code Suggestion:
app.configure_sets(Update, CursorSet.after(InputSet).before(TransformSet));Evidence: path:src/pause.rs
| destroy_all_bricks_on_key, | ||
| process_level_switch_requests, | ||
| sync_level_presentation, | ||
| ( |
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.
P2 | Confidence: Medium
The system tuple splitting for IntoScheduleConfigs compatibility creates artificial grouping boundaries that may not reflect logical system relationships. While necessary for Bevy 0.17 compatibility, this could obscure the intended execution order and make future system modifications more error-prone. The two-tuple structure appears arbitrary rather than reflecting clear domain boundaries.
| bevy_rapier3d = { version = "0.32.0", features = [ | ||
| "debug-render-3d", | ||
| ] } |
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.
P2 | Confidence: Medium
Speculative: Removal of the simd-stable feature from bevy_rapier3d may impact physics performance on supported hardware. While done for compatibility reasons, this change could degrade performance in physics-heavy scenarios. The related_context doesn't provide performance benchmarks to quantify the impact, but SIMD optimizations typically provide significant speedups for collision detection and resolution.
User description
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.
PR Type
Enhancement, Tests
Description
Update Bevy from 0.16 to 0.17.3 and bevy_rapier3d from 0.31 to 0.32.0
Migrate Events/EventReader/EventWriter to Messages/MessageReader/MessageWriter
Fix breaking API changes: Timer::finished() → is_finished(), Trigger → On, CursorOptions component
Split large system tuples (>8) for IntoScheduleConfigs compatibility
Update Entity::from_raw to Entity::from_row with EntityRow, iter_entities() to query_filtered()
Diagram Walkthrough
File Walkthrough
1 files
Update Bevy and bevy_rapier3d dependencies9 files
Migrate to Bevy 0.17 APIs and message systemSplit large system tuples for compatibilityUpdate cursor handling with CursorOptions componentConvert Event to Message for level switchingMigrate Events to Messages and update Entity APIConvert Event to Message for preview requestsUpdate to MessageReader for asset eventsUpdate to Messages for asset event handlingUpdate JustifyText to Justify API7 files
Update test utilities to Messages APIMigrate tests to query_filtered and MessagesUpdate tests with EntityRow and query_filteredRemove placeholder assertions from testsUpdate test to use write_message APIMigrate test to Messages and query_filteredUpdate test with EntityRow and query_filtered1 files
Update Active Technologies to Bevy 0.17Important
Update Bevy to 0.17.3 and bevy_rapier3d to 0.32.0, adjusting code for API changes and ensuring compatibility.
bevyfrom 0.16 to 0.17.3 inCargo.toml.bevy_rapier3dfrom 0.31 to 0.32.0, removingsimd-stablefeature.EventtoMessageandEventReader/EventWritertoMessageReader/MessageWriterin multiple files.add_eventtoadd_messagefor message types.cursor_optionstoCursorOptionscomponent inlib.rsandpause.rs.JustifyTexttoJustifyinpause_overlay.rs.Timer::finished()toTimer::is_finished()inlib.rsandrespawn.rs.RenderAssetUsagesfrombevy::imagetobevy::assetinlib.rs.IntoScheduleConfigscompatibility inlevel_loader.rs.Entity::from_rawtoEntity::from_rowwithEntityRowinrespawn.rs.TriggertoOnfor observer events inlib.rs.iter_entities()withquery_filtered()inrespawn.rs.clippywith no deprecation warnings.cargo fmtto fix code formatting.This description was created by
for 9bc4675. You can customize this summary. It will automatically update as commits are pushed.