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

[Merged by Bors] - bevy_input: Fix process touch event #4352

Closed
wants to merge 1 commit into from

Conversation

light4
Copy link
Contributor

@light4 light4 commented Mar 28, 2022

Objective

  • process_touch_event in bevy_input don't update position info. TouchPhase::Ended and TouchPhase::Cancelled should use the position info from pressed. Otherwise, it'll not update. The position info is updated from TouchPhase::Moved.

Solution

  • Use updated touch info.

Changelog

This section is optional. If this was a trivial fix, or has no externally-visible impact, feel free to skip this section.

  • Fixed: bevy_input, fix process touch event, update touch info

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Mar 28, 2022
self.just_released.insert(event.id, event.into());
self.pressed.remove_entry(&event.id);
if let Some((_, v)) = self.pressed.remove_entry(&event.id) {
self.just_released.insert(event.id, v);
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we update the touch phase before inserting? (Same on canceled)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The touch info is from self.pressed or event, so I think this is fine.
I can change this if you prefer to update it first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, the touch info should come from self.pressed.
If not found in self.pressed, there may be some error occurred, set it to default just to get rid of panic.

Copy link
Member

Choose a reason for hiding this comment

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

The touch info is from self.pressed or event, so I think this is fine.

You mean it already reflects the Ended/Canceled phase? I haven't used touches in Bevy before but I assume the event returned from just_released would be one of those phases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The info is updated from phase TouchPhase::Moved

Copy link
Member

Choose a reason for hiding this comment

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

Oh interesting. See I'd think that one should also update the phase to Moved. 🤔

But maybe I'm just mistaken in how the touch system is supposed to work.

@IceSentry IceSentry added A-Input Player input via keyboard, mouse, gamepad, and more and removed S-Needs-Triage This issue needs to be labelled labels Mar 28, 2022
@alice-i-cecile alice-i-cecile added the C-Bug An unexpected or incorrect behavior label Mar 28, 2022
@alice-i-cecile
Copy link
Member

Requesting review from @HackerFoo, who has by far the most practical experience with Bevy's touch APIs.

Copy link
Contributor

@HackerFoo HackerFoo left a comment

Choose a reason for hiding this comment

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

I couldn't understand the PR description. It would be good to update this to make this easier for other reviewers.

From looking at the code and test, it looks like this makes sure .position/.force gets moved to .prev_position/.prev_force for TouchPhase::Ended (the last update in a motion) as it is in other phases. Before this change, the current position would be assigned to both in this phase.

So this change makes sense and is useful to get a correct delta on the last update.

touches.process_touch_event(&end_touch_event);

assert!(touches.just_released.get(&touch_event.id).is_some());
assert!(touches.pressed.get(&touch_event.id).is_none());
let touch = touches.just_released.get(&touch_event.id).unwrap();
assert!(touch.previous_position != touch.position);
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be good to add an explanation for this assertion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

@light4 light4 force-pushed the fix_process_touch_event branch 2 times, most recently from dfeaa8d to 166fd50 Compare March 30, 2022 04:43
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

If you're around, could you try to improve the PR description and add comments to the if let blocks added? This change is correct, and I won't block on it, but it would be nice to clean it up as we go.

I'll merge this once that's done or next Monday.

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Oct 10, 2022
@light4 light4 force-pushed the fix_process_touch_event branch 4 times, most recently from df24196 to 52856a8 Compare October 10, 2022 17:28
`TouchPhase::Ended` and `TouchPhase::Cancelled` should use the position info
from `pressed`. Otherwise, it'll not update. The position info is updated from
`TouchPhase::Moved`.
@light4
Copy link
Contributor Author

light4 commented Oct 10, 2022

Rebased and add comments to if let blocks.
Sorry, I'm not a native English speaker. It'll be great if you can help me to improve the description.

@alice-i-cecile
Copy link
Member

Thanks a ton, and no worries :)

process_touch_event in bevy_input don't update position info. TouchPhase::Ended and TouchPhase::Cancelled should use the position info from pressed. Otherwise, it'll not update. The position info is updated from TouchPhase::Moved.

This is actually quite good now! Thank you very much.

bors r+

bors bot pushed a commit that referenced this pull request Oct 10, 2022
# Objective

- `process_touch_event` in `bevy_input` don't update position info. `TouchPhase::Ended` and `TouchPhase::Cancelled` should use the position info from `pressed`. Otherwise, it'll not update. The position info is updated from `TouchPhase::Moved`.

## Solution

- Use updated touch info. 

---

## Changelog

> This section is optional. If this was a trivial fix, or has no externally-visible impact, feel free to skip this section.

- Fixed: bevy_input, fix process touch event, update touch info
@bors
Copy link
Contributor

bors bot commented Oct 10, 2022

@bors bors bot changed the title bevy_input: Fix process touch event [Merged by Bors] - bevy_input: Fix process touch event Oct 10, 2022
@bors bors bot closed this Oct 10, 2022
@light4 light4 deleted the fix_process_touch_event branch October 10, 2022 17:58
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 19, 2022
# Objective

- `process_touch_event` in `bevy_input` don't update position info. `TouchPhase::Ended` and `TouchPhase::Cancelled` should use the position info from `pressed`. Otherwise, it'll not update. The position info is updated from `TouchPhase::Moved`.

## Solution

- Use updated touch info. 

---

## Changelog

> This section is optional. If this was a trivial fix, or has no externally-visible impact, feel free to skip this section.

- Fixed: bevy_input, fix process touch event, update touch info
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 28, 2022
# Objective

- `process_touch_event` in `bevy_input` don't update position info. `TouchPhase::Ended` and `TouchPhase::Cancelled` should use the position info from `pressed`. Otherwise, it'll not update. The position info is updated from `TouchPhase::Moved`.

## Solution

- Use updated touch info. 

---

## Changelog

> This section is optional. If this was a trivial fix, or has no externally-visible impact, feel free to skip this section.

- Fixed: bevy_input, fix process touch event, update touch info
Pietrek14 pushed a commit to Pietrek14/bevy that referenced this pull request Dec 17, 2022
# Objective

- `process_touch_event` in `bevy_input` don't update position info. `TouchPhase::Ended` and `TouchPhase::Cancelled` should use the position info from `pressed`. Otherwise, it'll not update. The position info is updated from `TouchPhase::Moved`.

## Solution

- Use updated touch info. 

---

## Changelog

> This section is optional. If this was a trivial fix, or has no externally-visible impact, feel free to skip this section.

- Fixed: bevy_input, fix process touch event, update touch info
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective

- `process_touch_event` in `bevy_input` don't update position info. `TouchPhase::Ended` and `TouchPhase::Cancelled` should use the position info from `pressed`. Otherwise, it'll not update. The position info is updated from `TouchPhase::Moved`.

## Solution

- Use updated touch info. 

---

## Changelog

> This section is optional. If this was a trivial fix, or has no externally-visible impact, feel free to skip this section.

- Fixed: bevy_input, fix process touch event, update touch info
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Input Player input via keyboard, mouse, gamepad, and more C-Bug An unexpected or incorrect behavior S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants