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.

Sign up for GitHub

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
+45 −28
Draft

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 34 additions & 21 deletions core/input/input.cpp
View file
Original file line number Diff line number Diff line change
@@ -40,7 +40,8 @@
#include "core/os/thread.h"
#endif

static const char *_joy_buttons[(size_t)JoyButton::SDL_MAX] = {
// The naming convention follows the SDL game controller database.
static const char *_joy_button_names[(size_t)JoyButton::SDL_MAX] = {
"a",
"b",
"x",
@@ -64,7 +65,7 @@ static const char *_joy_buttons[(size_t)JoyButton::SDL_MAX] = {
"touchpad",
};

static const char *_joy_axes[(size_t)JoyAxis::SDL_MAX] = {
static const char *_joy_axes_names[(size_t)JoyAxis::SDL_MAX] = {
"leftx",
"lefty",
"rightx",
@@ -585,7 +586,7 @@ void Input::joy_connection_changed(int p_idx, bool p_connected, const String &p_
}
js.uid = uidname;
js.connected = true;
int mapping = fallback_mapping;
int mapping = fallback_mapping; // By default only used on Android for regular XR (not OpenXR).
for (int i = 0; i < map_db.size(); i++) {
if (js.uid == map_db[i].uid) {
mapping = i;
@@ -1220,21 +1221,23 @@ void Input::set_event_dispatch_function(EventDispatchFunc p_function) {
event_dispatch_function = p_function;
}

void Input::joy_button(int p_device, JoyButton p_button, bool p_pressed) {
void Input::joy_button(int p_device, JoyButton p_input_button, bool p_pressed) {
_THREAD_SAFE_METHOD_;
Joypad &joy = joy_names[p_device];
ERR_FAIL_INDEX((int)p_button, (int)JoyButton::MAX);
ERR_FAIL_INDEX((int)p_input_button, (int)JoyButton::MAX);

if (joy.last_buttons[(size_t)p_button] == p_pressed) {
if (joy.last_buttons[(size_t)p_input_button] == p_pressed) {
return;
}
joy.last_buttons[(size_t)p_button] = p_pressed;
joy.last_buttons[(size_t)p_input_button] = p_pressed;
if (joy.mapping == -1) {
_button_event(p_device, p_button, p_pressed);
// Passing on the raw, unmapped button index.
_button_event(p_device, p_input_button, p_pressed);
return;
}

JoyEvent map = _get_mapped_button_event(map_db[joy.mapping], p_button);
// Map to an actual JoyButton.
JoyEvent map = _get_mapped_button_event(map_db[joy.mapping], p_input_button);

if (map.type == TYPE_BUTTON) {
_button_event(p_device, (JoyButton)map.index, p_pressed);
@@ -1247,26 +1250,28 @@ void Input::joy_button(int p_device, JoyButton p_button, bool p_pressed) {
// no event?
}

void Input::joy_axis(int p_device, JoyAxis p_axis, float p_value) {
void Input::joy_axis(int p_device, JoyAxis p_input_axis, float p_value) {
_THREAD_SAFE_METHOD_;

ERR_FAIL_INDEX((int)p_axis, (int)JoyAxis::MAX);
ERR_FAIL_INDEX((int)p_input_axis, (int)JoyAxis::MAX);

Joypad &joy = joy_names[p_device];

if (joy.last_axis[(size_t)p_axis] == p_value) {
if (joy.last_axis[(size_t)p_input_axis] == p_value) {
return;
}

joy.last_axis[(size_t)p_axis] = p_value;
joy.last_axis[(size_t)p_input_axis] = p_value;

if (joy.mapping == -1) {
_axis_event(p_device, p_axis, p_value);
// Passing on the raw, unmapped axis index.
_axis_event(p_device, p_input_axis, p_value);
return;
}

// Map to an actual JoyAxis.
JoyAxisRange range;
JoyEvent map = _get_mapped_axis_event(map_db[joy.mapping], p_axis, p_value, range);
JoyEvent map = _get_mapped_axis_event(map_db[joy.mapping], p_input_axis, p_value, range);

if (map.type == TYPE_BUTTON) {
bool pressed = map.value > 0.5;
@@ -1558,7 +1563,7 @@ void Input::_get_mapped_hat_events(const JoyDeviceMapping &mapping, HatDir p_hat

JoyButton Input::_get_output_button(const String &output) {
for (int i = 0; i < (int)JoyButton::SDL_MAX; i++) {
if (output == _joy_buttons[i]) {
if (output == _joy_button_names[i]) {
return JoyButton(i);
}
}
@@ -1567,7 +1572,7 @@ JoyButton Input::_get_output_button(const String &output) {

JoyAxis Input::_get_output_axis(const String &output) {
for (int i = 0; i < (int)JoyAxis::SDL_MAX; i++) {
if (output == _joy_axes[i]) {
if (output == _joy_axes_names[i]) {
return JoyAxis(i);
}
}
@@ -1595,14 +1600,16 @@ void Input::parse_mapping(const String &p_mapping) {
continue;
}

String output = entry[idx].get_slice(":", 0).replace(" ", "");
String input = entry[idx].get_slice(":", 1).replace(" ", "");
String output = entry[idx].get_slice(":", 0).replace(" ", ""); // SDL button name for parsing, or metadata.
String input = entry[idx].get_slice(":", 1).replace(" ", ""); // Technical button ID for parsing.
if (output.length() < 1 || input.length() < 2) {
ERR_CONTINUE_MSG(output.length() < 2,
vformat("Invalid entry \"%s\" in mapping:\n%s", entry[idx], p_mapping));
continue;
}

if (output == "platform" || output == "hint") {
continue;
continue; // Skip metadata.
}

JoyAxisRange output_range = FULL_AXIS;
@@ -1663,7 +1670,7 @@ void Input::parse_mapping(const String &p_mapping) {
break;
case 'h':
ERR_CONTINUE_MSG(input.length() != 4 || input[2] != '.',
vformat("Invalid had input \"%s\" in mapping:\n%s", input, p_mapping));
vformat("Invalid hat input \"%s\" in mapping:\n%s", input, p_mapping));
binding.inputType = TYPE_HAT;
binding.input.hat.hat = (HatDir)input.substr(1, 1).to_int();
binding.input.hat.hat_mask = static_cast<HatMask>(input.substr(3).to_int());
@@ -1672,6 +1679,12 @@ void Input::parse_mapping(const String &p_mapping) {
ERR_CONTINUE_MSG(true, vformat("Unrecognized input string \"%s\" in mapping:\n%s", input, p_mapping));
}

if (binding.inputType == TYPE_BUTTON && binding.input.button >= JoyButton::MAX) {
// Can happen on Android and Linux:
WARN_PRINT(vformat("Too many joypad buttons, maximum %d are allowed.", (int)JoyButton::MAX));
continue; // Don't allow mapping. Would be thrown away by `Input::joy_button` anyway.
}

mapping.bindings.push_back(binding);
}

10 changes: 5 additions & 5 deletions core/input/input.h
View file
Original file line number Diff line number Diff line change
@@ -89,7 +89,7 @@ class Input : public Object {
RBSet<Key> key_label_pressed;
RBSet<Key> physical_keys_pressed;
RBSet<Key> keys_pressed;
RBSet<JoyButton> joy_buttons_pressed;
RBSet<JoyButton> joy_buttons_pressed; // Holds pseudo joy buttons, which requresents the actual button + device index.
RBMap<JoyAxis, float> _joy_axis;
//RBMap<StringName,int> custom_action_press;
bool gravity_enabled = false;
@@ -210,7 +210,7 @@ class Input : public Object {
struct JoyBinding {
JoyType inputType;
union {
JoyButton button;
JoyButton button; // The raw button index, which usually does not match the named JoyButton value.

struct {
JoyAxis axis;
@@ -227,7 +227,7 @@ class Input : public Object {

JoyType outputType;
union {
JoyButton button;
JoyButton button; // The JoyButton value `JoyBinding.input` maps to.

struct {
JoyAxis axis;
@@ -359,8 +359,8 @@ class Input : public Object {
void set_custom_mouse_cursor(const Ref<Resource> &p_cursor, CursorShape p_shape = Input::CURSOR_ARROW, const Vector2 &p_hotspot = Vector2());

void parse_mapping(const String &p_mapping);
void joy_button(int p_device, JoyButton p_button, bool p_pressed);
void joy_axis(int p_device, JoyAxis p_axis, float p_value);
void joy_button(int p_device, JoyButton p_input_button, bool p_pressed);
void joy_axis(int p_device, JoyAxis p_input_axis, float p_value);
void joy_hat(int p_device, BitField<HatMask> p_val);

void add_joy_mapping(const String &p_mapping, bool p_update_existing = false);
2 changes: 1 addition & 1 deletion core/input/input_enums.h
View file
Original file line number Diff line number Diff line change
@@ -83,7 +83,7 @@ enum class JoyButton {
PADDLE4 = 19,
TOUCHPAD = 20,
SDL_MAX = 21,
MAX = 128, // Android supports up to 36 buttons. DirectInput supports up to 128 buttons.
MAX = 128, // DirectX supports up to 14 buttons. Android supports up to 36 buttons. DirectInput supports up to 128 buttons. Linux is clamped to 128 buttons.
};

enum class MIDIMessage {
4 changes: 3 additions & 1 deletion core/input/input_event.cpp
View file
Original file line number Diff line number Diff line change
@@ -1085,7 +1085,7 @@ void InputEventMouseMotion::_bind_methods() {
///////////////////////////////////

void InputEventJoypadMotion::set_axis(JoyAxis p_axis) {
ERR_FAIL_COND(p_axis < JoyAxis::INVALID || p_axis > JoyAxis::MAX);
ERR_FAIL_COND(p_axis < JoyAxis::INVALID || p_axis >= JoyAxis::MAX);

axis = p_axis;
emit_changed();
@@ -1201,6 +1201,8 @@ void InputEventJoypadMotion::_bind_methods() {
///////////////////////////////////

void InputEventJoypadButton::set_button_index(JoyButton p_index) {
ERR_FAIL_COND(p_index < JoyButton::INVALID || p_index >= JoyButton::MAX);

button_index = p_index;
emit_changed();
}
1 change: 1 addition & 0 deletions doc/classes/InputEventJoypadButton.xml
View file
Original file line number Diff line number Diff line change
@@ -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 •
edited
Loading

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 •
edited
Loading

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)

</member>
<member name="pressed" type="bool" setter="set_pressed" getter="is_pressed" default="false">
If [code]true[/code], the button's state is pressed. If [code]false[/code], the button's state is released.
1 change: 1 addition & 0 deletions doc/classes/InputEventJoypadMotion.xml
View file
Original file line number Diff line number Diff line change
@@ -12,6 +12,7 @@
<members>
<member name="axis" type="int" setter="set_axis" getter="get_axis" enum="JoyAxis" default="0">
Axis identifier. Use one of the [enum JoyAxis] axis constants.
For unmapped joypads, this identifier does not necessarily match the [enum JoyAxis] axis constants. To check if your device is mapped, use [method Input.is_joy_known].
</member>
<member name="axis_value" type="float" setter="set_axis_value" getter="get_axis_value" default="0.0">
Current position of the joystick on the given axis. The value ranges from [code]-1.0[/code] to [code]1.0[/code]. A value of [code]0[/code] means the axis is in its resting position.