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

only update Touches resource when needed #12048

Merged

Conversation

JeanMertz
Copy link
Contributor

@JeanMertz JeanMertz commented Feb 22, 2024

Objective

  • The touch_screen_input_system system runs on every tick.
  • It unconditionally calls update(&mut self), on the Touches resource.
  • This blocks the usage of a resource_changed::<Touches> run condition.

Solution

  • Remove update(&mut self) as it's only used in this one system, and in-lining the method implementation removes an indirection to an ambiguously named method.
  • Add conditional checks around the calls to clearing the internal maps.

Signed-off-by: Jean Mertz <git@jeanmertz.com>
@thebluefish
Copy link
Contributor

We don't really need to tie these 3 maps together like this, and I feel that attempting to split this into separate methods to check and clear just muddy it up. I would prefer something more along the lines of

pub fn touch_screen_input_system(
    mut touch_state: ResMut<Touches>,
    mut touch_input_events: EventReader<TouchInput>,
) {
    if !touch_state.just_pressed.is_empty() {
        touch_state.just_pressed.clear();
    }
    if !touch_state.just_released.is_empty() {
        touch_state.just_released.clear();
    }
    if !touch_state.just_canceled.is_empty() {
        touch_state.just_canceled.clear();
    }

    for event in touch_input_events.read() {
        touch_state.process_touch_event(event);
    }
}```

@JeanMertz
Copy link
Contributor Author

We don't really need to tie these 3 maps together like this

Yeah, I was wondering why this was done as well. Happy to make that change 👍

@JeanMertz
Copy link
Contributor Author

@thebluefish made the changes as mentioned, LMKWYT.

Copy link
Contributor

@thebluefish thebluefish left a comment

Choose a reason for hiding this comment

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

Looks good

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-Input Player input via keyboard, mouse, gamepad, and more labels Feb 22, 2024
@@ -793,4 +794,10 @@ mod test {
assert!(!touches.just_canceled(touch_canceled_event.id));
assert!(!touches.just_released(touch_released_event.id));
}

fn update(touch_state: &mut Touches) {
Copy link
Member

@alice-i-cecile alice-i-cecile Feb 22, 2024

Choose a reason for hiding this comment

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

If this is just used in tests, it should be called something like clear_all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed in 1b3d0bd (with the caveat that clear_all is a misnomer, as it only clears three of the four maps in Touches)

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.

Looks good :)

@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 Feb 22, 2024
@BD103
Copy link
Member

BD103 commented Feb 22, 2024

The CI error on Windows does not seem related to the changes in this PR. The Windows crate was updated within the past two hours, so I think it may be the source of the issue.

Edit:

Yup, it was the Windows crate. See microsoft/windows-rs#2870 and microsoft/windows-rs#2869.

@BD103 BD103 added the S-Blocked This cannot move forward until something else changes label Feb 22, 2024
@kennykerr
Copy link

An update is on the way.

@kennykerr
Copy link

The windows-targets crate has now been updated.

@alice-i-cecile alice-i-cecile removed the S-Blocked This cannot move forward until something else changes label Feb 22, 2024
@alice-i-cecile
Copy link
Member

Awesome, thank you @kennykerr. Retrying CI and attempting to merge :)

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Feb 22, 2024
Merged via the queue into bevyengine:main with commit 9dfef45 Feb 22, 2024
25 checks passed
msvbg pushed a commit to msvbg/bevy that referenced this pull request Feb 26, 2024
# Objective

- The `touch_screen_input_system` system runs on every tick.
- It unconditionally calls `update(&mut self)`, on the `Touches`
resource.
- This blocks the usage of a `resource_changed::<Touches>` run
condition.

## Solution

- Remove `update(&mut self)` as it's only used in this one system, and
in-lining the method implementation removes an indirection to an
ambiguously named method.
- Add conditional checks around the calls to clearing the internal maps.

---------

Signed-off-by: Jean Mertz <git@jeanmertz.com>
msvbg pushed a commit to msvbg/bevy that referenced this pull request Feb 26, 2024
# Objective

- The `touch_screen_input_system` system runs on every tick.
- It unconditionally calls `update(&mut self)`, on the `Touches`
resource.
- This blocks the usage of a `resource_changed::<Touches>` run
condition.

## Solution

- Remove `update(&mut self)` as it's only used in this one system, and
in-lining the method implementation removes an indirection to an
ambiguously named method.
- Add conditional checks around the calls to clearing the internal maps.

---------

Signed-off-by: Jean Mertz <git@jeanmertz.com>
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.

None yet

5 participants