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

Add rotary input support for Android platform #88130

Merged
merged 1 commit into from
Feb 13, 2024

Conversation

AlekseyKapustyanenko
Copy link
Contributor

This PR adds Rotary input support for wear os devices. It converts RotaryInput to vertical mouse wheel event. Without it Godot does not recognise this event at all.

@AlekseyKapustyanenko AlekseyKapustyanenko requested a review from a team as a code owner February 9, 2024 09:56
@akien-mga akien-mga changed the title Add rotary input support for android platform Add rotary input support for Android platform Feb 9, 2024
@akien-mga akien-mga added this to the 4.x milestone Feb 9, 2024
@akien-mga
Copy link
Member

Assuming the implementation will be accepted by the @godotengine/android team, one step prior to merging would be to squash the commits. See PR workflow for instructions.

@AlekseyKapustyanenko
Copy link
Contributor Author

AlekseyKapustyanenko commented Feb 9, 2024

Here is an issue in proposal repo godotengine/godot-proposals#9062

if (event.isFromSource(InputDevice.SOURCE_ROTARY_ENCODER)) {
// If event came from RotaryEncoder (Bezel or Crown rotate event on Wear OS smart watches),
// convert it to vertical mouse wheel event.
verticalFactor = event.getAxisValue(MotionEvent.AXIS_SCROLL);
Copy link
Contributor

Choose a reason for hiding this comment

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

The documentation you linked to has additional logic that is not replicated here; is there a reason to leave that logic out?

You should also provide the ability for users to select whether this should be converted to a vertical event or to a horizontal event using project settings.

Copy link
Contributor Author

@AlekseyKapustyanenko AlekseyKapustyanenko Feb 10, 2024

Choose a reason for hiding this comment

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

Thank you for review. Yes you are right I need to make negation for axis value. All other logic in this documentation connected with getting some settings from configuration, like getting scroll velocity and applying RotaryInput to UI control.

You should also provide the ability for users to select whether this should be converted to a vertical event or to a horizontal event using project settings.

I have concerns about ability to change direction of this input from vertical to horizontal through project settings. As I understand we should convert some specific inputs to another more general inputs. So here I conver RotaryInput to mouse wheel input(most of miсe have only one wheel for vertical scrolling). If I added such settings It would be very disappointed, because in the same game running on PC mouse wheel would cause vertical scrolling but on smart watches crown wheel or bezel wheel would cause horizontal scrolling. Anyway In the same game such event can cause as vertical as horizontal scrolling in different scenes. So how to handle this events and which axis should be chaned by this event should be defined in specific places(inside controls or scene or other object scripts) in my opinion.

Considering fact that if developer wants to handle this events as horizaontal scrolling he can override it with a couple lines of script such configuration option contradicts this rule:

  1. Can the feature be worked around in script with a few lines?
    If the feature is only intended to save users a few lines of code it is unlikely to be accepted.

So if I have not convinced you, let me know and I will add this setting.

Copy link
Contributor

Choose a reason for hiding this comment

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

For a cross-platform game/app, I'd agree that consistency is key and for that reason the default value for the project setting should be set to vertical to simulate similar interaction on other platforms.

However a developer can also decide to target wear os devices only and as such should not be restricted by other platforms conventions. For that reason, we should provide the option to override how the rotary input is interpreted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added option to change RotaryInpur axis through project settings.

main/main.cpp Outdated
@@ -2857,6 +2857,7 @@ Error Main::setup2() {

GLOBAL_DEF_BASIC("input_devices/pointing/android/enable_long_press_as_right_click", false);
GLOBAL_DEF_BASIC("input_devices/pointing/android/enable_pan_and_scale_gestures", false);
GLOBAL_DEF_RST_NOVAL(PropertyInfo(Variant::STRING, "input_devices/pointing/android/rotary_input_scroll_axis", PROPERTY_HINT_ENUM, "Y,X"), "Y");
Copy link
Contributor

Choose a reason for hiding this comment

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

I personally suggest switching the order here as X is conventionally the first axis.

Suggested change
GLOBAL_DEF_RST_NOVAL(PropertyInfo(Variant::STRING, "input_devices/pointing/android/rotary_input_scroll_axis", PROPERTY_HINT_ENUM, "Y,X"), "Y");
GLOBAL_DEF_RST_NOVAL(PropertyInfo(Variant::STRING, "input_devices/pointing/android/rotary_input_scroll_axis", PROPERTY_HINT_ENUM, "X,Y"), "Y");

But, is it appropriate to use a string for this setting? Could it also not be a numerical value or a boolean (0 or 1), instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree the axis value should be an int instead of a string. This matches the pattern used in the codebase.

As for the labels, I think using Horizontal, Vertical would be more clear.

Copy link
Contributor

@Mickeon Mickeon Feb 13, 2024

Choose a reason for hiding this comment

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

Horizontal,Vertical. No spacing, it is not trimmed when visualized. Notably, though, there's really going to ever be just two options so the setting could also be reworked into a toggle. Unless for some reason other mouse button combinations are considered.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the correction!
I think it's okay to keep both options since it's consistent with what we usually do for axes, and it provides more clarity to the end user.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

doc/classes/InputEventMouse.xml Outdated Show resolved Hide resolved
@@ -1358,6 +1358,9 @@
<member name="input_devices/pointing/android/enable_pan_and_scale_gestures" type="bool" setter="" getter="" default="false">
If [code]true[/code], multi-touch pan and scale gestures are enabled on Android devices.
</member>
<member name="input_devices/pointing/android/rotary_input_scroll_axis" type="String" setter="" getter="" default="Y">
Defines which axis of mouse wheel RotaryInput will be mapped on Wear OS devices.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Defines which axis of mouse wheel RotaryInput will be mapped on Wear OS devices.
On Wear OS devices, defines which axis of the mouse wheel rotary input is mapped to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

main/main.cpp Outdated
@@ -2857,6 +2857,7 @@ Error Main::setup2() {

GLOBAL_DEF_BASIC("input_devices/pointing/android/enable_long_press_as_right_click", false);
GLOBAL_DEF_BASIC("input_devices/pointing/android/enable_pan_and_scale_gestures", false);
GLOBAL_DEF_RST_NOVAL(PropertyInfo(Variant::STRING, "input_devices/pointing/android/rotary_input_scroll_axis", PROPERTY_HINT_ENUM, "Y,X"), "Y");
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree the axis value should be an int instead of a string. This matches the pattern used in the codebase.

As for the labels, I think using Horizontal, Vertical would be more clear.

/**
* Define which axis of mouse wheel event will be used for mapping RotaryInput.
*/
protected open fun setRotaryInputAxis() =
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be getRotaryInputAxis().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@@ -70,6 +73,8 @@ public class GodotInputHandler implements InputManager.InputDeviceListener {
* Used to decide whether mouse capture can be enabled.
*/
private int lastSeenToolType = MotionEvent.TOOL_TYPE_UNKNOWN;

private static String rotaryInputAxis = AXIS_Y;
Copy link
Contributor

Choose a reason for hiding this comment

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

You can remove static.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No I can not. This property is used in handleMouseEvent function which is static.

// convert it to vertical mouse wheel event.
if (event.isFromSource(InputDevice.SOURCE_ROTARY_ENCODER)) {
if (rotaryInputAxis.equals(AXIS_X)) {
horizontalFactor = event.getAxisValue(MotionEvent.AXIS_SCROLL);
Copy link
Contributor

Choose a reason for hiding this comment

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

The negation should be applied here as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@@ -101,6 +106,13 @@ public void enableLongPress(boolean enable) {
public void enablePanningAndScalingGestures(boolean enable) {
this.godotGestureHandler.setPanningAndScalingEnabled(enable);
}

/**
* Set RotaryInput axis. This is Y by default.
Copy link
Contributor

Choose a reason for hiding this comment

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

This javadoc should be updated to match the added documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

float horizontalFactor = 0;

// If event came from RotaryEncoder (Bezel or Crown rotate event on Wear OS smart watches),
// convert it to vertical mouse wheel event.
Copy link
Contributor

Choose a reason for hiding this comment

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

The vertical in this sentence can be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@@ -272,6 +273,11 @@ open class GodotEditor : GodotActivity() {
*/
protected open fun enablePanAndScaleGestures() =
java.lang.Boolean.parseBoolean(GodotLib.getEditorSetting("interface/touchscreen/enable_pan_and_scale_gestures"))
/**
* Define which axis of mouse wheel event will be used for mapping RotaryInput.
Copy link
Contributor

Choose a reason for hiding this comment

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

The javadoc should be updated to match the added documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@@ -1358,6 +1358,9 @@
<member name="input_devices/pointing/android/enable_pan_and_scale_gestures" type="bool" setter="" getter="" default="false">
If [code]true[/code], multi-touch pan and scale gestures are enabled on Android devices.
</member>
<member name="input_devices/pointing/android/rotary_input_scroll_axis" type="int" setter="" getter="" default="Vertical">
Copy link
Contributor

Choose a reason for hiding this comment

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

The value for default should be 1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@@ -110,6 +110,7 @@ open class GodotEditor : GodotActivity() {
super.onGodotSetupCompleted()
val longPressEnabled = enableLongPressGestures()
val panScaleEnabled = enablePanAndScaleGestures()
val rotaryInputAxis = getRotaryInputAxis()
Copy link
Contributor

Choose a reason for hiding this comment

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

The rotaryInputAxis value needs to be passed to the GodotInputHandler below on line 121.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, I made a mistake; the changes in the GodotEditor class are not needed because the editor just used the Godot object which applies the setting for it.
So you should be able to revert the changes in this file.

* Define which axis of mouse wheel event will be used for mapping RotaryInput.
*/
protected open fun getRotaryInputAxis() =
GodotLib.getEditorSetting("input_devices/pointing/android/rotary_input_scroll_axis")
Copy link
Contributor

Choose a reason for hiding this comment

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

GodotLib.getEditorSetting(...) returns a String, so you need to use Integer.parseInt(...) to parse it to an int value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Comment on lines 60 to 61
private static final int VERTICAL_AXIS = 1;
private static final int HORIZONTAL_AXIS = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's update these constants to ROTARY_INPUT_VERTICAL_AXIS and ROTARY_INPUT_HORIZONTAL_AXIS for clarity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Comment on lines 114 to 115
rotaryInputAxis = axis;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like there is an issue with indentation here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. And All others indentation errors.

main/main.cpp Outdated
@@ -2857,7 +2857,7 @@ Error Main::setup2() {

GLOBAL_DEF_BASIC("input_devices/pointing/android/enable_long_press_as_right_click", false);
GLOBAL_DEF_BASIC("input_devices/pointing/android/enable_pan_and_scale_gestures", false);

GLOBAL_DEF_BASIC(PropertyInfo(Variant::INT, "input_devices/pointing/android/rotary_input_scroll_axis", PROPERTY_HINT_ENUM, "Horizontal,Vertical"), "1");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
GLOBAL_DEF_BASIC(PropertyInfo(Variant::INT, "input_devices/pointing/android/rotary_input_scroll_axis", PROPERTY_HINT_ENUM, "Horizontal,Vertical"), "1");
GLOBAL_DEF_BASIC(PropertyInfo(Variant::INT, "input_devices/pointing/android/rotary_input_scroll_axis", PROPERTY_HINT_ENUM, "Horizontal,Vertical"), 1);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you. Fixed

Copy link
Contributor

@m4gr3d m4gr3d left a comment

Choose a reason for hiding this comment

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

Looks great!

There is one set of changes that need to be reverted then it'll be good to merge!

@@ -110,6 +110,7 @@ open class GodotEditor : GodotActivity() {
super.onGodotSetupCompleted()
val longPressEnabled = enableLongPressGestures()
val panScaleEnabled = enablePanAndScaleGestures()
val rotaryInputAxis = getRotaryInputAxis()
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, I made a mistake; the changes in the GodotEditor class are not needed because the editor just used the Godot object which applies the setting for it.
So you should be able to revert the changes in this file.

Copy link
Contributor

@Mickeon Mickeon left a comment

Choose a reason for hiding this comment

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

Documentation is good now.

@@ -118,6 +118,7 @@ open class GodotEditor : GodotActivity() {
godotFragment?.godot?.renderView?.inputHandler?.apply {
enableLongPress(longPressEnabled)
enablePanningAndScalingGestures(panScaleEnabled)
setRotaryInputAxis(rotaryInputAxis)
Copy link
Contributor

Choose a reason for hiding this comment

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

You can also revert this change for the GodotEditor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

* On Wear OS devices, defines which axis of the mouse wheel rotary input is mapped to.
*/
protected open fun getRotaryInputAxis() =
Integer.parseInt(GodotLib.getEditorSetting("input_devices/pointing/android/rotary_input_scroll_axis"))
Copy link
Contributor

Choose a reason for hiding this comment

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

You can also revert this change for the GodotEditor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Copy link
Contributor

@m4gr3d m4gr3d left a comment

Choose a reason for hiding this comment

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

Looks great!

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Code and docs look good to me. I don't have a setup to test this locally though.1

Footnotes

  1. I acquired a Galaxy Watch 6 the other day, but didn't look at sideloading apps on it yet.

@akien-mga akien-mga merged commit afc49e5 into godotengine:master Feb 13, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

@AlekseyKapustyanenko
Copy link
Contributor Author

Code and docs look good to me. I don't have a setup to test this locally though.1

Footnotes

  1. I acquired a Galaxy Watch 6 the other day, but didn't look at sideloading apps on it yet.

I have checked this solution on my smartwatches. So let me know if I could help you to check it on your side.

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.

6 participants