Fix Input.get_joy_info() regression after the SDL input driver PR#108214
Fix Input.get_joy_info() regression after the SDL input driver PR#108214Repiteo merged 1 commit intogodotengine:masterfrom
Input.get_joy_info() regression after the SDL input driver PR#108214Conversation
adamscott
left a comment
adamscott
left a comment
There was a problem hiding this comment.
Finally, I found some issues.
drivers/sdl/joypad_sdl.cpp
Outdated
| if (SDL_GetJoystickPlayerIndex(joy) >= 0) { | ||
| // For XInput controllers SDL_GetJoystickPlayerIndex returns the XInput user index. | ||
| joypad_info["xinput_index"] = itos(SDL_GetJoystickPlayerIndex(joy)); | ||
| } |
There was a problem hiding this comment.
Should we name the key to be player_index instead, as it is named by SDL? I wonder if we could do like SDL and just return 0 on platforms that it doesn't work on.
| if (SDL_GetJoystickPlayerIndex(joy) >= 0) { | |
| // For XInput controllers SDL_GetJoystickPlayerIndex returns the XInput user index. | |
| joypad_info["xinput_index"] = itos(SDL_GetJoystickPlayerIndex(joy)); | |
| } | |
| joypad_info["player_index"] = itos(SDL_GetJoystickPlayerIndex(joy)); |
There was a problem hiding this comment.
Just realized that's because it was named like this before. Could we add both for the time, marking xinput_index as deprecated?
There was a problem hiding this comment.
What is the benefit of renaming it? Just to match the naming in SDL?
There was a problem hiding this comment.
Maybe it's not that urgent. I just felt that if SDL has a different name, then it would maybe make more sense to follow its standard. (i.e. they must know what they're doing)
There was a problem hiding this comment.
I'll note that the SDL documentation specifies an unsupported value as -1 (zero-based index), while our original implementation for xinput_index only assigned a value for XInput devices. That is: if it didn't use the value, it was simply excluded
I think we should leave the name alone, and keep the SDL_GetJoystickPlayerIndex(joy) >= 0 conditional as-is, as that reflects the 4.4 behavior. We can investigate a rename in 4.6, alongside possibly passing all generic attributes in the call (unsupported platforms would have them all set to -1, or whatever SDL defines as default)
adamscott
changed the title
Input.get_joy_info() regression after the SDL input driver PR
|
I'm testing your PR on macOS and it seems that the dictionary returned by |
|
Thank you for your review, adamscott! |
Nintorch
force-pushed
the
fix-joypad-vendor-product
branch
from
626e7f8 to
9848f2e
Compare
|
Thank you for your review, AThousandShips! I fixed the documentation according to your changes! |
Nintorch
force-pushed
the
fix-joypad-vendor-product
branch
from
9848f2e to
e9386bd
Compare
SDL input driver did not have the "xinput_index", "raw_name", "vendor_id" and "product_id" fields for this method and exposed an additional, essentially useless for the users "mapping_handled" field. This commit fixes these issues.
Nintorch
force-pushed
the
fix-joypad-vendor-product
branch
from
e9386bd to
f28acf9
Compare
|
Thank you for your review, Repiteo! |
|
Thanks! |
6 participants
While making the SDL input driver PR I didn't check what dictionary fields
Input.get_joy_info()should return, so they went unnoticed until now.The fields in question are
"xinput_index","raw_name","vendor_id","product_id"and"steam_input_index", and my PR also exposed an additional, essentially useless for the users"mapping_handled"field, that is used internally to skip Godot's gamepad mapping system (because SDL already handles that for us).This PR should fix these issues.
Production edit: Fixes #110067