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

Support AB / XY Button Swapping / Remap #5723

Closed
wants to merge 2 commits into from

Conversation

dcvz
Copy link
Contributor

@dcvz dcvz commented Sep 28, 2022

This PR primarily adds support for swapped AB & XY buttons (classic on Nintendo consoles)

It also disables SDL_HAS_CAPTURE_AND_GLOBAL_MOUSE for the __SWITCH__ platform but I'm not sure if its ok to add this check when there's no build workflow for it. Though one could likely be added using homebrew development tools. But again that's up for discussion.

@dcvz dcvz marked this pull request as draft September 28, 2022 11:13
@dcvz dcvz changed the title Support AB / XY Button Swapping and Switch's Virtual Keyboard Support AB / XY Button Swapping Sep 28, 2022
@@ -72,7 +75,7 @@
#include <TargetConditionals.h>
#endif

#if SDL_VERSION_ATLEAST(2,0,4) && !defined(__EMSCRIPTEN__) && !defined(__ANDROID__) && !(defined(__APPLE__) && TARGET_OS_IOS) && !defined(__amigaos4__)
#if SDL_VERSION_ATLEAST(2,0,4) && !defined(__EMSCRIPTEN__) && !defined(__ANDROID__) && !(defined(__APPLE__) && TARGET_OS_IOS) && !defined(__amigaos4__) && !defined(__SWITCH__)
Copy link
Contributor Author

@dcvz dcvz Sep 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The specific __SWITCH__ check could be replaced with a generic IMGUI_ define

@dcvz dcvz force-pushed the pr/upstream-soh-changes branch from 4ba455c to 06f4433 Compare September 28, 2022 12:16
@dcvz dcvz marked this pull request as ready for review September 28, 2022 12:25
@ocornut
Copy link
Owner

ocornut commented Sep 28, 2022

It also disables SDL_HAS_CAPTURE_AND_GLOBAL_MOUSE for the SWITCH

Please clarify why you need to disable SDL_HAS_CAPTURE_AND_GLOBAL_MOUSE.
Which function is failing or returning an error?
What does SDL_GetCurrentVideoDriver() return on your system?
Can you specify which SDK that is for (official or unofficial one, which one if the later).

This PR primarily adds support for swapped AB & XY buttons (classic on Nintendo consoles)

I think it would be preferable to provide a backend-agnostic and less intrusive alternative, e.g. by iterating g.InputEventsQueue[], and when event->Type == ImGuiInputEventType_Key check for event->Key.Key and swap A/B and X/Y.

@ocornut
Copy link
Owner

ocornut commented Sep 28, 2022

I think it would be preferable to provide a backend-agnostic and less intrusive alternative, e.g. by iterating g.InputEventsQueue[], and when event->Type == ImGuiInputEventType_Key check for event->Key.Key and swap A/B and X/Y.

Here's a function doing that (untested)

#include "imgui_internal.h"

void ImGui_ImplSwitch_SwapABXY(int start_event)
{
    auto& input_queue = ImGui::GetCurrentContext()->InputEventsQueue;

    for (int n = start_event; n < input_queue.Size; n++)
    {
        ImGuiInputEvent& e = input_queue[n];
        if (e.Type == ImGuiInputEventType_Key)
        {
            switch (e.Key.Key)
            {
            case ImGuiKey_GamepadFaceLeft:  e.Key.Key = ImGuiKey_GamepadFaceUp; break;   // X<>Y
            case ImGuiKey_GamepadFaceUp:    e.Key.Key = ImGuiKey_GamepadFaceLeft; break;
            case ImGuiKey_GamepadFaceRight: e.Key.Key = ImGuiKey_GamepadFaceDown; break;   // X<>B
            case ImGuiKey_GamepadFaceDown:  e.Key.Key = ImGuiKey_GamepadFaceRight; break;
            }
        }
    }
}

Usage:

// Start the Dear ImGui frame
const int input_queue_size_before = ImGui::GetCurrentContext()->InputEventsQueue.Size; // record event count before backend submit them
ImGui_ImplDX11_NewFrame();
ImGui_ImplWin32_NewFrame();

// Call between backend and main lib NewFrame():
ImGui_ImplSwitch_SwapABXY(input_queue_size_before);

ImGui::NewFrame();

@ocornut
Copy link
Owner

ocornut commented Sep 28, 2022

As the imgui core code never actually uses ImGuiKey_GamepadFaceXXX input directly but uses those alias with extra semantic:

#define ImGuiKey_NavGamepadActivate     ImGuiKey_GamepadFaceDown
#define ImGuiKey_NavGamepadCancel       ImGuiKey_GamepadFaceRight
#define ImGuiKey_NavGamepadMenu         ImGuiKey_GamepadFaceLeft
#define ImGuiKey_NavGamepadInput        ImGuiKey_GamepadFaceUp

Another and more correct solution would be to allow you to redefine those, then access to e.g. ImGuiKey_GamepadFaceUp are actually correct. In practice, I suspect this is going to be more dangerous as it likely that either core lib or extension or user code would eventually mix things up. I'll advise just swapping as suggested above.

Closing as solved but let me know if you have any more issue.

@ocornut ocornut closed this Sep 28, 2022
@dcvz
Copy link
Contributor Author

dcvz commented Sep 29, 2022

I think it would be preferable to provide a backend-agnostic and less intrusive alternative, e.g. by iterating g.InputEventsQueue[], and when event->Type == ImGuiInputEventType_Key check for event->Key.Key and swap A/B and X/Y.

Here's a function doing that (untested)

#include "imgui_internal.h"

void ImGui_ImplSwitch_SwapABXY(int start_event)
{
    auto& input_queue = ImGui::GetCurrentContext()->InputEventsQueue;

    for (int n = start_event; n < input_queue.Size; n++)
    {
        ImGuiInputEvent& e = input_queue[n];
        if (e.Type == ImGuiInputEventType_Key)
        {
            switch (e.Key.Key)
            {
            case ImGuiKey_GamepadFaceLeft:  e.Key.Key = ImGuiKey_GamepadFaceUp; break;   // X<>Y
            case ImGuiKey_GamepadFaceUp:    e.Key.Key = ImGuiKey_GamepadFaceLeft; break;
            case ImGuiKey_GamepadFaceRight: e.Key.Key = ImGuiKey_GamepadFaceDown; break;   // X<>B
            case ImGuiKey_GamepadFaceDown:  e.Key.Key = ImGuiKey_GamepadFaceRight; break;
            }
        }
    }
}

Usage:

// Start the Dear ImGui frame
const int input_queue_size_before = ImGui::GetCurrentContext()->InputEventsQueue.Size;
ImGui_ImplDX11_NewFrame();
ImGui_ImplWin32_NewFrame();

// Call between backend and main lib NewFrame():
ImGui_ImplSwitch_SwapABXY(input_queue_size_before);

ImGui::NewFrame();

Thanks for the suggestion, I'm doing it this way now. It also seems to be fine now with SDL_HAS_CAPTURE_AND_GLOBAL_MOUSE it might've been an issue with something else we were doing early on in supporting the Switch. SDL_GetCurrentVideoDriver () is reporting Switch.

Looks like all is working well! In case its still useful using the latest SDL from devKitPro

@dcvz dcvz deleted the pr/upstream-soh-changes branch September 29, 2022 16:17
@dcvz
Copy link
Contributor Author

dcvz commented Feb 9, 2023

I finally ended up getting this into our production. Unfortunately doing the swap in this manner messed with NavEnableGamepad. I’m exploring now the option of redefining what ImGui uses.

If you happen to have any other suggestions I’d appreciate it! Would be good to have a good sign off to guide the work in case it’s something that’s useful to upstream.

@ocornut ocornut added the nav keyboard/gamepad navigation label Feb 9, 2023
@ocornut
Copy link
Owner

ocornut commented Feb 9, 2023

Unfortunately doing the swap in this manner messed with NavEnableGamepad.

Can you clarify?

@dcvz
Copy link
Contributor Author

dcvz commented Feb 9, 2023

Unfortunately doing the swap in this manner messed with NavEnableGamepad.

Can you clarify?

Yeah.. still digging but essentially: doing a swap in this way is messing with the ability of navigating via gamepad buttons. It now seems to only handle the first button press then nothing more.

Before we were doing it in sdl_impl:

    #ifdef __SWITCH__
    MAP_BUTTON(ImGuiKey_GamepadFaceDown,        SDL_CONTROLLER_BUTTON_B);              // Xbox A, PS Cross
    MAP_BUTTON(ImGuiKey_GamepadFaceRight,       SDL_CONTROLLER_BUTTON_A);              // Xbox B, PS Circle
    MAP_BUTTON(ImGuiKey_GamepadFaceLeft,        SDL_CONTROLLER_BUTTON_Y);              // Xbox X, PS Square
    MAP_BUTTON(ImGuiKey_GamepadFaceUp,          SDL_CONTROLLER_BUTTON_X);              // Xbox Y, PS Triangle
    #else

My guess is that it’s the difference between changing the definition itself vs swapping the button press — but still trying to find a better working solution.

@dcvz
Copy link
Contributor Author

dcvz commented Feb 9, 2023

Ok, can confirm that this instead still has gamepad navigation working: (doing this just if platform is SWITCH)

#define ImGuiKey_NavGamepadActivate     ImGuiKey_GamepadFaceRight
#define ImGuiKey_NavGamepadCancel       ImGuiKey_GamepadFaceDown
#define ImGuiKey_NavGamepadMenu         ImGuiKey_GamepadFaceUp
#define ImGuiKey_NavGamepadInput        ImGuiKey_GamepadFaceLeft

then changing imgui_internal to do:

#ifndef ImGuiKey_NavGamepadActivate
#define ImGuiKey_NavGamepadActivate     ImGuiKey_GamepadFaceDown
#endif

#ifndef ImGuiKey_NavGamepadCancel
#define ImGuiKey_NavGamepadCancel       ImGuiKey_GamepadFaceRight
#endif

#ifndef ImGuiKey_NavGamepadMenu
#define ImGuiKey_NavGamepadMenu         ImGuiKey_GamepadFaceLeft
#endif

#ifndef ImGuiKey_NavGamepadInput
#define ImGuiKey_NavGamepadInput        ImGuiKey_GamepadFaceUp
#endif

Would you like me to PR something like this @ocornut ? and if so, should all properties be able to be overwritten?

@ocornut
Copy link
Owner

ocornut commented Feb 9, 2023

This is not the right solution, you should be able swap the buttons in the InputQueue using code similar to what I provided:
#5723 (comment)
If you don't you'll need to dig into it.

@dcvz
Copy link
Contributor Author

dcvz commented Feb 9, 2023

This is not the right solution, you should be able swap the buttons in the InputQueue using code similar to what I provided:
#5723 (comment)
If you don't you'll need to dig into it.

Yeah, this is what I had originally. But here, if I understand correctly we’re basically just replacing the button press.

The code in ImGui for game pad navigation is specifically looking for ImGuiKey_NavGamepadInput and friends. Swapping button presses won’t trigger that input then right?

@ocornut
Copy link
Owner

ocornut commented Feb 9, 2023

ImGuiKey_NavGamepadInput is defined to ImGuiKey_GamepadFaceLeft so if you swap _Left to something else it'll work.

@dcvz
Copy link
Contributor Author

dcvz commented Feb 9, 2023

ImGuiKey_NavGamepadInput is defined to ImGuiKey_GamepadFaceLeft so if you swap _Left to something else it'll work.

It doesn’t for some reason. None of the buttons trigger navigation. I’ll try to dig into it later.. technically I have two solutions that work, but was aiming to do it without modifying ImGui. I’ll see if I can get the example project setup with the reproduction.

@ocornut
Copy link
Owner

ocornut commented Feb 9, 2023

My solution doesn't modify dear imgui, and I can't comprehend how it would not work, it's a very simple thing just swapping buttons after the backend submitted them. You'll have to focus on getting this working or understanding why it doesn't.

@ocornut ocornut changed the title Support AB / XY Button Swapping Support AB / XY Button Swapping / Remap Jul 11, 2023
@ocornut ocornut mentioned this pull request Jul 11, 2023
@ocornut
Copy link
Owner

ocornut commented Dec 11, 2023

@dcvz Did you get to the bottom of that? I was thinking that I may understand why would attempt may be broken: if you have a function doing a single replacement and calling this function multiple times, one remap would lead to another. Note how my own does all the replacements in the same spot.

Also note that since 1.89.5 we have added ImU32 EventID field to ImGuiInputEvent which is an always increasing unique number per event. You may use that as a way to track which event to modify in the queue (versus my approach below of reading InputEventsQueue.Size before and after the backend NewFrame calls, but both will work).

ocornut added a commit that referenced this pull request Jul 26, 2024
…uttons, to match the typical "Nintendo/Japanese consoles" button layout when using Gamepad navigation. (#787, #5723)
@ocornut
Copy link
Owner

ocornut commented Jul 26, 2024

I have now added a runtime option io.ConfigNavSwapGamepadButtons to swap the A<>B (cancel<>activate) button behaviors for navigation at a higher level, see b3ba6b3

But I was wondering why did you also need to swap X<>Y ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backends inputs nav keyboard/gamepad navigation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants