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

Android Stylus pressure and tilt support. #80644

Merged
merged 1 commit into from
Sep 17, 2023

Conversation

Distantz
Copy link
Contributor

@Distantz Distantz commented Aug 14, 2023

Modified the Android input code to send pressure and tilt information.

Bugsquad edit:

@Distantz Distantz requested a review from a team as a code owner August 14, 2023 23:58
@Distantz
Copy link
Contributor Author

Distantz commented Aug 14, 2023

Have verified that this works on my Galaxy Tab S7.

@akien-mga
Copy link
Member

Thanks for the contribution! Did you check if this solves any existing bug report or feature proposal?

Please make changes to the PR by amending the commit and force pushing the branch, so style fixes for example become part of the main commit, instead of follow up fixup commits. See PR workflow for details.

@Distantz
Copy link
Contributor Author

I believe it satisfies the Android half of godotengine/godot-proposals#735.

@akien-mga
Copy link
Member

akien-mga commented Aug 15, 2023

I believe it satisfies the Android half of godotengine/godot-proposals#735.

Thanks! Please also have a look at #49609 for comparison - it looks like your PR is more feature complete as it also implements tilt and not just pressure. The discussion in the PR might still be useful to make this one fully ready to merge.

CC @ModProg @HEAVYPOLY

@Distantz
Copy link
Contributor Author

Had a look at #49609, seem's like they had a couple of issues with getting the stylus to register up and down events. From what I can see testing right now, my PR does emit these events.

@Distantz
Copy link
Contributor Author

image
Logcat output from my tablet for a stylus press, move and release.

@akien-mga
Copy link
Member

Have verified that this works on my Galaxy Tab S7.

Did some testing to get another data point, before remembering that my device is also a Galaxy Tab S7 :P

Anyway, I can confirm it seems to work well on that device, with a generic stylus I got with it.

Screenshot_20230815_110059_Godot Editor 4 (debug)

Screenshot of received events while writing my name with the stylus.

@Distantz
Copy link
Contributor Author

Good to know its not a fluke haha

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Tested functionality, works fine for a basic test (didn't try it on a real app that would make use of pressure and tilt). Code style looks good.

I'll let @godotengine/android review the actual implementation for Android.

@Calinou
Copy link
Member

Calinou commented Aug 15, 2023

Do we have a testing project that features pen drawing somewhere? (If not, this could be worth adding to godot-demo-projects.)

@Distantz
Copy link
Contributor Author

I've got a project I've been using to test this that displays the pressure (in both text and colour background) alongside the tilt, can post that here if needed.

@Calinou
Copy link
Member

Calinou commented Aug 15, 2023

I've got a project I've been using to test this that displays the pressure (in both text and colour background) alongside the tilt, can post that here if needed.

Please upload it here 🙂

@Distantz
Copy link
Contributor Author

PressureTest.zip

Here you are. Hopefully it isn't too sloppy code-wise.

@ModProg
Copy link
Contributor

ModProg commented Aug 15, 2023

I'll probably not be able to experiment with this before the beginning of September, unless someone builds the APK for me 😬.

My devicd would be a Note 10 lite, i.e. a smart phone.

@Distantz
Copy link
Contributor Author

@ModProg I am able to give you the APK, but GitHub only allows max 25mb as attachments. Any way to send it to you?

@Zireael07
Copy link
Contributor

Won't a zipped apk fit under the limit?

@Distantz
Copy link
Contributor Author

Distantz commented Aug 15, 2023

Mine is 37mb at max compression (for reference, the raw APK is 40mb), although there might be a way to shrink it in Godot that I don't know about?

@Calinou
Copy link
Member

Calinou commented Aug 15, 2023

Mine is 37mb at max compression (for reference, the raw APK is 40mb), although there might be a way to shrink it in Godot that I don't know about?

It can be shrunk by only including a single architecture. arm64 nowadays is ubiquitous on phones and tablets, with x86_64 only being relevant for Chromebooks and emulators on PC. Some low-end phones/tablets in use today are still armv7, but their count is only decreasing as time goes on.

Here's the release export template APK I built from this PR (arm64 only): android_release.apk.zip

Exported project APK: Android Pressure Test.apk.zip

@Distantz
Copy link
Contributor Author

Legendary, @ModProg this is for you 👍

@Calinou
Copy link
Member

Calinou commented Aug 15, 2023

I've tested the project locally. Both pressure and tilt reporting work as expected (Samsung Galaxy Z Fold4, Android 13, S Pen Fold Edition).

Note that if the pen appears to do nothing, make sure it's enabled in the system settings.

@ModProg
Copy link
Contributor

ModProg commented Aug 15, 2023

I think I ran into a similar problem I had when implementing this IIRC.

There is only an output when I start moving, simply pressing down doesn't do anything independent from how hard I press down.

Screen_Recording_20230815_193156_Android.Pressure.Test.mp4

@ModProg
Copy link
Contributor

ModProg commented Aug 15, 2023

Slow movements also don't trigger it. Thou I haven't looked at the Testprojekt, might be it is written to behave this way?

@Distantz
Copy link
Contributor Author

Distantz commented Aug 15, 2023

Only InputEventMouseMotion has a pressure field, so the InputEventMouseButton events don't have any pressure information. I think there must be some sort of threshold (for displacement from the beginning position) built-in for triggering the motion events. The test project I supplied only displays motion events

@ModProg
Copy link
Contributor

ModProg commented Aug 15, 2023

One thing I noticed in my testing, was that pen touching without moving wasn't detected at all, because Samsung introduced custom actions: ModProg@cbce6a1

Edit: This could also have been related to the eraser thinking about it now.

2 years is a long time and I'm not 100% sure, but would be good if we supported the action button an s-pen has on the side as well.

@Distantz
Copy link
Contributor Author

I think supporting the stylus buttons is a different issue and should probably be in its own PR. If we do support the Samsung codes, could that potentially cause issues relating to duplicate events?

@ModProg
Copy link
Contributor

ModProg commented Aug 16, 2023

I think supporting the stylus buttons is a different issue and should probably be in its own PR. If we do support the Samsung codes, could that potentially cause issues relating to duplicate events?

IIRC they just didn't send the nornal codes. But it would also be interesting whether e.g. Huawei sends the same or different codes. I don't think I ever researched that.

@ModProg
Copy link
Contributor

ModProg commented Aug 16, 2023

But I agree, this is not necessarily a blocker for this PR. When I hopefully continue the project I wanted pen support for, I'll revisit this and can add those then.

@Distantz
Copy link
Contributor Author

I see, thank you for the clarification!

@akien-mga akien-mga requested a review from a team August 17, 2023 10:08
@akien-mga akien-mga modified the milestones: 4.x, 4.2 Aug 17, 2023
@@ -433,7 +433,7 @@ static boolean isMouseEvent(MotionEvent event) {
}

private static boolean isMouseEvent(int eventSource) {
boolean mouseSource = ((eventSource & InputDevice.SOURCE_MOUSE) == InputDevice.SOURCE_MOUSE) || ((eventSource & (InputDevice.SOURCE_TOUCHSCREEN | InputDevice.SOURCE_STYLUS)) == InputDevice.SOURCE_STYLUS);
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the reason for removing the SOURCE_TOUCHSCREEN check?

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 couldn't get it to work without removing that. I think SOURCE_TOUCHSCREEN is used for fingers also?

Copy link
Member

Choose a reason for hiding this comment

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

Should we try it like this, or investigate further?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From what I can test, there seems to be no regression in touch handling when removing that SOURCE_TOUCHSCREEN, I think it should be fine to try it like this.

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this change has introduced the following regression: #88621

@m4gr3d
Copy link
Contributor

m4gr3d commented Aug 17, 2023

Asides from the comment above the logic looks good!

Only InputEventMouseMotion has a pressure field, so the InputEventMouseButton events don't have any pressure information.

InputEventMouseMotion extends from InputEventMouseButton, so it's worth considering whether we should move the tilt and pressure fields into InputEventMouseButton instead.

@Distantz Thoughts?

@Distantz
Copy link
Contributor Author

I think that could definitely be a good change, but I think it would require some conversation with the other platforms. For Android specifically, it should be easy to implement.

@akien-mga
Copy link
Member

Asides from the comment above the logic looks good!

Only InputEventMouseMotion has a pressure field, so the InputEventMouseButton events don't have any pressure information.

InputEventMouseMotion extends from InputEventMouseButton, so it's worth considering whether we should move the tilt and pressure fields into InputEventMouseButton instead.

To be clear, the inheritance tree is like this:

  • InputEventMouse
    • InputEventMouseButton
    • InputEventMouseMotion

So indeed the tilt and pressure properties could be made available to both Button and Motion events if moved to the base InputEventMouse.

This is IMO outside the scope of this PR though, and we'd need to evaluate whether moving properties to a parent class like this will break API or ABI compatibility.

@akien-mga akien-mga requested a review from m4gr3d September 4, 2023 07:09
@akien-mga
Copy link
Member

Let's proceed and merge, if @m4gr3d still has concerns on the change to isMouseEvent, we can improve it later on.

@akien-mga akien-mga merged commit 4b2fb36 into godotengine:master Sep 17, 2023
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

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.

7 participants