Android: Fix joypad trigger value range #81322

johnnyw · 2023-09-05T01:05:57Z

Input::joy_axis converts trigger values to be between 0.0f to 1.0f by default. This is not needed for Android, as values are already within that range, as per Android documentation: https://developer.android.com/reference/android/view/MotionEvent#AXIS_RTRIGGER

This patch prevents this conversion on Android, which caused L2 and R2 triggers to get stuck pressed. #79263

This patch is compatible with 3.x.

Bugsquad edit:

Fixes Controller Triggers stay pressed on Android #79263

akien-mga · 2023-09-05T05:31:55Z

platform/android/android_input_handler.cpp

@@ -39,7 +39,7 @@ void AndroidInputHandler::process_joy_event(AndroidInputHandler::JoypadEvent p_e
 			Input::get_singleton()->joy_button(p_event.device, (JoyButton)p_event.index, p_event.pressed);
 			break;
 		case JOY_EVENT_AXIS:
-			Input::get_singleton()->joy_axis(p_event.device, (JoyAxis)p_event.index, p_event.value);
+			Input::get_singleton()->joy_axis(p_event.device, (JoyAxis)p_event.index, p_event.value, false);


Warrants adding a comment to clarify that Android axis events are already normalized.

Is that true for all axes?

After reading your comment, I did a bit of extra digging. It turns out, they're also already normalized on iOS. So I dug further and found that there are some Android trigger mappings that use the positive axis, and some that use the full range. I've updated the patch to take the controller mapping range into account before applying the correction. I don't have the hardware to test but I think my previous revision would have fixed some Android controllers (such as mine) and broken some others. Taking the specific device mapping into account sounds like a more robust solution.

akien-mga · 2023-09-06T19:37:27Z

core/input/input.cpp

@@ -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;


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.

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.

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?)

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.

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.

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.

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

Eoin-ONeill-Yokai

I've tested this and it seems to work without regression on my end while resolving the reported issue.

I'll approve it, but I don't think it should be merged until we can be sure that these changes are absolutely necessary to core. See the conversion in question here which asks whether this is a gamecontrollerdb.txt accuracy issue or a failure for us to interpret it correctly.

Once that comment chain is resolved, it should be clear to merge. The code looks clean otherwise.

Eoin-ONeill-Yokai · 2023-10-13T01:20:23Z

core/input/input.cpp

@@ -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;


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?)

akien-mga

Making Eoin's approval "count".

akien-mga · 2023-10-20T13:06:00Z

Looks good! Could you squash the commits? See PR workflow for instructions.

`Input::joy_axis` converts trigger values to be between 0.0f to 1.0f by default. This is not needed for Android, as values are already within that range, as per Android documentation: https://developer.android.com/reference/android/view/MotionEvent#AXIS_RTRIGGER This patch prevents this conversion on Android, which caused L2 and R2 triggers to get stuck pressed. godotengine#79263

akien-mga · 2023-10-20T20:06:37Z

Thanks! And congrats for your first merged Godot contribution 🎉

YuriSizov · 2024-01-23T15:04:09Z

Cherry-picked for 4.1.4.

johnnyw requested review from a team as code owners September 5, 2023 01:05

akien-mga reviewed Sep 5, 2023

View reviewed changes

akien-mga added bug platform:android topic:input cherrypick:3.x Considered for cherry-picking into a future 3.x release cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release labels Sep 5, 2023

akien-mga added this to the 4.2 milestone Sep 5, 2023

YuriSizov requested review from a team September 6, 2023 09:16

johnnyw requested a review from akien-mga September 6, 2023 13:53

akien-mga reviewed Sep 6, 2023

View reviewed changes

Eoin-ONeill-Yokai approved these changes Oct 13, 2023

View reviewed changes

RandomShaper modified the milestones: 4.2, 4.3 Oct 17, 2023

akien-mga modified the milestones: 4.3, 4.2 Oct 20, 2023

akien-mga approved these changes Oct 20, 2023

View reviewed changes

johnnyw force-pushed the android_fix_joypad_trigger_range branch from 28d9e1e to d413a02 Compare October 20, 2023 18:33

akien-mga merged commit e6e9b04 into godotengine:master Oct 20, 2023
15 checks passed

YuriSizov removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Jan 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Android: Fix joypad trigger value range #81322

Android: Fix joypad trigger value range #81322

johnnyw commented Sep 5, 2023 •

edited by akien-mga

Loading

akien-mga Sep 5, 2023

johnnyw Sep 5, 2023

akien-mga Sep 6, 2023

johnnyw Sep 6, 2023

Eoin-ONeill-Yokai Oct 13, 2023

johnnyw Oct 19, 2023

Eoin-ONeill-Yokai Oct 19, 2023

RandomShaper Oct 20, 2023

akien-mga Oct 20, 2023

Eoin-ONeill-Yokai left a comment

Eoin-ONeill-Yokai Oct 13, 2023

akien-mga left a comment

akien-mga commented Oct 20, 2023

akien-mga commented Oct 20, 2023

YuriSizov commented Jan 23, 2024

Android: Fix joypad trigger value range #81322

Android: Fix joypad trigger value range #81322

Conversation

johnnyw commented Sep 5, 2023 • edited by akien-mga Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eoin-ONeill-Yokai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akien-mga left a comment

Choose a reason for hiding this comment

akien-mga commented Oct 20, 2023

akien-mga commented Oct 20, 2023

YuriSizov commented Jan 23, 2024

johnnyw commented Sep 5, 2023 •

edited by akien-mga

Loading