Skip to content
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

Mouse Inputs Passthrough from egui to bevy_panorbit_camera #75

Open
SK83RJOSH opened this issue May 10, 2024 · 22 comments
Open

Mouse Inputs Passthrough from egui to bevy_panorbit_camera #75

SK83RJOSH opened this issue May 10, 2024 · 22 comments

Comments

@SK83RJOSH
Copy link
Contributor

SK83RJOSH commented May 10, 2024

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:

bevy_panorbit_camera_repo_ZH28m2hvHm.mp4

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:

bevy_panorbit_camera_repo_0tAfLcCWZ4.mp4

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

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

@Plonq
Copy link
Owner

Plonq commented May 10, 2024

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.

Screen.Recording.2024-05-11.at.09.27.56.mov

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.

Screen.Recording.2024-05-11.at.09.28.52.mov

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
Copy link
Contributor Author

SK83RJOSH commented May 15, 2024

@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:

jc2_tools_nd8WBclE0C.mp4

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
Copy link
Owner

Plonq commented May 17, 2024

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 #76, which has been released in v0.18.1

@thmxv
Copy link
Contributor

thmxv commented May 24, 2024

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

  • Grab the cursor so it does not move while handling the camera
  • Warp the cursor around the viewport (Blender style). If the cursor quit the viewport by going to the left, put it on the right of the viewport. This does not completely because of toolbars or windows possibly overlapping the viewport. Also if the cursor quit the application window entirely, there is no known way (to me at least) to get the cursor position.

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

@Plonq
Copy link
Owner

Plonq commented May 24, 2024

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
Copy link
Contributor

thmxv commented Aug 12, 2024

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

  • Include the check to see if pointer is over an area in the check_egui_wants_focus system
  • Run the pan_orbit_camera system even if egui want the focus but only if ActiveCameraData is "set"
  • In the active_viewport_data system: Do not set the ActiveCameraData if the drag starts when egui wants the focus. In those case, set ActiveCameraData to default() ("unset" it)

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

  • When the drag starts on an egui area overlapping the viewport, the camera orbiting happens when the cursor goes out of the egui Area.
    This one can be tested by setting the window to immovable (movable(false)) in the egui example.

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
Copy link
Owner

Plonq commented Aug 13, 2024

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
Copy link
Contributor

thmxv commented Aug 13, 2024

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
Copy link
Contributor

thmxv commented Aug 14, 2024

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
Copy link

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
Copy link
Contributor

thmxv commented Aug 25, 2024

We will need more informations:

  • You definitely need the bevy_egui feature enabled to prevent the camera from moving when interacting with the GUI. But without this feature enabled, this issue should not be present at all (even if using old versions).
  • Are the camera controls working when starting the click in the viewport but stopping when the mouse cursor goes on top of an egui Area? Or are the camera controls simply, sometimes, not working at all even when the mouse cursor is still not on top of anything egui related? If this is the second case, this is a separate issue (the second issue I am talking about) but occasional, very rare, and difficult to replicate in "simple" games/application but can happen in much bigger/ CPU intensive code base). If this is the first case follow to the following question.
  • Have you updated bevy_panorbit_camera to version 0.19.2? If the Cargo.toml file does not contain the last pat (.2) than you need to make sure the Cargo.lock is using this last version (possibly with cargo update with some options if you only want to update this plugin and not all the dependencies). I think, I am new to Rust so do not quote/thrust me on this.

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
Copy link
Owner

Plonq commented Aug 26, 2024

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

@nixpulvis
Copy link

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

recording.webm

@Plonq
Copy link
Owner

Plonq commented Aug 28, 2024

@nixpulvis the recording/video is broken for me

@thmxv
Copy link
Contributor

thmxv commented Aug 28, 2024

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
Copy link

@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
Copy link

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

if let Some(ctx) = contexts.try_ctx_for_window_mut(window) {

@thmxv
Copy link
Contributor

thmxv commented Aug 29, 2024

@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
Copy link

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

@nixpulvis
Copy link

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
Copy link
Owner

Plonq commented Aug 30, 2024

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

@Plonq
Copy link
Owner

Plonq commented Aug 30, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants