Plonq / bevy_panorbit_camera

A simple pan and orbit camera for the Bevy game engine
Apache License 2.0
186 stars 35 forks source link

Mouse Inputs Passthrough from `egui` to `bevy_panorbit_camera` #75

Open SK83RJOSH opened 6 months ago

SK83RJOSH commented 6 months ago

When not directly scrolling / dragging / clicking egui ui elements, all inputs get passed through to bevy_panorbit_camera when using the following code to add ui systems:

add_systems(Update, (draw_title_bar, draw_file_tree).chain())

As demonstrated here:

https://github.com/Plonq/bevy_panorbit_camera/assets/1371787/cf471495-b9d0-4da2-9d53-e7a148f7520e

This appears to be a scheduling issue, and doing the following somewhat resolves the issue and blocks scroll events, but not mouse drag events:

add_systems(PreUpdate, (draw_title_bar, draw_file_tree).chain().after(EguiSet::BeginFrame))

As demonstrated here:

https://github.com/Plonq/bevy_panorbit_camera/assets/1371787/ddb11fee-1224-4b8d-9061-3ac5916e4fc8

Using the solution mentioned in #39 yields no further improvements.

Here is the code used for both of the above: repro.zip

Plonq commented 6 months ago

Hey, thanks for the repro. This appears to be a quirk of egui, haven't found a solution yet.

So, bevy_panorbit_camera tracks a boolean value in EguiWantsFocus reflecting the values of Context::wants_pointer_input and Context::wants_keyboard_input. If either of those return true in the current and previous frames, then panorbit ignores inputs.

The problem is that these values are false when they should seemingly be true. I tested your repro and logged to console whenever EguiWantsFocus changed. The results are surprising.

First I tested without your half-workaround, i.e. using add_systems(Update, (draw_title_bar, draw_file_tree).chain()). Most of the time, this resulted in neither dragging nor scrolling working as expected. Sometimes it resulted in scroll being blocked correctly. If I use the half-workaround, it consistently blocks scroll. There's obviously some sort of scheduling issue, but it looks like it's within egui somehow.

This first video shows what happens when both scrolling and dragging 'pass through' the egui side panel. You can see that EguiWantsFocus never changes, which means obviously panorbit assumes egui doesn't need input and thus uses the input. I added an egui Window as well, and that works as expected, which tells me egui treats anything that isn't a Window differently when it comes to input.

https://github.com/Plonq/bevy_panorbit_camera/assets/7709415/f4b52e9c-a1c5-4c7f-b848-80f17a4c606f

The second video shows the half-workaround. You can see the difference is that on hover EguiWantsFocus correctly changes to true, blocking scrolling. But as soon as you click (on mouse down), it changes back to false, thus panorbit uses the drag/rotate input.

https://github.com/Plonq/bevy_panorbit_camera/assets/7709415/bbeb5600-ecdf-41f5-8929-7cc5720c2a1d

My conclusion so far is that there's nothing this plugin can do - it's a matter of wants_pointer_input and/or wants_keyboard_input acting differently for side panels vs windows. I'm not really familiar with egui (haven't actually used it to build UI or anything), so maybe you have some ideas?

P.S. I tried moving all panorbit's systems into PostUpdate and it made no difference.

SK83RJOSH commented 5 months ago

@Plonq after some investigation, I've found one solution that you can implement for now, the following code:

let new_wants_focus = windows.iter().any(|window| {
    let ctx = contexts.ctx_for_window_mut(window);
    ctx.wants_pointer_input() || ctx.wants_keyboard_input()
});

Should also check if the cursor has entered an egui area:

let new_wants_focus = windows.iter().any(|window| {
    let ctx = contexts.ctx_for_window_mut(window);
    ctx.wants_pointer_input() || ctx.wants_keyboard_input() || ctx.is_pointer_over_area()
});

Which then results in the following behavior, which I would say is most wanted/expected:

https://github.com/Plonq/bevy_panorbit_camera/assets/1371787/0960ae6b-24ed-4ab6-85e0-4e1447be6517

