fix(settings): networking tab input

[nelson137]

Fixes #933

Disabling only gamepad input seems to make the input behave as one would expect. Arrow keys, backspace, etc work like normal, and bound keys (e.g. A, D) only update the text and no longer move the caret.

Unfortunately I don't have a controller to test with, but I assume this will work with those kind of inputs too.

[MaxCWhitehead]

It's interesting to me that this fixes it, I wonder if disable_gamepad_input is not named very well?

This does seem to fix the issue with A/D, but I don't quite understand it. Anywho I'll test with gamepad at some point to make sure this doesn't impact anything outside network tab - am slightly concerned if leave the network menu gamepad will remain disabled but not sure.

Thanks!

[nelson137]

Yeah I was surprised it just worked. Re-reading through the 1 usage of disable_gamepad_input now it makes more sense:

• inputs are always parsed and the just_moved, move_direction, etc. are still set on the player controllers
• and when not disable_gamepad_input, a player controller with a move direction of left (.x < -0.1) would cause an egui::Key::ArrowLeft input event to be pushed to egui's events, for example

So the player controller is still registering A/D as left/right, but those aren't causing Arrow{Left,Right} events (which are read by the text area and would make the caret move).

am slightly concerned if leave the network menu gamepad will remain disabled but not sure

From my experimentation with egui, anything done to the context is scoped to that part of the UI, so leaving the networking tab would be in a different scope that doesn't have the EguiInputSettings set...? (Every now and then I jump into egui code but rarely understand what's going on.) And A/D still work after going into character selection.

[MaxCWhitehead]

Sorry for delay on this my gamepad was in a moving box - I tested this and I'm pretty sure the context state is global, it seems when opening the network tab gamepad input is disabled and gets stuck so on longer can navigate with it even after leaving menu.

I tried only disabling gamepad input when selecting the text field - but this means if scrolling to it with gamepad, it then gets disabled and stuck, even if the user wasn't even trying to edit it, but just scrolling around.

I think the best fix may be to add a new setting (called disable_keyboard_keys_text_editing?) (or something better lol, names are hard), that will disable any keys on keyboard only that are used in egui text editing (like wasd in this case). Then we would only disable this while text field is focused. This would allow gamepad to move off of it, but fix the WASD shifting cursor while typing.

For reference - this is how we can conditionally configure these settings only when text edit is selected (but this fix does not work, just showing using ID to check focus):

    if ui.ctx().memory(|m| m.has_focus("matchmaker".into())) {
        ui.ctx().set_state(EguiInputSettings {
            disable_gamepad_input: true,
            disable_keyboard_input: false,
        });
    } else {
        ui.ctx().set_state(EguiInputSettings {
            disable_gamepad_input: false,
            disable_keyboard_input: false,
        });
    }
    
    ...
    
        ui.add(
            egui::TextEdit::singleline(&mut state.modified_settings.matchmaking_server)
                .font(normal_font)
                .desired_width(ui.available_width() - bigger_font.size * 2.0)
                .id("matchmaker".into()),
        );

[MaxCWhitehead]

Thanks for the context on what this setting is doing and taking this stab at it, that is definitely helpful. I think we don't have enough flexibility in these settings atm for this. Going to mark this as needs changes + update the ticket with new info.

[MaxCWhitehead]

I need to type words to do review - see last comment :)

[nelson137]

I'm pretty sure the context state is global

Ah yep I'm seeing that now, when I click back to the main menu the setting is still enabled.

if scrolling to it with gamepad, it then gets disabled and stuck

Oh that makes sense, it seems obvious now lol.

Totally agree, I think the egui input handling logic needs to be "lower-level" and deal with KeyboardInputs and GamepadInputs directly to implement that. Then when the new setting is enabled (text_only_mode, keyboard_mode ??) we can selectively disable the W/A/S/D -> Up/Left/Down/Right event mappings.

[nelson137]

Actually.. should mapping W/A/S/D to egui arrow key events ever be enabled? We should always map gamepad inputs to arrow keys, but if you're on a keyboard we don't necessarily need to support W/A/S/D to move around since you can just use the actual arrow keys.

Same goes for Enter/Escape. I don't think we need to map keyboard events for egui at all since it already picks up on them, because I can comment out these lines and the confirm/back bindings still work. So if I reimpl this function I should only need to forward GamepadInputs to egui.

[MaxCWhitehead]

I think you are right - this section that maps player controls to egui inputs, we only want when those controls are from gamepad, not from keyboard.

Agreed going lower level from player control to just gamepad inputs seems like it should be the right fix. Reading from player control is doubly sending these it seems when from keyboard as egui already picks up on them. I understand better why this bug is here, thanks.

[MaxCWhitehead]

