-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
only update Touches
resource when needed
#12048
Conversation
Signed-off-by: Jean Mertz <[email protected]>
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);
}
}``` |
Yeah, I was wondering why this was done as well. Happy to make that change 👍 |
@thebluefish made the changes as mentioned, LMKWYT. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good
crates/bevy_input/src/touch.rs
Outdated
@@ -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) { |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good :)
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. |
An update is on the way. |
The |
Awesome, thank you @kennykerr. Retrying CI and attempting to merge :) |
# 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 <[email protected]>
# 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 <[email protected]>
Objective
touch_screen_input_system
system runs on every tick.update(&mut self)
, on theTouches
resource.resource_changed::<Touches>
run condition.Solution
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.