Bevy 0.13 #314
Bevy 0.13 #314
Conversation
StrikeForceZero
force-pushed
the
bevy-0.13
branch
from
bb671ee
to
14d7860
Compare
backends/bevy_picking_ui/src/lib.rs
Outdated
|
|
||
| // The last camera in the list will be the one with the highest order, and be the topmost. | ||
| let Some((camera_entity, camera, _)) = ui_cameras.last() else { | ||
| let Some((camera_entity, camera)) = ui_cameras.last() else { |
There was a problem hiding this comment.
might need some further consideration for setups using IsDefaultUiCamera?
There was a problem hiding this comment.
Might make sense in my opinion as well. Would probably be best to adjust the sorting above to consider this and then the "normal" ordering value as a second criterion. Something like
// ...
// Query<(..., Has<IsDefaultUiCamera>)>
// ...
sort_by(|(_, cam_a, has_default_a), (_, cam_b, has_default_b)| {
has_default_a.cmp(has_default_b) // false < true, so last is still the one with biggest prio
.then_with(|| cam_a.order.cmp(cam_b.order)
})
StrikeForceZero
force-pushed
the
bevy-0.13
branch
2 times, most recently
from
260b9fd
to
c2c48e9
Compare
| @@ -51,14 +51,16 @@ pub fn mouse_pick_events( | |||
| MouseButton::Right => PointerButton::Secondary, | |||
| MouseButton::Middle => PointerButton::Middle, | |||
| MouseButton::Other(_) => continue, | |||
| MouseButton::Back => continue, | |||
There was a problem hiding this comment.
Should we not add these new events?
There was a problem hiding this comment.
Considering it's just used as part of that debug UI (the [ ] [ ] [ ]) and the Other was not being handled, I'd assume there's no use case.
Otherwise, this struct and its implementation would likely have to be extended.
bevy_mod_picking/crates/bevy_picking_core/src/pointer.rs
Lines 101 to 130 in 1471a9f
There was a problem hiding this comment.
So, after further review, I was wrong in my previous response. The component PointerPress and the event InputPress could totally be used by a user.. Not sure where I got the idea that it was only used for that debugging UI...
Anyway, I'm not sure if PointerPress.is_any_pressed should include MouseButton::Back and MouseButton::Forward or not, it could break expectations for anyone using it as is currently.
And if we are exposing MouseButton::Forward and MouseButton::Backward in PointerPress, we should also expose MouseButton::Custom(_) to offer optional but full flexibility.
Thoughts?
There was a problem hiding this comment.
We'd add back and forward fields to PointerPress to match the new events. In terms of the debug UI, I agree that using the side buttons on the mouse is probably not "useful" or common, but I'm not sure that we need to implement them on the debug UI in order to still allow users to implement events on them (something I could see being more useful).
|
Update: I've spent quite a long while trying to figure out why the floating debug UI is not being rendered in the proper camera for
Also, egui may be crashing most of the examples with Might be similar to
The examples might need tweaking to the world lighting as everything is fairly dim. |
|
The |
StrikeForceZero
force-pushed
the
bevy-0.13
branch
from
103e5f2
to
62d7519
Compare
Seems to be the same issue as described in jakobhellermann/bevy-inspector-egui#185 (comment). Read the thread further up for more information. It appears to be a bevy bug |
On first glance might be related to https://bevyengine.org/learn/migration-guides/0-12-to-0-13/#camera-driven-ui. I take a closer look at it. |
|
The debug UI seems to work for me. What platform are you on?
|
Linux x86_64. Yourself? That screen recording looks like its using the egui ui. The system responsible for the one in my screenshot is bevy_mod_picking/src/debug/mod.rs Lines 355 to 388 in 1471a9f .add_plugins((
DefaultPlugins.set(low_latency_window_plugin()),
DefaultPickingPlugins,
// bevy_egui::EguiPlugin,
))If I add a Edit: now that I think of it I wonder if it even matches the mouse position on the current release. I didn't realize until seeing your screen recording that the egui version is different system than the debug_draw. I'll check in the morning. |
| (Option<&Sprite>, Option<&TextureAtlasSprite>), | ||
| (Option<&Handle<Image>>, Option<&Handle<TextureAtlas>>), | ||
| (Option<&Sprite>, Option<&TextureAtlas>), | ||
| (Option<&Handle<Image>>, Option<&TextureAtlas>), |
There was a problem hiding this comment.
It might make sense to un-tuple this to prevent the duplication of Option<&TextureAtlas>.
backends/bevy_picking_ui/src/lib.rs
Outdated
|
|
||
| // The last camera in the list will be the one with the highest order, and be the topmost. | ||
| let Some((camera_entity, camera, _)) = ui_cameras.last() else { | ||
| let Some((camera_entity, camera)) = ui_cameras.last() else { |
There was a problem hiding this comment.
Might make sense in my opinion as well. Would probably be best to adjust the sorting above to consider this and then the "normal" ordering value as a second criterion. Something like
// ...
// Query<(..., Has<IsDefaultUiCamera>)>
// ...
sort_by(|(_, cam_a, has_default_a), (_, cam_b, has_default_b)| {
has_default_a.cmp(has_default_b) // false < true, so last is still the one with biggest prio
.then_with(|| cam_a.order.cmp(cam_b.order)
})
|
@StrikeForceZero I opened a PR implementing all of my suggestions here StrikeForceZero#1 |
Me too.
Yes, how are you even running the example without egui? If I try to just run it on its own I get: so if it depends on the egui backend feature it's not really surprising to me that it won't work if you comment that out :D |
|
Still not sure how @RobWalt is able to run the examples, while I'm not able, but this diff below appears to have fixed the issue for me. I've also confirmed that the current release of I think only relying on the Do we want to include this in the PR? Should fixing the positioning logic for Index: src/debug/mod.rs
<+>UTF-8
===================================================================
diff --git a/src/debug/mod.rs b/src/debug/mod.rs
--- a/src/debug/mod.rs (revision HEAD)
+++ b/src/debug/mod.rs (revision Staged)
@@ -134,7 +134,9 @@
debug_draw.run_if(|r: Option<Res<bevy_egui::EguiUserTextures>>| r.is_none()),
// if egui is available, always draw the egui debug if possible
#[cfg(feature = "backend_egui")]
- debug_draw_egui.run_if(|r: Option<Res<bevy_egui::EguiUserTextures>>| r.is_some()),
+ debug_draw_egui
+ .run_if(|r: Option<Res<bevy_egui::EguiUserTextures>>| r.is_some())
+ .after(bevy_egui::systems::begin_frame_system),
)
.chain()
.distributive_run_if(DebugPickingMode::is_enabled)
|
|
I've also looked a bit deeper into that and you seem to be correct. The But ofc other opinions on that are also welcome and should be considered first! Also on a completely unrelated note: Can we change .run_if(|r: Option<Res<bevy_egui::EguiUserTextures>>| r.is_none())
.run_if(|r: Option<Res<bevy_egui::EguiUserTextures>>| r.is_some())to the more bevy idiomatic .run_if(not(resource_exists::<bevy_egui::EguiUserTextures>))
.run_if(resource_exists::<bevy_egui::EguiUserTextures>)please? 🙈 I think the latter actually runs the function only once and re-uses the produced value for both use cases while the first are two different checks. It's a bit nit picky so feel free to ignore. I just feel a constant urge to implement such new patterns everywhere to help establishing good habits for readers. |
|
Thanks for working on this. I might have some feedback, but am busy wrapping up some steps upstream. I've enabled CI, so you should be able to at least get that green. 🙂 |
StrikeForceZero
force-pushed
the
bevy-0.13
branch
from
4cfe442
to
a50be81
Compare
|
Welp, we will have to update the deprecated usage for shapes to get a green build. I'll see if I have time for that tonight. |
|
Everything is updated and should be generally correct now. I found a bug with sprites due to the texture atlas changes. I ran through all the examples, and they appear to be working correctly. I'd like to review the bevy_ui backend tomorrow before merging. Now that we have camera-driven bevy_ui, we should be able to remove the camera order hack that was added due to bevy_ui's special casing. There may also be work needed to make the debug overlay work on multiple cameras/viewports. If anyone here has bandwidth, I'd appreciate a double-check that the bevy_ui backend in this branch still matches the focus system on main, and works with multiple cameras. |
yeah looks like the issues I was seeing with debug_draw on the split screen examples are present with the ui + multiple cameras.
Reproduction Diff Index: examples/bevy_ui.rs
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/examples/bevy_ui.rs b/examples/bevy_ui.rs
--- a/examples/bevy_ui.rs (revision Staged)
+++ b/examples/bevy_ui.rs (date 1708835783274)
@@ -7,7 +7,7 @@
App::new()
.add_plugins(DefaultPlugins.set(low_latency_window_plugin()))
.add_plugins(DefaultPickingPlugins)
- .add_systems(Startup, (setup, setup_3d))
+ .add_systems(Startup, (setup, setup_3d, set_camera_viewports).chain())
.add_systems(Update, update_button_colors)
.insert_resource(UiScale(1.5))
.run();
@@ -62,6 +62,7 @@
/// set up a simple 3D scene
fn setup_3d(
+ root_node: Query<(Entity, Has<TargetCamera>), (With<Node>, Without<Parent>)>,
mut commands: Commands,
mut meshes: ResMut<Assets<Mesh>>,
mut materials: ResMut<Assets<StandardMaterial>>,
@@ -94,15 +95,87 @@
transform: Transform::from_xyz(4.0, 8.0, -4.0),
..default()
});
- commands.spawn((Camera3dBundle {
- transform: Transform::from_xyz(3.0, 3.0, 3.0).looking_at(Vec3::ZERO, Vec3::Y),
- camera: Camera {
- order: 1,
- ..default()
- },
- ..default()
- },));
+ commands.spawn((
+ Camera3dBundle {
+ transform: Transform::from_xyz(0.0, 20.0, -10.0).looking_at(Vec3::ZERO, Vec3::Y),
+ ..default()
+ },
+ LeftCamera,
+ ));
+
+ let ui_camera = commands
+ .spawn((
+ Camera3dBundle {
+ transform: Transform::from_xyz(10.0, 10., 15.0).looking_at(Vec3::ZERO, Vec3::Y),
+ camera: Camera {
+ // don't clear on the second camera because the first camera already cleared the window
+ clear_color: ClearColorConfig::None,
+ // Renders the right camera after the left camera, which has a default priority of 0
+ order: 1,
+ ..default()
+ },
+ ..default()
+ },
+ IsDefaultUiCamera,
+ RightCamera,
+ ))
+ .id()
+ ;
+
+ if root_node.iter().len() != 1 {
+ panic!("expecting exactly 1 root node!");
+ }
+
+ for (root_node, has_target_camera) in root_node.iter() {
+ if !has_target_camera {
+ commands
+ .entity(root_node)
+ .insert(TargetCamera(ui_camera))
+ ;
+ }
+ }
+}
+
+#[derive(Component)]
+struct LeftCamera;
+
+#[derive(Component)]
+struct RightCamera;
+
+
+fn set_camera_viewports(
+ windows: Query<&Window>,
+ mut resize_events: EventReader<bevy::window::WindowResized>,
+ mut left_camera: Query<&mut Camera, (With<LeftCamera>, Without<RightCamera>)>,
+ mut right_camera: Query<&mut Camera, With<RightCamera>>,
+) {
+ // We need to dynamically resize the camera's viewports whenever the window size changes so then
+ // each camera always takes up half the screen. A resize_event is sent when the window is first
+ // created, allowing us to reuse this system for initial setup.
+ for resize_event in resize_events.read() {
+ let window = windows.get(resize_event.window).unwrap();
+ let mut left_camera = left_camera.single_mut();
+ left_camera.viewport = Some(bevy::render::camera::Viewport {
+ physical_position: UVec2::new(0, 0),
+ physical_size: UVec2::new(
+ window.resolution.physical_width() / 2,
+ window.resolution.physical_height(),
+ ),
+ ..default()
+ });
+
+ let mut right_camera = right_camera.single_mut();
+ right_camera.viewport = Some(bevy::render::camera::Viewport {
+ physical_position: UVec2::new(window.resolution.physical_width() / 2, 0),
+ physical_size: UVec2::new(
+ window.resolution.physical_width() / 2,
+ window.resolution.physical_height(),
+ ),
+ ..default()
+ });
+ }
}
+
trait NewButton {
fn add_button(self, text: &str) -> Self; |
|
Don't want us to forget about these comments: (there's also some unresolved discussions) Back to the bevy_ui backend issues: Edit: I think my lack of experience with bevy is leaking. The replacement filter logic is likely more expensive or just plain wrong. Maybe I was fixing the side effect instead of the root cause..
Show diff referenced in above crossed out section```diff Index: backends/bevy_picking_ui/src/lib.rs IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/backends/bevy_picking_ui/src/lib.rs b/backends/bevy_picking_ui/src/lib.rs --- a/backends/bevy_picking_ui/src/lib.rs (revision HEAD) +++ b/backends/bevy_picking_ui/src/lib.rs (revision Staged) @@ -92,7 +92,7 @@ ( pointer, Location { - position: loc.position / ui_scale, + position: loc.position, ..loc }, ) @@ -110,8 +110,7 @@ let mut ui_cameras: Vec<_> = cameras .iter() .filter(|(_, camera, _)| { - camera.is_active - && camera.target.normalize(Some(window_entity)).unwrap() == location.target + location.is_in_viewport(camera, &primary_window) }) .filter(|(camera_entity, _, _)| { let matching_root_node = root_nodes.iter().find( ``` |
This is because prior to 0.13, bevy_ui didn't support per-camera ui at all. Now that it does, I'd like to get that fixed. First step is fixing the backend, which is what I'm doing now, followed by the debug overlay. |
For what it's worth, I spent the day learning how to do proper tests for Bevy apps. Let me know if you want us to get some tests written for anything in this PR. Otherwise, good luck! |
|
Tests are always appreciated, but not a blocker right now. Still trying to figure out what is going on with the bevy debug overlay. |
|
After much testing I'm under the impression that changing camera with TargetCamera results in a potential bevy bug. with these changes running Index: src/debug/mod.rs
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/debug/mod.rs b/src/debug/mod.rs
--- a/src/debug/mod.rs (revision HEAD)
+++ b/src/debug/mod.rs (revision Staged)
@@ -367,23 +367,21 @@
};
let text = format!("{id:?}\n{debug}");
- for camera in camera_query
+ for (camera_entity, camera) in camera_query
.iter()
.map(|(entity, camera)| {
(
entity,
+ camera,
camera.target.normalize(primary_window.get_single().ok()),
)
})
- .filter_map(|(entity, target)| Some(entity).zip(target))
- .filter(|(_entity, target)| target == &pointer_location.target)
- .map(|(cam_entity, _target)| cam_entity)
+ .filter_map(|(entity, camera, target)| Some((entity, camera)).zip(target))
+ .filter(|((_entity, camera), target)| target == &pointer_location.target && pointer_location.is_in_viewport(camera, &primary_window))
+ .map(|((cam_entity, camera), _target)| (cam_entity, camera))
{
let mut pointer_pos = pointer_location.position;
- if let Some(viewport) = camera_query
- .get(camera)
- .ok()
- .and_then(|(_, camera)| camera.logical_viewport_rect())
+ if let Some(viewport) = camera.logical_viewport_rect()
{
pointer_pos -= viewport.min;
}
The UI follows the cursor going from the left to the right viewport. but once you go from right to left, it jumps to a random spot on the left viewport (seems like it takes the last known y/top value, but sets x/left to some arbitrary value) and won't snap back to the mouse position until you return to the right viewport. My first suspicion was that the first time TargetCamera hit the scene, it took some weird priority for all other UI nodes. But that doesn't seem to be the case. Oddly, you can reverse this behavior by disabling the setup_ui system (as shown in the next diff below);
Index: examples/bevy_ui.rs
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/examples/bevy_ui.rs b/examples/bevy_ui.rs
--- a/examples/bevy_ui.rs (revision HEAD)
+++ b/examples/bevy_ui.rs (revision Staged)
@@ -7,7 +7,7 @@
App::new()
.add_plugins(DefaultPlugins.set(low_latency_window_plugin()))
.add_plugins(DefaultPickingPlugins)
- .add_systems(Startup, (setup_3d, setup_ui).chain())
+ .add_systems(Startup, (setup_3d/*, setup_ui*/).chain())
.add_systems(Update, (update_button_colors, set_camera_viewports))
.insert_resource(UiScale(1.5))
.run();
I've also swapped the camera order just in case, and that seemed to have no effect. This contradicts that theory, with the setup_ui system enabled it doesn't matter what side you start in. Only once you cross the viewport boundary, it breaks whenever your mouse is in the left viewport. So, it can render perfectly fine as long as your mouse stays in the left viewport. The only reason egui is able to work is because they are using their own draw calls that bypass bevy all together. Edit: I forgot to mention I was using |
|
I found a workaround that seems to work. If you want to roll with this until we can identify if this behavior is a bug or not, let me know, and I can push it up. The workaround uses a HashMap to keep track of a single UI node per camera and toggles their visibility based on whether the cursor is in the viewport. By not changing the
I left a TODO in the diff that mentions side effects with multiple cameras overlapping viewports, if we ensure we process cameras from smallest to highest order it should be resolved. Index: src/debug/mod.rs
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/debug/mod.rs b/src/debug/mod.rs
--- a/src/debug/mod.rs (revision HEAD)
+++ b/src/debug/mod.rs (revision Staged)
@@ -12,6 +12,7 @@
use bevy_math::prelude::*;
use bevy_reflect::prelude::*;
use bevy_render::prelude::*;
+use bevy_utils::HashMap;
use bevy_utils::tracing::{debug, trace};
/// This state determines the runtime behavior of the debug plugin.
@@ -91,6 +92,7 @@
};
app.init_state::<DebugPickingMode>()
+ .init_resource::<CameraDebugUINodeMap>()
.insert_resource(State::new(start_mode))
.add_systems(
PreUpdate,
@@ -350,9 +352,15 @@
}
}
+/// Keeps track of debug_draw ui nodes for each camera to avoid updating TargetCamera
+/// see https://github.com/aevyrie/bevy_mod_picking/pull/314#issuecomment-1974034094
+#[derive(Debug, Clone, Resource, Default)]
+pub struct CameraDebugUINodeMap(pub HashMap<Entity, Entity>);
+
#[cfg(feature = "backend_bevy_ui")]
/// Draw text on each cursor with debug info
pub fn debug_draw(
+ mut camera_debug_ui_node_map: ResMut<CameraDebugUINodeMap>,
mut commands: Commands,
camera_query: Query<(Entity, &Camera)>,
primary_window: Query<Entity, With<bevy_window::PrimaryWindow>>,
@@ -361,35 +369,47 @@
) {
use bevy_text::prelude::*;
use bevy_ui::prelude::*;
- for (entity, id, debug) in pointers.iter() {
+ for (_entity, id, debug) in pointers.iter() {
let Some(pointer_location) = &debug.location else {
continue;
};
let text = format!("{id:?}\n{debug}");
- for camera in camera_query
+ for (camera_entity, camera) in camera_query
.iter()
.map(|(entity, camera)| {
(
entity,
+ camera,
camera.target.normalize(primary_window.get_single().ok()),
)
})
- .filter_map(|(entity, target)| Some(entity).zip(target))
- .filter(|(_entity, target)| target == &pointer_location.target)
- .map(|(cam_entity, _target)| cam_entity)
+ .filter_map(|(entity, camera, target)| Some((entity, camera)).zip(target))
+ .filter(|((_entity, camera), target)| target == &pointer_location.target && pointer_location.is_in_viewport(camera, &primary_window))
+ .map(|((cam_entity, camera), _target)| (cam_entity, camera))
{
let mut pointer_pos = pointer_location.position;
- if let Some(viewport) = camera_query
- .get(camera)
- .ok()
- .and_then(|(_, camera)| camera.logical_viewport_rect())
+ if let Some(viewport) = camera.logical_viewport_rect()
{
pointer_pos -= viewport.min;
}
+ // update visibility for each debug ui node
+ // TODO: this might have side effects if viewports overlapped?
+ for (&entry_camera_entity, &entry_debug_ui_node_entity) in camera_debug_ui_node_map.0.iter() {
+ // make sure the debug ui node for the current camera is visible
+ if entry_camera_entity == camera_entity {
+ commands.entity(entry_debug_ui_node_entity).insert(InheritedVisibility::VISIBLE);
+ continue;
+ }
+ commands.entity(entry_debug_ui_node_entity).insert(InheritedVisibility::HIDDEN);
+ }
+
+ // get or spawn a debug ui node for this camera
+ let ui_node_entity = *camera_debug_ui_node_map.0.entry(camera_entity).or_insert_with(|| commands.spawn(NodeBundle::default()).id());
+
commands
- .entity(entity)
+ .entity(ui_node_entity)
.insert(TextBundle {
text: Text::from_section(
text.clone(),
@@ -408,7 +428,7 @@
..Default::default()
})
.insert(Pickable::IGNORE)
- .insert(TargetCamera(camera));
+ .insert(TargetCamera(camera_entity));
}
}
}
|
|
I'm going to merge this as-is, so I can continue working on some other PRs, while we continue to work on the bevy_ui debug issues. Thanks for doing this! |
|
@aevyrie mind bumping the version so folks can migrate to 0.13? Or are you waiting for this bug to get fixed first? |
|
I was planning on waiting for the bugfixes to land, but I don't know how long that will take, and at this point it's probably better to just ship with the bevy_ui issues, and fix it when we can. |
|
You could use a git dependency temporarily until a version is cut @feelingsonice |
|
I've gone ahead and published 0.18 on crates.io with the bug. I'll make a bugfix once bevy 0.13.1 is out. Unfortunately the docs.rs build failed for the workspace crate, trying to figure that out too. |
Attempting to support bevy 0.13
Resolves #313
Disclaimer: Assumptions were made and might be wrong.
TODO:
0.18?)Bevy Version SupportBlocked on: