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

[Input] Add comments, and elaborate on unmapped joypads in class documentations #99314

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

MJacred
Copy link
Contributor

@MJacred MJacred commented Nov 16, 2024

  • I also renamed some parameters, as they are raw input button IDs, and the code attempts to map them to output button IDs. It helps to know when you're dealing with which (especially for first-time readers of that stuff).
  • Added one ERR_CONTINUE_MSG for an error, that was silently dropped. I also have no problem removing the error print and leaving a comment such as Silently ignoring this error on purpose.
  • The PR is a draft, for now, until I've actually tested the behavior described in the classes. It should be accurate, judging from the code

@@ -12,6 +12,7 @@
<members>
<member name="button_index" type="int" setter="set_button_index" getter="get_button_index" enum="JoyButton" default="0">
Button identifier. One of the [enum JoyButton] button constants.
For unmapped joypads, this identifier does not necessarily match the [enum JoyButton] button constants. To check if your device is mapped, use [method Input.is_joy_known].
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this mean that the value of button_index could even be none of the existing constants (e.g. 30). Is that even possible in the code? Assigning an arbitrary integer to an enum will trigger a warning in GDScript, and this note may encourage it. Just worth keeping in mind.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's technically possible in C++

I want to test (this weekend) it by removing the mapping for my controller and then checking the outputs. Can be tested w/o a custom build. Just "hoping" that my joypad's technical numbers are "out of range" to verify this

Copy link
Contributor Author

@MJacred MJacred Nov 17, 2024

Choose a reason for hiding this comment

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

Ok, I could test on Linux, and it doesn't go out-of-range. But that is only because the indices start at 0, and are manually incremented. And the technical max number is 767 (as taken from the Linux kernel code). And I have no controller with that many buttons. So yeah, it could happen there, if you have a controller with enough buttons.
But the button index order did change for unmapped joypads, as expected, so the "JoyButton" has basically no reliability for those.

It seems this out-of-range issue is somewhat expected looking at some code, such as InputEventJoypadButton::as_text. Or it was a "just in case" safety implementation, who knows.

On iOS and macOS, it seems they hard-coded to use JoyButton and JoyAxis. So that's safe.

On Web, it seems they clamp it at 16 buttons and 10 axes (in godot_js_input_gamepad_sample_get). So that's safe as well.

On Android, there's getGodotButton, where default could result in any button index of value getMaxKeyCode (is 318 atm) - 168.

On Windows, it's clamped to 17 buttons for XInput (even though there are only 14 supported according to the docs, I made a PR to fix that, though I'm curious to check it out, because XInput officially does not seem to support the GUIDE button), and 128 for DirectInput.

So, I'll test soon on Windows for XInput devices (I have no DirectInput device), and then I reckon I'll update the class docs and give some info per target platform.

And I think I'll hard-code some fake high button index and test GDScript on Linux and see what happens. I expect nothing, because the binding for the property is int: ADD_PROPERTY(PropertyInfo(Variant::INT, "button_index"), "set_button_index", "get_button_index");

Copy link
Contributor Author

@MJacred MJacred Nov 17, 2024

Choose a reason for hiding this comment

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

OK, I overlooked ERR_FAIL_INDEX((int)p_button, (int)JoyButton::MAX); in Input::joy_button, so Godot will catch technical button indices that are out-of-range.

The problem is: you can map joypads with more than 128 buttons, but then Input::joy_button throws them away. Affected are Android and Linux. I guess I'll make a warning print in the mapping func (in this PR)

@AThousandShips AThousandShips added this to the 4.x milestone Nov 16, 2024
@MJacred MJacred force-pushed the input_comments branch 3 times, most recently from 073e7ed to a21469d Compare November 17, 2024 14:07
@MJacred
Copy link
Contributor Author

MJacred commented Nov 17, 2024

I fixed the check in InputEventJoypadButton::set_axis, which allowed JoyAxis::MAX as a value…

Added its counterpart in InputEventJoypadButton::set_button_index. This was noticed before: #97068 (comment)

Should the idea of consolidating parameter validation for input events be discussed here? @Sauermann @akien-mga @Mickeon

I did a reference check and the values have already been checked against MAX before they reach this point, usually because they are used to access an array that has MAX-size.

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.

3 participants