Pushed a fix for crash here if mapping does not exist (like on main menu before going to player select.

I also hit some issues with joystick input hanging around after releasing joystick, I think it is related to f549a2e - haven't identified exactly what is wrong here.

What is even weirder is if I revert that commit locally, joystick issue goes away, but when using dpad, it sometime drops input until I release and repress. I almost think this is somehow caused by the egui input hook change, as that's the only diff, but I can't see how that would have side effects. I will look into this more though, maybe that is the cause. But can't get this to repro on main, so related to something here. 🤔

This is tricky - might recommend reverting f549a2e (or we can try to fix it) - but I think there are issues beyond that, will report back later once have a bit more time to debug.

The issue on network settings tab + menu navigation all seems to work well tho so that is great.

[nelson137]

I had a feeling that commit would be fishy.. I'm glad I kept it separate from the other stuff. I find it hard to believe it's the early returns in those for loops causing the problem which really only leaves the merge_inputs refactor. But glad this does fix the bug.

This is really making me wish I had a working controller. I do have some old xbox controllers and a receiver that's supposed to work with my PC but it's super flaky. I'll try to see if I can get it working. Thanks for the patch and for testing this for me!

[MaxCWhitehead]

What is even weirder is if I revert that commit locally, joystick issue goes away, but when using dpad, it sometime drops input until I release and repress. I almost think this is somehow caused by the egui input hook change, as that's the only diff, but I can't see how that would have side effects. I will look into this more though, maybe that is the cause. But can't get this to repro on main, so related to something here. 🤔

So this issue here I was reproing on this branch (even with that commit reverted) - but I think it is unrelated to these changes as if I merge main into this branch, it stops happening. So I think this is unrelated - Sorry for the confusion, forgot that testing on main is not good comparison as this branch is a bit behind. (gilrs update in bones probably fixed something, I have had issues with my controller and dpad in the past.)

For the joystick issue (that does seem to be introduced by that commit), I think it probably is the early returning instead of checking all gamepad events causing issues.

I think it might be that for let's say the AxisPositive event: Maybe there are multiple of these in the given frame (multiple occurred since last processed, and get queued up?). It could be that the first one in &gamepad.gamepad_events is 1.0, and second one is 0.0. If we returned only the first 1.0, and the stick was released creating the 0.0, the player would keep running right - which is what I am seeing, release seemed to be missed sometimes. So I think we do have to process all and not just first. I haven't confirmed this is the case looking how this all works, but seems probable to me.

As for the other change reducing merge for alt inputs to:

                get_input_value(input1, source)
                    .filter(|v| *v != 0.0)
                    .or_else(|| get_input_value(input2, source))
                    .map(f32::abs)

I am pretty sure this is correct, and is a much more concise version of what I had written previously lol :) If want to try removing first bit mentioned and keeping this other cleanup, that sounds good to me.

[MaxCWhitehead]

This is really making me wish I had a working controller. I do have some old xbox controllers and a receiver that's supposed to work with my PC but it's super flaky. I'll try to see if I can get it working. Thanks for the patch and for testing this for me!

Yeah that's difficult - thanks for taking a stab despite not being able to 100% verify, definitely happy to help troubleshoot.

[nelson137]

I got the controller working! My PC only blue-screened twice while building jumpy (my CPU really didn't like being maxed out for more than a couple minutes) 😅.

So there's a couple problems here.

First was a bug in my condensed merge_inputs. I wrote some tests to compare it with the original and found that when input1 had Some(0.0) (and input2 had nothing) it was getting filtered out to None but that zero was important. This is now a match on the 2 inputs, which I think is even easier to reason about than the .filter(...).or_else(...).

Then I reverted the for loops back to the non-early return and it's still retaining the joystick inputs and making you skip around the menu really fast.

I think it might be that for let's say the AxisPositive event: Maybe there are multiple of these in the given frame (multiple occurred since last processed, and get queued up?).

I've made them iter in reverse and early-return the first match since that was the behavior of the original loops. But I'm thinking what we're missing here is the if statements from the original logic:

        for player_control in controls.values() {
            if player_control.just_moved {
                if player_control.move_direction.y > 0.1 {
                    push_key(events, egui::Key::ArrowUp);
                } else if player_control.move_direction.y < -0.1 {
                    push_key(events, egui::Key::ArrowDown);
                } else if player_control.move_direction.x < -0.1 {
                    push_key(events, egui::Key::ArrowLeft);
                } else if player_control.move_direction.x > 0.1 {
                    push_key(events, egui::Key::ArrowRight);
                }
            }
            if player_control.menu_confirm_just_pressed {
                push_key(events, egui::Key::Enter);
            }
            if player_control.menu_back_just_pressed {
                push_key(events, egui::Key::Escape);
            }
        }

I think the just_moved, menu_confirm_just_pressed, etc. are important to make sure it doesn't continuously push key events -- it should only push once. Since this is now lower level than the player controller I'm going to have to reimpl that just_moved/just_pressed logic in this system and see if that works.

[MaxCWhitehead]

I got the controller working! My PC only blue-screened twice while building jumpy (my CPU really didn't like being maxed out for more than a couple minutes) 😅.

Oh no lol - reminds me of my old cranky windows box, takes 15min to boot, and if I leave it on too long it tries to force update and then fails and spends another 15+min undoing it's failed update. Something is very wrong, but it hasn't died enough to get the TLC it deserves.

Anywho, this change is looking good to me as well, thanks!