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: Add support for cursor icons #44201

Merged
merged 1 commit into from
May 19, 2021

Conversation

thebestnom
Copy link
Contributor

@thebestnom thebestnom commented Dec 8, 2020

Adds support for cursor shapes on Android

Custom cursor shape is way harder, there need to be a way to move image from native to java by jni and I can't seem to find any way to do it, any help would be appriciated

I'll add a branch for 3.2 with #43249 together once this is ready

@m4gr3d

@thebestnom
Copy link
Contributor Author

Anythinhg else on this? @m4gr3d

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.

The PR looks good. I've added a few pieces of feedback.

On the java side, the same logic is repeated both in the GodotGLRenderView and GodotVulkanRenderView. Move that logic into a helper class (e.g: GodotPointerIconHandler), and have both instances of GodotRenderView forward the necessary calls to that class instead.

Let's move support for custom cursor shape into another PR. From the look of the Android api, only passing Bitmap is supported for custom pointer icon. The Godot native api has a matching Bitmap resource class so we may be able to add a conversion utility between the two (and any other matching Godot resource types).

@thebestnom
Copy link
Contributor Author

Let's move support for custom cursor shape into another PR. From the look of the Android api, only passing Bitmap is supported for custom pointer icon. The Godot native api has a matching Bitmap resource class so we may be able to add a conversion utility between the two (and any other matching Godot resource types).

Yeah, that what I was going to do

I already tried a way to convert native bitmap to java bitmap but jni didn't like that, the best way I can think of is sending bitmap as array, but Ill think of a way when well get there

For now Ill try to fix the code this week, thanks for the cr ☺️

@thebestnom thebestnom force-pushed the android-cursor-icons branch 3 times, most recently from ea88d25 to 12ec200 Compare January 9, 2021 22:16
@thebestnom
Copy link
Contributor Author

@m4gr3d tried fixing the comment, while doing this I rebased buy it looks like android build stop working, trying to fix

@thebestnom
Copy link
Contributor Author

thebestnom commented Jan 22, 2021

looks like it's broken on master

@thebestnom thebestnom force-pushed the android-cursor-icons branch 3 times, most recently from 084940a to 802fc71 Compare February 10, 2021 23:55
@thebestnom thebestnom requested a review from a team as a code owner March 7, 2021 20:54
@thebestnom thebestnom force-pushed the android-cursor-icons branch 2 times, most recently from 71e0c60 to ad0bf30 Compare March 21, 2021 22:36
@akien-mga akien-mga requested a review from m4gr3d May 18, 2021 13:13
@akien-mga
Copy link
Member

Code looks good to me, is this ready to merge @thebestnom @m4gr3d?

@thebestnom
Copy link
Contributor Author

Last time I tested that it worked, but I can't run master on phone for about 4 month (can't remember exactly) so I haven't tested that since

@thebestnom
Copy link
Contributor Author

@akien-mga ok, finally could test today and looks like it is still working!

@m4gr3d
Copy link
Contributor

m4gr3d commented May 19, 2021

@akien-mga ok, finally could test today and looks like it is still working!

Great! Are all the requested changes addressed?

@thebestnom
Copy link
Contributor Author

@akien-mga ok, finally could test today and looks like it is still working!

Great! Are all the requested changes addressed?

Looked at the pr comments and the changes to refresh my memories and looks like I already addressed the comments, yep

@thebestnom thebestnom force-pushed the android-cursor-icons branch 2 times, most recently from 8fbec58 to 2631517 Compare May 19, 2021 18:41
@thebestnom
Copy link
Contributor Author

ok, finally fixed static checks... I think this is ready for merge

@akien-mga akien-mga merged commit 1450a72 into godotengine:master May 19, 2021
@akien-mga
Copy link
Member

Thanks!

@thebestnom thebestnom deleted the android-cursor-icons branch May 19, 2021 19:50
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.

4 participants