godot icon indicating copy to clipboard operation
godot copied to clipboard

Get joypad's vendor ID, product ID and name on Windows for XInput devices.

Open MJacred opened this issue 1 year ago • 11 comments

Currently, XInput devices (Windows only) lack info on vendor, product and its name. DirectInput also does not provide vendor and product. This PR is to fix that.

TODO

  • [x] vendor ID (both)
  • [x] product ID (both)
  • [x] name (XInput only)

NOTES

  • Previously, all XInput joypads had the name XInput Gamepad, now they have proper names
  • I'm stripping "Controller (…)" from the name. Also for DirectInput devices. There's no reason for keeping this redundancy.

Ideas for alternative implementation

  • using joyGetDevCapsW, therefore loading Winmm.dll
    • I haven't tested it, but it should also be possible to get product and vendor ID using GetRawInputDeviceInfoA or DIDEVICEINSTANCE, basically making loading Winmm.dll obsolete. If I should check it out, I will. I'm not doing that right now, because I'm lacking background logic of the whole device mess (these APIs are all deprecated anyway...)
  • I also could have generated a guid, as the DirectInput code does, but I cannot verify if they are correct…
    • sprintf_s(uid, "%04x%04x%04x%04x%04x%04x%04x%04x", BSWAP16(0x03), 0, jc.wMid, 0, jc.wPid, 0, 0, 0); generated 03000000045e0000028e000000000000 and 03000000046d0000c21f000000000000

Testproject for this PR:
xinput_info.zip

output of Testproject on Windows finally

MJacred avatar Nov 05 '24 19:11 MJacred

OK, so the Godot build artifact for Windows crashes on startup. As it's not a debug build, there's no stacktrace. And I can't get Godot to build on Windows, because it just can't find mingw (yes, I followed Godot's build instructions).
I'll try finding help on Rocket Chat/Discord over the weekend. If nothing helps, I'll try cross-compilation from Linux...

MJacred avatar Nov 06 '24 16:11 MJacred

@MJacred 64-bit Windows editor binary of this PR with MinGW debug symbols (in a separate file): https://fromsmash.com/Godot-PR-test-GH-98861

Calinou avatar Nov 06 '24 17:11 Calinou

@Calinou: I think I found something: "3221225477", which is apparently an access violation..

As Godot only crashes when I plugin an XInput device, I suspect the issue lies in JoypadWindows::probe_joypads. I noticed I did a sizeof(jc) instead of sizeof(JOYCAPS) (fixed that just now), but ~~I don't think that was it~~ (it wasn't). It might be String(jc.szPname), where szPname is a char[32]...


This is the command I used

gdb --symbols=godot.windows.editor.x86_64.exe.debugsymbols --exec=godot.windows.editor.x86_64.exe

but I got

Reading symbols from C:...\godot.windows.editor.x86_64.exe.debugsymbols...
C:...\godot.windows.editor.x86_64.exe.debugsymbols: Invalid argument.

symbols argument is optional, it will find and read the file. But it will deem it invalid.

Starting Godot with XInput device plugged in

found info on error code

Starting program: C:...\godot.windows.editor.x86_64.console.exe
[New Thread 14628.0x315c]
[New Thread 14628.0x170c]
[New Thread 14628.0x3bc4]
Godot Engine v4.4.dev.custom_build.949f572c7 (2024-11-05 20:27:39 UTC) - https://godotengine.org

================================================================
CrashHandlerException: Program crashed with signal 11
Engine version: Godot Engine v4.4.dev.custom_build (949f572c70cfc15b0711ba23bd745874d8038d2f)
Dumping the backtrace. Please include this when reporting the bug to the project developer.
[1] error(-1): no debug info in PE/COFF executable
[2] error(-1): no debug info in PE/COFF executable
[3] error(-1): no debug info in PE/COFF executable
[4] error(-1): no debug info in PE/COFF executable
[5] error(-1): no debug info in PE/COFF executable
[6] error(-1): no debug info in PE/COFF executable
-- END OF BACKTRACE --
================================================================
[Thread 14628.0x170c exited with code 3221225477]
[Thread 14628.0x3bc4 exited with code 3221225477]
[Thread 14628.0x315c exited with code 3221225477]

Program terminated with signal SIGSEGV, Segmentation fault.
The program no longer exists.

Starting Godot, then plugging in XInput device

meaning of error code unknown

Starting program: C:...\godot.windows.editor.x86_64.console.exe
[New Thread 6944.0x2ba0]
[New Thread 6944.0x2a88]
[New Thread 6944.0x920]
Godot Engine v4.4.dev.custom_build.949f572c7 (2024-11-05 20:27:39 UTC) - https://godotengine.org
OpenGL API 3.3.0 NVIDIA 560.94 - Compatibility - Using Device: NVIDIA - NVIDIA GeForce GTX 970

================================================================
CrashHandlerException: Program crashed with signal 11
Engine version: Godot Engine v4.4.dev.custom_build (949f572c70cfc15b0711ba23bd745874d8038d2f)
Dumping the backtrace. Please include this when reporting the bug to the project developer.
[1] error(-1): no debug info in PE/COFF executable
[2] error(-1): no debug info in PE/COFF executable
[3] error(-1): no debug info in PE/COFF executable
[4] error(-1): no debug info in PE/COFF executable
[5] error(-1): no debug info in PE/COFF executable
[6] error(-1): no debug info in PE/COFF executable
[7] error(-1): no debug info in PE/COFF executable
[8] error(-1): no debug info in PE/COFF executable
-- END OF BACKTRACE --
================================================================
[Thread 6944.0x920 exited with code 3221226525]
[Thread 6944.0x2a88 exited with code 3221226525]
[Thread 6944.0x2ba0 exited with code 3221226525]
[Inferior 1 (process 6944) exited with code 030000002035]

MJacred avatar Nov 07 '24 13:11 MJacred

OK, the good news: I fixed my compiling on windows. The bad news is: gdb still reads the *.debugsymbols file as an Invalid argument (both from what @Calinou provided, and my own build), so the debugger isn't really helpful right now. Is GNU gdb (GDB) 11.2 (2022) too old?

Apart from that, I get this warning (even if no XInput device gets plugged in), currently I don't think that is caused by me (but I'll check later):

warning: clientcore\windows\dwm\dwmapi\attribute.cpp(185)\dwmapi.dll!00007FFC17C53647: (caller: 00007FF73D6FE436) ReturnHr(1) tid(1864) 80070057 The parameter is incorrect.
warning: clientcore\windows\dwm\dwmapi\attribute.cpp(185)\dwmapi.dll!00007FFC17C53647: (caller: 00007FF73D707AF4) ReturnHr(2) tid(1864) 80070057 The parameter is incorrect.

MJacred avatar Nov 08 '24 15:11 MJacred

Current status: it would work if I used joyGetDevCaps directly, like midiInGetDevCaps from drivers\winmidi\midi_driver_winmidi.cpp. So... I'm doing sth. wrong regarding joyGetDevCaps_t. No idea what exactly...

So help here is more than appreciated.

And the result I got when using the func directly was this: get_xinfo

As you can see, the device name JOYCAPS.szPname is disappointing. I know that Windows knows the name. While the device manager has no clue, the game controllers settings do.

windows_devices

There's also

  • JOYCAPS.szOEMVxD: EMPTY
  • JOYCAPS.szRegKey: DINPUT.DLL (quite funny, actually)

Right now, I see 2 options on how to get the name:

  1. Make use of JoypadWindows::enumCallback -> with winmm, we get product and vendor ID; then fetch them again using DirectInput, wich also gets us the name (additionally to the same IDs). Then simply map back.
    • Unless DirectInput stays consistent with winmm and renders this approach useless. I have no DirectInput device to test their output
  2. build a guid, like DirectInput does, and let Input::joy_connection_changed get the name using the internal gamecontrollerdb.
  3. Reading joypads as devices and then using some generic Windows api for reading device name (should work, but right now no idea how)

MJacred avatar Nov 08 '24 19:11 MJacred

Switched to fetching the func joyGetDevCapsW instead of the macro joyGetDevCaps, because it seems it's impossible with the macro. As *W and *A funcs are used in the engine all over the place, I'll stick to *W here.

I'll tackle getting the name next ~~(but not this weekend)~~.

MJacred avatar Nov 09 '24 19:11 MJacred

OK, I got the name - ready for review

MJacred avatar Nov 09 '24 21:11 MJacred

Applied all requested changes. Thanks for the feedback, everybody!

What do you all think about the change I mentioned in the issue's description regarding changing the joy name for XInput devices? I.e. not adding xinput_name to the dictionary. The "name" XInput Gamepad is completely redundant. We already have the XInput "GUID" __XINPUT_DEVICE__

As an example, for "Xbox Elite Series 2 (wired)", you would get

Xbox One For Windows: { "xinput_index": 0, "vendor_id": "1118", "product_id": "767" }

instead of

XInput Gamepad: { "xinput_index": 0, "vendor_id": "1118", "product_id": "767", "xinput_name": "Xbox One For Windows" }

This would make the whole thing more coherent not only with DirectInput joypads, but also with the other OS.

MJacred avatar Nov 12 '24 13:11 MJacred

@bruvzg: Could you take a look at this PR, please? And also give feedback regarding the proposal to not override the joy names for XInput devices to "XInput Gamepad", but use the real name which we can now fetch (see here).

MJacred avatar Dec 15 '24 13:12 MJacred

And also give feedback regarding the proposal to not override the joy names for XInput devices to "XInput Gamepad", but use the real name which we can now fetch (see https://github.com/godotengine/godot/pull/98861#issuecomment-2470513052).

Fully agree with it, XInput Gamepad as a name is redundant, and using actual name is better. I'll test PR tomorrow, code looks fine.

bruvzg avatar Dec 15 '24 18:12 bruvzg

Regarding testing the PR: I replaced the test project in the issue description with one that reflects the latest change (i.e. XInput joypad name).

I also noticed just now that the name set in the mapping db replaces the name coming from the OS/driver in joy_connection_changed(…, p_connected = true, …). Which makes sense as it improves naming consistency across platforms - though it won't work for xinput devices and (possibly) devices for regular xr on android (which makes use of fallback mapping).
Anyway… Input::add_joy_mapping(…, p_update_existing = true) didn't replace the joypad's name to reflect the new mapping, even though it acts similar to joy_connection_changed. Which is a bug.
Long story short: I added _set_joypad_mapping to handle mapping changes for a specific Joypad in one place.

MJacred avatar Dec 15 '24 23:12 MJacred

Thanks!

Repiteo avatar Dec 16 '24 18:12 Repiteo