Fixed _input being falsely detected when a Godot GUI element is on top of the other element.

[jacobsky]

This PR closes #7

A note about the changes. As you noted in #7 the correct place to put the input is _gui_input as that respects the input propagation and resolves the issue when multiple elements are overlapping with one another.

The one use-case this may break is anyone that expects two overlapping EGUI controls to both accept input will end up with broken behaviors between these UIs. This may break some expectations.

In addition, it appears that _gui_input() doesn't require matching the mouse position offset from the screen, it seems to get the correct coordinates, so I removed that code. (it still needs to be mapped to egui::Vec2 though.

Please let me know if you have any questions.

[setzer22]

The one use-case this may break is anyone that expects two overlapping EGUI controls to both accept input will end up with broken behaviors between these UIs. This may break some expectations

I'm a bit concerned about this, because I've been relying on this exact behavior for some features. For instance, having a fullscreen Control showing a puffin_egui window would prevent input from falling through, even when the mouse is not hovering the window (see pic below). Any fullscreen Control using egui::Window will have this problem, if I understand this correctly.

There should be a way to let godot know you want a click to "fall through" when the mouse is hovering empty space. That would be the ideal way to handle this, but I'm not sure godot's event system supports this. Is there a way to ensure the mouse event propagates to other nodes? From the egui side, I think there are ways to detect when the pointer is hovering some GUI element (e.g. using the ui rect for the last frame).

That being said, I admit the current state of input support is no better than this. If we can't find a solution that allows both workflows, we could at least make this a configurable setting, so that you could choose between _input or _gui_input modes. But I'd rather have something that works well all the time, of course!

[jacobsky]

I see. I can see where that is a concern. From testing, this appears to use the Godot input event propagation.

Generally speaking, this is all controlled based on which node has focus and what the focus mode and mouse filtering settings are.

In the example above, the puffin profiler gui will be stop all input events as long as Godot's mouse filter setting for the node is set to either "MOUSE_FILTER_STOP" where it will consume all mouse events or "MOUSE_FILTER_PASS" where it will only consume it if it is actually handled.

Regarding keyboard input, keyboard input also requires that the Control be focused in order for the keyboard events to propagate.

In this way, the real workflow breakage is mainly returning to using the Godot editor's settings in order to manage this. I think this is appropriate for most users.

One thing I did just realize is that I need to do another quick push to make the defaults for the example and the stylist to use click as the "focus_mode"

[setzer22]

The problem is a bit more subtle than that. To my understanding, there's no way the user can do the right thing, even when in control of the whole scene tree. Let me ellaborate:

In Godot, mouse filter can either be set as ignore, pass or stop. The difference between pass and stop is whether mouse events "bubble up" to their parents. Godot roughly proceeds as follows:

1- Select the bottommost Control node that's in front of the screen where mouse input occurs, let's call it target
2- The target's _gui_input notification is sent.
3- If the mouse_filter is set to stop, propagation ends here.
4- If the mouse_filter is set to pass, we go back to step 2, but setting target to the current target's parent.

At no point, event propagation considers siblings on the scene tree, it only travels up. This becomes an issue with GodotEgui's workflow: What people are usually doing is having one fullscreen Control node to draw the whole UI. Egui has its own notion of panels, buttons and windows, but to Godot, the whole egui rect is like a huge button covering the whole screen.

So, if at any point, the GodotEgui node takes a _gui_input mouse event, we can guarantee no siblings will ever get it. This is where my puffin example comes in: As long as the puffin control exists, its _gui_input will get called, and the game's GUI will never take any input. Even if the puffin window may be covering a portion of the screen, Godot does not know that.

To illustrate my point, here's a very simple Godot project setup with three nodes, a Parent node with two children ChildA and ChildB. ChildB is only drawing to the top-left corner of the screen, but its Control rect is covering the full screen. ChildA is actually drawing to the whole screen. Everyone has their mouse_filter set to Pass. When running the example, ChildA never gets a mouse event, but ChildB and parent do.

Input_Mouse_Example.zip

I'm not sure how can we go about solving this in a way that's completely transparent to the user, honestly 🤔 Something that would almost work is setting _mouse_filter to ignore when the mouse is not hovering an egui control. But then you start loosing all subsequent mouse events, because there will be no more pointer events that give egui the chance to notify it's using the pointer again. Something more complex along those lines might work.

