aevyrie / bevy_mod_picking

Picking and pointer events for Bevy.
https://crates.io/crates/bevy_mod_picking
Apache License 2.0
764 stars 170 forks source link

Bevy 0.13 #314

Closed StrikeForceZero closed 6 months ago

StrikeForceZero commented 6 months ago

Attempting to support bevy 0.13

Resolves https://github.com/aevyrie/bevy_mod_picking/issues/313

Disclaimer: Assumptions were made and might be wrong.

TODO:

Blocked on:

StrikeForceZero commented 6 months ago

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 example/split_screen to no avail. Would appreciate some help.

image


Also, egui may be crashing most of the examples with No fonts available until first call to Context::run() commenting out the egui plugin lets the examples run.

Might be similar to https://github.com/mvlabat/bevy_egui/issues/106#issuecomment-1144158462

trying to render something before the context is initialized


The examples might need tweaking to the world lighting as everything is fairly dim.

chrisjuchem commented 6 months ago

The Pointer derive needs to specify that the event should bubble. This was a breaking change added in bevy_eventlistener. https://github.com/aevyrie/bevy_eventlistener/commit/a300dc5f1076564387d06ab67f5ba84c95964b7f

RobWalt commented 6 months ago

The examples might need tweaking to the world lighting as everything is fairly dim.

Seems to be the same issue as described in https://github.com/jakobhellermann/bevy-inspector-egui/pull/185#issuecomment-1954958763. Read the thread further up for more information. It appears to be a bevy bug

RobWalt commented 6 months ago

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 example/split_screen to no avail. Would appreciate some help.

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.

RobWalt commented 6 months ago

The debug UI seems to work for me. What platform are you on?

Kooha-2024-02-21-08-43-20

StrikeForceZero commented 6 months ago

The debug UI seems to work for me. What platform are you on?

[embedded file removed for readability]

Linux x86_64. Yourself?

That screen recording looks like its using the egui ui.

The system responsible for the one in my screenshot is debug_draw since I have to disable egui to get the examples to run https://github.com/aevyrie/bevy_mod_picking/blob/1471a9f83435f3fdd43829b90391e6ab965e0e73/src/debug/mod.rs#L355-L388

        .add_plugins((
            DefaultPlugins.set(low_latency_window_plugin()),
            DefaultPickingPlugins,
            // bevy_egui::EguiPlugin,
        ))

If I add a TargetCamera component to that entity collection being spawned in debug_draw and correctly identify what camera view the mouse is over, I think it works as expected.

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.

RobWalt commented 6 months ago

@StrikeForceZero I opened a PR implementing all of my suggestions here https://github.com/StrikeForceZero/bevy_mod_picking/pull/1

RobWalt commented 6 months ago

Linux x86_64. Yourself?

Me too.

That screen recording looks like its using the egui ui.

Yes, how are you even running the example without egui? If I try to just run it on its own I get:

error: target `split_screen` in package `bevy_mod_picking` requires the features: `backend_egui`
Consider enabling them by passing, e.g., `--features="backend_egui"`

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

StrikeForceZero commented 6 months ago

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 bevy_mod_picking exhibits similar positioning issues in the split_screen example. In this case, with the egui plugin disabled, the debug UI, which follows the mouse when multiple camera views are active, relies on the debug_draw system.

I think only relying on the bevy_egui::EguiUserTextures to exist might be introducing indeterministic behavior.

Do we want to include this in the PR?

Should fixing the positioning logic for debug_draw be part of the scope for this PR? IMO, I think it should skipped for now since it existed prior.

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)
RobWalt commented 6 months ago

I've also looked a bit deeper into that and you seem to be correct. The bevy_egui::systems::begin_frame_system system also runs in the PreUpdate Schedule and this is most likely a system ordering issue and hence a slight bug. Cool that you found out about it. I'll approve your change there!

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.

aevyrie commented 6 months ago

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 commented 6 months ago

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.

aevyrie commented 6 months ago

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.

StrikeForceZero commented 6 months ago

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.

image

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;
StrikeForceZero commented 6 months ago

Don't want us to forget about these comments:

https://github.com/aevyrie/bevy_mod_picking/pull/314#issuecomment-1958466369

https://github.com/aevyrie/bevy_mod_picking/pull/314#issuecomment-1958826242

(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..

I'm probably misunderstanding the scope of some of this, but I was fiddling around with some of the different systems trying to identify what's the culprit for the various issues with the bevy_ui multiple cameras example. From my testing, it was resolving the wrong ui_camera until I removed / UIScale on Location.position. Which is set to 1.5 on the bevy_ui example. It also might be too late for me, and I've chased a red herring, but I wanna leave this here in case we need to return to it.

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( ```
aevyrie commented 6 months ago

I've also confirmed that the current release of bevy_mod_picking exhibits similar positioning issues in the split_screen example.

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.

StrikeForceZero commented 6 months ago

I've also confirmed that the current release of bevy_mod_picking exhibits similar positioning issues in the split_screen example.

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!

aevyrie commented 6 months ago

Tests are always appreciated, but not a blocker right now. Still trying to figure out what is going on with the bevy debug overlay.

StrikeForceZero commented 6 months ago

After much testing I'm under the impression that changing camera with TargetCamera results in a potential bevy bug.

with these changes running examples/bevy_ui

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 bevy-inspector-egui to monitor the Style of the debug text, and the top/left fields are being updated as the mouse moves as expected. Hence, why I'm suspicious that there's potentially a bevy-related bug when you try updating TargetCamera.

StrikeForceZero commented 6 months ago

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 TargetCamera, we avoid the potential bug.

If we agree that behavior looks like a bug based on my findings, I'll create a cleanroom example and open an issue in bevy. Opened 12255

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));
         }
     }
 }
aevyrie commented 6 months ago

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!

https://github.com/aevyrie/bevy_mod_picking/issues/317

feelingsonice commented 6 months ago

@aevyrie mind bumping the version so folks can migrate to 0.13? Or are you waiting for this bug to get fixed first?

aevyrie commented 6 months ago

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. ☚ī¸

tbillington commented 6 months ago

You could use a git dependency temporarily until a version is cut @feelingsonice

aevyrie commented 6 months ago

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.

https://github.com/rust-lang/docs.rs/issues/2441