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

Android: Fix joypad trigger value range #81322

Merged
merged 1 commit into from
Oct 20, 2023
Merged

Android: Fix joypad trigger value range #81322

Show file tree
Hide file tree
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
8 changes: 5 additions & 3 deletions core/input/input.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1075,7 +1075,8 @@ void Input::joy_axis(int p_device, JoyAxis p_axis, float p_value) {
return;
}

JoyEvent map = _get_mapped_axis_event(map_db[joy.mapping], p_axis, p_value);
JoyAxisRange range;
JoyEvent map = _get_mapped_axis_event(map_db[joy.mapping], p_axis, p_value, range);

if (map.type == TYPE_BUTTON) {
bool pressed = map.value > 0.5;
Expand Down Expand Up @@ -1115,7 +1116,7 @@ void Input::joy_axis(int p_device, JoyAxis p_axis, float p_value) {
if (map.type == TYPE_AXIS) {
JoyAxis axis = JoyAxis(map.index);
float value = map.value;
if (axis == JoyAxis::TRIGGER_LEFT || axis == JoyAxis::TRIGGER_RIGHT) {
if (range == FULL_AXIS && (axis == JoyAxis::TRIGGER_LEFT || axis == JoyAxis::TRIGGER_RIGHT)) {
// Convert to a value between 0.0f and 1.0f.
value = 0.5f + value / 2.0f;
}
Expand Down Expand Up @@ -1221,7 +1222,7 @@ Input::JoyEvent Input::_get_mapped_button_event(const JoyDeviceMapping &mapping,
return event;
}

Input::JoyEvent Input::_get_mapped_axis_event(const JoyDeviceMapping &mapping, JoyAxis p_axis, float p_value) {
Input::JoyEvent Input::_get_mapped_axis_event(const JoyDeviceMapping &mapping, JoyAxis p_axis, float p_value, JoyAxisRange &r_range) {
JoyEvent event;

for (int i = 0; i < mapping.bindings.size(); i++) {
Expand Down Expand Up @@ -1267,6 +1268,7 @@ Input::JoyEvent Input::_get_mapped_axis_event(const JoyDeviceMapping &mapping, J
case TYPE_AXIS:
event.index = (int)binding.output.axis.axis;
event.value = value;
r_range = binding.output.axis.range;
Copy link
Member

Choose a reason for hiding this comment

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

Isn't the switch below on axis.range meant to handle this exact issue already? I'm not deeply familiar with that code but I wonder why this normalisation is handled in two different locations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm also not super familiar with what's going on here, but my impression is that the private method is converting to match the gamecontrollerdb.txt entry, and joy_axis is normalizing consistently for 0.0 to 1.0. On Android and iOS we should be able to bypass this entirely for the triggers. I wasn't sure about the private method conversion so I didn't want to mess with it, so I figured using the binding as the source of truth for the trigger-specific normalization was a reliable way to get it working.

Maybe the private method can branch on if it's an L/R trigger inside the switch and do the proper conversion for those specific axes only once? Because it seems like the switch in joy_axis is correcting an error introduced by the switch in _get_mapped_axis_event not being specific enough.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've tested this and it works well and makes some sense, but I'm not sure I entirely get what you mean by this comment. Are you trying to say that you're avoiding updating gamecontrollerdb.txt and that it isn't configured to the right axis range? Or are you trying to say that the range is correct in gamecontrollerdb.txt and that this PR is making sure that Godot converts from those values appropriately (which it wasn't before?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe that the gamecontrollerdb.txt entry is correct (I recall cross-checking across some other repos), but Godot wasn't correctly converting them specifically in the case of the triggers.

Copy link
Contributor

Choose a reason for hiding this comment

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

In which case then it all looks correct to me. I see that @RandomShaper has already marked it for 4.3 milestone but let's focus on getting this into 4.3/master ASAP (after 4.2 release) as it can be understandably annoying if certain controllers don't work correctly on Android. We could then introduce it as a hotfix if it checks out.

Copy link
Member

Choose a reason for hiding this comment

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

Well, we decided to lie on the (maybe too) safe side. If you have tested it and think the changes are good, let's reschedule for 4.2.

Copy link
Member

Choose a reason for hiding this comment

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

This seems ok for 4.2 if we merge it now so we get at least a couple betas/RCs of testing.

if (binding.output.axis.range != binding.input.axis.range) {
switch (binding.output.axis.range) {
case POSITIVE_HALF_AXIS:
Expand Down
2 changes: 1 addition & 1 deletion core/input/input.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,7 @@ class Input : public Object {
Vector<JoyDeviceMapping> map_db;

JoyEvent _get_mapped_button_event(const JoyDeviceMapping &mapping, JoyButton p_button);
JoyEvent _get_mapped_axis_event(const JoyDeviceMapping &mapping, JoyAxis p_axis, float p_value);
JoyEvent _get_mapped_axis_event(const JoyDeviceMapping &mapping, JoyAxis p_axis, float p_value, JoyAxisRange &r_range);
void _get_mapped_hat_events(const JoyDeviceMapping &mapping, HatDir p_hat, JoyEvent r_events[(size_t)HatDir::MAX]);
JoyButton _get_output_button(String output);
JoyAxis _get_output_axis(String output);
Expand Down
Loading