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

Jump to bottom

Add iOS pen pressure #47469

Merged
merged 1 commit into from
Apr 28, 2021
Merged

Add iOS pen pressure #47469

merged 1 commit into from
Apr 28, 2021

Conversation

HEAVYPOLY
Copy link
Contributor

@HEAVYPOLY HEAVYPOLY commented Mar 29, 2021 •
edited
Loading

Adds pen pressure support for iOS Apple Pencil
@bruvzg

@HEAVYPOLY HEAVYPOLY requested review from a team as code owners March 29, 2021 19:47
@Calinou Calinou added this to the 3.4 milestone Mar 29, 2021
@naithar
Copy link
Contributor

naithar commented Mar 29, 2021

@HEAVYPOLY This feature is probably not cherry-pickable to master directly due to input changes, so you might want to make separate PR for master

@naithar
Copy link
Contributor

naithar commented Mar 30, 2021

I think commit i18n: Sync translations with Weblate shouldn't be in this PR. Did you use rebase to update your local branch or just a pull - pulling might be the cause for it. Rebasing is described in Godot's docs: https://docs.godotengine.org/en/stable/community/contributing/pr_workflow.html#updating-your-branch. Also yaml formatting wouldn't be needed after fixing this.
You should probably also squash all your commits into a single one after.

@HEAVYPOLY HEAVYPOLY requested a review from a team as a code owner March 30, 2021 16:06
@HEAVYPOLY HEAVYPOLY force-pushed the ios-pen-pressure branch 2 times, most recently from a0fa3f2 to 9fd95d5 Compare March 30, 2021 16:29
naithar
naithar reviewed Mar 30, 2021
View reviewed changes
editor/translations/af.po Outdated Show resolved Hide resolved
@naithar
Copy link
Contributor

naithar commented Mar 30, 2021

Changes look good to me overall, but I'm still not really sure about usage of InputEventMouseMotion in touch based OS. Maybe extending InputEventScreenDrag and InputEventScreenTouch to support force/tilt would be a better solution or it's fine as it is now @bruvzg?

@HEAVYPOLY
Copy link
Contributor Author

HEAVYPOLY commented Mar 30, 2021 •
edited
Loading

Changes look good to me overall, but I'm still not really sure about usage of InputEventMouseMotion in touch based OS. Maybe extending InputEventScreenDrag and InputEventScreenTouch to support force/tilt would be a better solution or it's fine as it is now @bruvzg?

I think MouseEvents would be better for consistency with Desktop platforms, where pen input is treated as mouse events, even on devices with touch capability. Also on android pen is handled as mouse.

@bruvzg
Copy link
Member

bruvzg commented Mar 30, 2021

I think MouseEvents would be better for consistency with Desktop platforms, where pen input is treated as mouse events, even on devices with touch capability. Also on android pen is handled as mouse.

It's handled like a mouse on other platforms. Would be nice to have separate pen events, but on desktops, it's not always possible to distinguish between mouse and pen/touch events. I agree it should use mouse events for compatibility.

bruvzg
bruvzg approved these changes Mar 30, 2021
View reviewed changes
Copy link
Member

@bruvzg bruvzg left a comment

Choose a reason for hiding this comment

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

I do not have Apple Pencil to test, but code seems fine.

akien-mga
akien-mga reviewed Mar 31, 2021
View reviewed changes
platform/iphone/os_iphone.mm Outdated
Comment on lines 231 to 232
//ev->set_pressure(p_force);
//ev->set_tilt(p_tilt);
Copy link
Member

Choose a reason for hiding this comment

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

I'd remove the commented out code. If it's meant as a reminder to implement pressure and tilt support, it should be a // TODO or // FIXME comment explaining what needs to be done.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, done!

@HEAVYPOLY HEAVYPOLY force-pushed the ios-pen-pressure branch 2 times, most recently from 0a62ccb to af44f74 Compare April 1, 2021 00:21
@akien-mga akien-mga merged commit 8ec14c9 into godotengine:3.x Apr 28, 2021
@akien-mga
Copy link
Member

Thanks!

@akien-mga
Copy link
Member

This should also be forward ported to master (even if it might be trickier to test there as things are a lot more unstable), so that the feature is available in 4.0+ too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akien-mga akien-mga akien-mga left review comments

@naithar naithar naithar approved these changes

@bruvzg bruvzg bruvzg approved these changes

Assignees
No one assigned
Labels
enhancement platform:ios topic:input topic:porting
Projects
None yet
Milestone
3.4
Development

Successfully merging this pull request may close these issues.
5 participants
@HEAVYPOLY @naithar @bruvzg @akien-mga @Calinou