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

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

Merged
merged 1 commit into from
Dec 16, 2024

Conversation

MJacred
Copy link
Contributor

@MJacred MJacred commented Nov 5, 2024

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

  • vendor ID (both)
  • product ID (both)
  • 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 MJacred force-pushed the get_joypad_infos_on_windows branch 4 times, most recently from 195dddc to 949f572 Compare November 5, 2024 20:27
@AThousandShips AThousandShips added this to the 4.x milestone Nov 6, 2024
@MJacred
Copy link
Contributor Author

MJacred commented Nov 6, 2024

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...

@Calinou
Copy link
Member

Calinou commented Nov 6, 2024

@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

@MJacred MJacred force-pushed the get_joypad_infos_on_windows branch from 949f572 to 924d72b Compare November 7, 2024 13:54
@MJacred
Copy link
Contributor Author

MJacred commented Nov 7, 2024

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

MJacred commented Nov 8, 2024

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 MJacred force-pushed the get_joypad_infos_on_windows branch 2 times, most recently from d6583d6 to 417b029 Compare November 8, 2024 19:16
@MJacred
Copy link
Contributor Author

MJacred commented Nov 8, 2024

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 MJacred force-pushed the get_joypad_infos_on_windows branch from 417b029 to db42a30 Compare November 9, 2024 19:26
@MJacred
Copy link
Contributor Author

MJacred commented Nov 9, 2024

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 MJacred force-pushed the get_joypad_infos_on_windows branch 2 times, most recently from b463a9a to e0f24c2 Compare November 9, 2024 21:32
@MJacred
Copy link
Contributor Author

MJacred commented Nov 9, 2024

OK, I got the name - ready for review

@MJacred MJacred marked this pull request as ready for review November 9, 2024 21:47
@MJacred MJacred requested review from a team as code owners November 9, 2024 21:47
doc/classes/Input.xml Outdated Show resolved Hide resolved
doc/classes/Input.xml Outdated Show resolved Hide resolved
doc/classes/Input.xml Outdated Show resolved Hide resolved
Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally, it works as expected on Windows 11 23H2.

Testing project: test-joypad-vendor.zip

Format for the results below:

Name { Info }

Xbox Elite Series 2 (wired):

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

DualSense (wired):

PS5 Controller: { "vendor_id": "19461", "product_id": "58892" }

Switch Pro Controller (wired):

Nintendo Switch Pro Controller: { "vendor_id": "32261", "product_id": "2336" }

Nyxi Warrior (wired):

XInput Gamepad: { "xinput_index": 1, "vendor_id": "1118", "product_id": "2850", "xinput_name": "XBOX 360 For Windows" }

doc/classes/Input.xml Outdated Show resolved Hide resolved
doc/classes/Input.xml Outdated Show resolved Hide resolved
@MJacred MJacred force-pushed the get_joypad_infos_on_windows branch from e0f24c2 to e5e1c80 Compare November 12, 2024 13:14
@MJacred
Copy link
Contributor Author

MJacred commented Nov 12, 2024

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 MJacred force-pushed the get_joypad_infos_on_windows branch 2 times, most recently from c058d7b to 91b4f5f Compare November 13, 2024 12:25
@akien-mga akien-mga requested a review from bruvzg November 28, 2024 17:11
@MJacred MJacred force-pushed the get_joypad_infos_on_windows branch from 91b4f5f to eb3c0b9 Compare December 15, 2024 12:46
@MJacred
Copy link
Contributor Author

MJacred commented Dec 15, 2024

@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).

@bruvzg
Copy link
Member

bruvzg commented Dec 15, 2024

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 #98861 (comment)).

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.

@MJacred MJacred force-pushed the get_joypad_infos_on_windows branch from eb3c0b9 to 7f80d24 Compare December 15, 2024 19:47
@MJacred MJacred requested a review from a team as a code owner December 15, 2024 19:47
@MJacred MJacred force-pushed the get_joypad_infos_on_windows branch from 7f80d24 to 01f9878 Compare December 15, 2024 19:50
@MJacred MJacred force-pushed the get_joypad_infos_on_windows branch from 01f9878 to 01a2726 Compare December 15, 2024 22:59
@MJacred
Copy link
Contributor Author

MJacred commented Dec 15, 2024

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.

@akien-mga akien-mga modified the milestones: 4.x, 4.4 Dec 16, 2024
@Repiteo Repiteo merged commit 7254761 into godotengine:master Dec 16, 2024
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Dec 16, 2024

Thanks!

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

Successfully merging this pull request may close these issues.

7 participants