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

Fix Android logic for deferred window input events being inverted #83301

Merged
merged 1 commit into from
Oct 24, 2023
Merged

Fix Android logic for deferred window input events being inverted #83301

merged 1 commit into from
Oct 24, 2023

Conversation

Alex2782
Copy link
Contributor

@Alex2782 Alex2782 commented Oct 14, 2023 •
edited by akien-mga
Loading

Workaround / Fixes #82396 (comment) #66318
maybe more: Github search

Bugsquad edit:

physics_frames comparison at TouchScreenButton is not always possible. input.cpp#L306
TouchScreenButton::_press is mostly triggered after _physics_process.

The change should have no effect on Input Map actions: input.cpp#L716

Minimal test project: test.zip

Screen_recording_20231022_030544.mp4

@Alex2782 Alex2782 requested a review from a team as a code owner October 14, 2023 01:11
@AThousandShips AThousandShips added this to the 4.2 milestone Oct 14, 2023
@Alex2782 Alex2782 force-pushed the touch_screen_button_physics_process branch from 672b9ce to e757fa2 Compare October 15, 2023 01:14
@Alex2782 Alex2782 changed the title Fixes the 'is_action_just_pressed' query in '_physics_process' for 'TouchScreenButton'. Fixes the 'is_action_just' queries in '_physics_process' for 'TouchScreenButton'. Oct 15, 2023
@Alex2782

This comment was marked as outdated.

Sign in to view
@Alex2782 Alex2782 mentioned this pull request Oct 15, 2023
Closed
@lawnjelly
Copy link
Member

lawnjelly commented Oct 16, 2023 •
edited
Loading

Great work on this PR (helped me work out what was likely going wrong) - I think there is a better way to fix, as this PR will introduce a tick delay in input.

See #66318 (comment)

EDIT: Just clear any confusion, this comment refers to previous version of the PR, author has since changed the commit significantly.

@lawnjelly lawnjelly mentioned this pull request Oct 16, 2023
Closed
@Alex2782

This comment was marked as outdated.

Sign in to view
@Alex2782

This comment was marked as outdated.

Sign in to view
@Alex2782 Alex2782 mentioned this pull request Oct 22, 2023
Closed
@Alex2782

This comment was marked as outdated.

Sign in to view
@Alex2782 Alex2782 force-pushed the touch_screen_button_physics_process branch from 660efb8 to 458b32e Compare October 23, 2023 22:15
@Alex2782 Alex2782 requested a review from a team as a code owner October 23, 2023 22:15
@Alex2782
Copy link
Contributor Author

Looks to me like a "deferred" error, PR was 2 years ago.
I'll ask in the Android chat what effects it has and if other errors are known.

image

@lawnjelly
Copy link
Member

PR concerned is #51597 . Pinging @RandomShaper .

RandomShaper
RandomShaper approved these changes Oct 24, 2023
View reviewed changes
Copy link
Member

@RandomShaper RandomShaper left a comment

Choose a reason for hiding this comment

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

🤦‍♂️

@lawnjelly
Copy link
Member

lawnjelly commented Oct 24, 2023 •
edited
Loading

Great spot @Alex2782 ! 👍

EDIT: Actually the commit message is not that bad, I'm sure production team will say if they prefer different wording. I try to include in the commit message what the PR actually does (prevents deferred call bug).

For future reference it's fine to open multiple PRs using different approaches, and close previous ones as you find a better approach.

@akien-mga akien-mga changed the title Fixes the 'is_action_just' queries in '_physics_process' for 'TouchScreenButton'. Fix Android logic for deferred window input events being inverted Oct 24, 2023
@akien-mga
Copy link
Member

akien-mga commented Oct 24, 2023 •
edited
Loading

I amended the PR title to match what the actual bug was, and not one of its prominent symptoms.

The commit message could be amended similarly, but it's not critical.

Edit: I amended the commit message myself as I want to merge this now for 4.2 beta 3.
I kept the context about TouchScreenButton in the commit's description.

@Alex2782 @akien-mga
Fix Android logic for deferred window input events being inverted
5137497 
Notably fixes issues with `is_action_just_*` queries in `_physics_process`
for TouchScreenButton.

Fixes godotengine#66318.
Fixes godotengine#82396.
@akien-mga akien-mga force-pushed the touch_screen_button_physics_process branch from 458b32e to 5137497 Compare October 24, 2023 08:43
@akien-mga akien-mga merged commit 4ec07ff into godotengine:master Oct 24, 2023
14 checks passed
@akien-mga
Copy link
Member

Thanks!

@akien-mga akien-mga added cherrypick:4.0 cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release labels Oct 24, 2023
@Alex2782 Alex2782 deleted the touch_screen_button_physics_process branch October 24, 2023 12:27
@YuriSizov YuriSizov removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Oct 24, 2023
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.1.3. Note that without #79341 the code was different, so I had to remake the changes. It's unlikely that we would make another 4.0.x release, but if we do, the same changes need to be remade there.

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

@RandomShaper RandomShaper RandomShaper approved these changes

@lawnjelly lawnjelly Awaiting requested review from lawnjelly

Assignees
No one assigned
Labels
bug platform:android topic:input topic:porting
Projects
None yet
Milestone
4.2
6 participants
@Alex2782 @lawnjelly @akien-mga @YuriSizov @RandomShaper @AThousandShips