Do note however, as mentioned here, this should occur in the frame as late as possible. So perhaps we should combine it with PostUpdate as well as the scheduling fix, as to not break it for other plugins (as alluded to there)/to be as correct as possible (though it's unclear what the intended order of operations is, unfortunately).

My apologies for the delay on triaging this, it's been a busy week. I've also pinged the egui author on the above thread in hopes they can shed some light on this issue. Though I think the above solution is adequate / the best possible for the time being. 😄

Plonq commented 5 months ago

That is a decent temporary workaround, but it's not ideal. If you start dragging outside egui, and drag over egui, it should continue that drag operation (e.g. rotating the camera). This is a very common UX pattern. For example if you click and drag a scrollbar you can move the mouse anywhere - it doesn't just stop working if you move your mouse outside the scrollbar track.

For this reason, I have implemented it as an optional configuration, disabled by default. See https://github.com/Plonq/bevy_panorbit_camera/pull/76, which has been released in v0.18.1

thmxv commented 5 months ago

Two possible "solutions" to avoid the camera handling stopping to work if the cursor quit the Bevy viewport:

Unfortunately this is platform specific and even depends on the windowing system (X11 or Wayland) on Linux.

Plonq commented 5 months ago

I don't particularly like the sound of either of those solutions.

  1. Grabbing the cursor would feel odd and unintuitive
  2. The egui panels display on top of the viewport, so this would need to be egui-specific

The best idea I have is to save some sort of state that regarding the current 'gesture', e.g. 'panning', and then if currently panning, ignore the 'egui wants focus' check. When the user lets the mouse go, the 'panning' gesture stops and thus the 'egui wants focus' check is once again in effect. The only thing is, this adds a bunch of complexity for something for a very specific use case that shouldn't be a problem in the first place, which is why I'm hesitant.

thmxv commented 3 months ago

One solution I found for bevy_blendy_cameras (not committed yet) is pretty much the following:

This fixes this issue and another related (invert case):

Edit: Unrelated to this issue but I thought you might want to know. While developing/using bevy_blendy_cameras (I have an example and multiple apps using bevy_blendy_cameras), I had a lot of issues related to systems ordering. Some example app where functioning fine, other not at all saying egui wanted the focus all the time. This behavior was changing at each launch of the app/example even without recompilation or with recompilation of small code change unrelated to the camera. It took me a lot of time experimenting with systems orders and debugging and I am still not sure it is correct, but the only way to get consistent behavior was to put the check_egui_wants_focus system in PreUpdate after EguiSet::BeginFrame.

Plonq commented 2 months ago

Thanks for commenting with this idea! I will try it out. Or if you're willing I'd be glad to accept a PR.

bevy_blendy_cameras sounds great. I'm guessing you skipped the smoothing (like blender)? That was the one thing that made adding auto-depth ultimately impractical to implement in this plugin. (At least with the way I implemented smoothing.) So it's cool that there's another plugin that implements it.

Edit: Unrelated to this issue but I thought you might want to know. While developing/using bevy_blendy_cameras (I have an example and multiple apps using bevy_blendy_cameras), I had a lot of issues related to systems ordering. Some example app where functioning fine, other not at all saying egui wanted the focus all the time. This behavior was changing at each launch of the app/example even without recompilation or with recompilation of small code change unrelated to the camera. It took me a lot of time experimenting with systems orders and debugging and I am still not sure it is correct, but the only way to get consistent behavior was to put the check_egui_wants_focus system in PreUpdate after EguiSet::BeginFrame.

Interesting. I'm guessing you used a lot of the same code? I wonder whether it's a shared problem, or something unique about your plugin. I'd expect to have bug reports about it if people were experiencing that with bevy_panorbit_camera. Have you replicated with this plugin?

thmxv commented 2 months ago

Yes, I removed the smoothing. I also added a Fly camera controller, the possibility to switch between those, the possibility to set the viewpoint (top, bottom, left, right, front and rear) and the possibility to frame the camera view around entities.

For the PR, I do not have time available this week, but I will try this week-end.

For the egui focus issue: Yes I used a lot of the same code. I did not replicated the issue with bevy_panorbit_camera but even with bevy_blendy_cameras it worked fine a lot of the time. It is only when I had multiple applications using it (some with the use of egui in the PostUpdate stage because of bevy-inspector-egui) that the issue began to appear. And even than, it was very sporadically. It was a real pain to find something that works consistently in all the apps and I am still not sure it is the correct solution.

thmxv commented 2 months ago

I implemented my "solution" in PR #81

I tried to reproduce the other issue in bevy_panorbit_camera but did not managed to do it yet.

nixpulvis commented 2 months ago

Trying out this plugin now and hitting this. I tried the bevy_egui feature as well. I'm not sure I understand this thread fully yet, but I'm curious if I'm doing anything wrong or if it's still broken.

thmxv commented 2 months ago

We will need more informations:

Edit: Sorry, I am a bit confused. I was thinking this issue was related to the @SK83RJOSH comment and video (first comment, not original post). Regarding the movement stopping when the movement is started on top of the viewport but moved on top of the GUI. Actually the original post is more related to what I call "the other issue" where the computing of "does egui wants the focus" returns wrong results (sometimes true when it is false, sometimes false when it is true). I fixed this in my code (another camera controller plugin: bevy_blendy_cameras) by changing the order of the system check_egui_wants_focus to be in PreUpdate after EguiSet::BeginFrame. But I never managed to replicate this for this plugin and I am not sure this is the "correct" solution.

Second Edit: Note that this behavior is unrelated to the egui issue linked by @SK83RJOSH and which can be worked around by checking if the pointer is over an egui Area. This work around is present in this plugin. But to the fact that sometimes it is detected that egui wants the focus event if the cursor in in the 3D viewport and not on top of any egui area. And that sometimes the contrary happens: it is detected that egui does not want the focus even if we are currently dragging an egui DragValue or clicking an egui button or something similar.

Plonq commented 2 months ago

@nixpulvis Can you clarify which issue you are having? A video would be helpful.

nixpulvis commented 2 months ago

I tried with both the bevy_egui feature enable and without it. Both experience the same behavior.

recording.webm

Plonq commented 2 months ago

@nixpulvis the recording/video is broken for me

thmxv commented 2 months ago

The video is working for me. This looks like the OP issue (which I mistakenly called "the other issue"). Where the check to see if egui wants the focus returns false even when it should be true.

Note that this behavior is normal and expected without the use of the bevy_egui feature.

With the feature enabled, it is quite rare and difficult to replicate. And the only times I replicated it, it was not consistent sometimes being present or not varying between launch of the executable (without code change/recompilation). Also it was with another camera controller crate (but with identical check/code to see if egui wants the focus).

Can you try using the code from this git repository (and not the crates.io version if it is the case) and modify the scheduling of the check_egui_wants_focus system to be in PreUpdate after EguiSet::BeginFrame to see if this fix/workaround works for you too? The changes are realy minimals. It would be possible to create a separate branch with those changes for you to check them but you would still need to use a git version of this code and not the one from crates.io.

diff --git a/src/lib.rs b/src/lib.rs
index eba6521..a23c82f 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -66,9 +66,9 @@ impl Plugin for PanOrbitCameraPlugin {
             app.init_resource::<EguiWantsFocus>()
                 .init_resource::<EguiFocusIncludesHover>()
                 .add_systems(
-                    PostUpdate,
+                    PreUpdate,
                     egui::check_egui_wants_focus
-                        .after(EguiSet::InitContexts)
+                        .after(EguiSet::BeginFrame)
                         .before(PanOrbitCameraSystemSet),
                 );
         }

As @Plonq said, there is a big chance that this issue is not coming from to this plugin but either from egui itself or from bevy_egui. And there is also a bigger chance that my "fix" is not the correct way to fix it but just a lucky workaround that worked for my cases. (I never saw this bug since this change, but it was quite rare for me even before)

nixpulvis commented 2 months ago

@thmxv unfortunately that patch on top of bdf1ccb doesn't change anything for me.

You mentioned not being able to reproduce. Perhaps looking at my code could shed some light? I'm still learning both Bevy and egui, so I'm not sure I'm doing everything correctly either, but still I think this should work. There's not much special going on here from my end. Anyway, here's where I'm using it in case it helps: https://github.com/nixpulvis/galos/tree/master/starmap

I'm also happy to try some more debugging for you.

nixpulvis commented 2 months ago

This call to try_ctx_for_window_mut is always returning None for me. Seems wrong.

https://github.com/Plonq/bevy_panorbit_camera/blob/bdf1ccbe8c479313839fa316d412e580a9ff3489/src/egui.rs#L42

thmxv commented 2 months ago

@nixpulvis, thanks, I will try your code to replicate the issue. On a quick glance I did not spotted anything that could cause it.

The call to try_ctx_for_window_mut always returning None is definitely not normal and most probably the cause of the issue but not similar to what I experienced the few times I (inconsistently) reproduced the same issue.

Edit: A search for bevy_egui in your base repo show the Cargo.lock using 2 different versions of the bevy_egui crate at the same time. This is not good at all. It happened to me once (but 2 versions of the egui crate) and caused really weird behavior. I do not know how it came to this point (I am newish to Rust) but I supposed it was because I did something wrong in my repo/workspace/crates/subcrates organization and/or my usage of cargo update and cargo upgrade. I think it is more accurate to say that I have no clue on how this happened, but it was bad.

nixpulvis commented 2 months ago

Oh wow, good call. I should have checked that. Let me resolve the issue and report back.

nixpulvis commented 2 months ago

Boom, that was it! Thanks for catching that. It looks like I was running bevy_egui = v0.29.0 and this crate only supports up to v0.28.0. No big deal for me, I'll grab the update when it's available.

Plonq commented 2 months ago

Glad a solution was found, and thanks @thmxv for helping

Plonq commented 2 months ago

Turns out try_ctx_for_window_mut was removed in bevy_egui 0.29. I've just released v0.19.3 of this crate which updates bevy_egui.