Otherwise, we need to consider this a tradeoff. You can either use _input, and always get mouse input but manage overlapping yourself in code, or use _gui_input and carefully structure your scene tree to get around its drawbacks (e.g. ensure all your non-egui controls are on top, set mouse filters for every control node instead of relying on defaults...)

[jacobsky]

Based on our conversation in discord, there's still a lot more that probably needs to be done, so I'm downgrading this PR to a work in progress.

I'll work on making the following changes

• Change GodotEgui to no longer handle input directly. Instead, it will have a method called handle_godot_input() which will hold all the egui adaptor code for the input, it will be up to the users to determine when to do input.
• Modify the existing samples to use this.
• Update the existing documentation to reflect this.
• test _input, _gui_input and _unhandled_input to ensure that everything works.
• Create a sample demonstrating multiple overlapping UserEgui nodes (the actual guis that users will use)
• Document how users should interact with this code.

[jacobsky]

I've done the initial refactor. The next step is to add this to the documentation to help ensure that users understand that they are responsible for passing the input events to GodotEgui as well as reacting to and marking the input as handled or accepted to prevent/allow propagation.

[jacobsky]

I've fixed up the remaining stuff. @setzer22 Let me know what you think when you get a chance :)

[jacobsky]

One thing I'm not completely satisfied with is how at the moment, for generic use-cases, you have to always implement

#[export]
pub fn _gui_input(&mut self, owner: TRef<Control>, event: Ref<InputEvent>) {
    let gui = unsafe { self.gui.as_ref().expect("GUI initialized").assume_safe() };
    gui.map_mut(|gui, instance| {
        gui.handle_godot_input(instance, event, true);
        if gui.mouse_was_captured(instance) {
            owner.accept_event();
        }
    })
    .expect("map_mut should succeed");
}

For every class, which strikes me a kind of boiler-platey. Maybe this could use some kind of macro to automatically implement a generic solution?

It seems like a lot of work to add a proc macro though 🤣

I wonder if a fairly generic normal macro would be helpful. Something like handle_gui_input(self.gui) and handle_input(self.gui) could alleviate this.

🤔

Maybe this sort of thing should be added in a separate PR to avoid adding too much complexity right now.

[setzer22]

Sorry about the delay! The changes look good to me overall 🙂 There's just one thing I feel remains to be discussed:

It's not ideal forcing users to have a wrapping Control to every GodotEgui node that forwards input events, and indeed it looks a bit boilerplate-y. However I don't think a macro would be very useful in abstracting this implementation detail away: The main drawback I see is that a GodotEgui node is not useful on its own anymore. Now it needs a parent node that forwards its events. Creating this parent node is where the boilerplate lies in, but this can't be done with a Rust macro alone, you still need a way to create the node in Godot's scene tree.

How adding a way to opt into the old behavior. Let me ellaborate: We add a new parameter input_process_type which can have one of the following values: Input, GuiInput or None.

• None :: Avoid any automatic event processing by the GodotEgui node, and forward that instead with custom logic.
• Input :: This would register an _input handler and call input on it, just like the original code did.
• GuiInput :: Same as above, but the _gui_input handler is registered instead.

The default would still be None, so people are forced to make a choice if they want this behaviour to happen (no magic, unless explicitly requested).

I believe this would be a good way to get rid of the boilerplate in some cases. It would also make the migration process way simpler for those of us who have a substantial amount of code using the old style and no incentive to change (I can't use _gui_input, I'll always use _input for my projects due to its drawbacks).

I'm not sure how many people are using this library right now, but I believe it's unfair to ask our users for this kind of refactor for a minor version bump: People that are happy with the current state of things should be able to just "toggle a flag" to restore the old behavior.

Anyway, I don't think any of this is a blocker for merging the PR, which looks good already! But for an official 0.2.0, I would like to see a solution that requires a lower migration effort for existing projects.

[jacobsky]

@setzer22 This now has the flag, if you have any other changes you'd like, let me know.

[setzer22]

Thanks! I'll try upgrading my project to the latest release ASAP to confirm this is working (I'm waiting on godot-rust/gdnative#791 to be merged for that)

I think this should be good for a merge. If there's any other changes we can always open another PR :)

[jacobsky]

@setzer22 Sounds great! I'll go ahead and merge this, with the new toggle there's technically no breaking changes (you just need to set the one flag and then you should be good to go). Feel free to ping me on the discord if you have any other changes you'd like me to look into.