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

[Merged by Bors] - Avoid triggering change detection for inputs #6847

Closed
wants to merge 5 commits into from
Closed

[Merged by Bors] - Avoid triggering change detection for inputs #6847

wants to merge 5 commits into from

Conversation

james7132
Copy link
Member

Objective

Fix #5292.

Solution

Avoid derefencing when clearing to ensure that change detection is not triggered when there is nothing to clear.

@james7132 james7132 added C-Bug An unexpected or incorrect behavior A-Input Player input via keyboard, mouse, gamepad, and more labels Dec 4, 2022
@alice-i-cecile
Copy link
Member

Do we need to do similar things for our other Input types? If so, I'd like to hit those all in one PR.

@mockersf
Copy link
Member

mockersf commented Dec 4, 2022 •
edited
Loading

It seems this pattern should be generalised and public for other type of input.

Would there be a way to have a clear_if_needed that would not trigger change detection on the Input if not needed? I don't think it's possible, but would be nice...

@james7132
Copy link
Member Author

It seems this pattern should be generalised and public for other type of input.

Would there be a way to have a clear_if_needed that would not trigger change detection on the Input if not needed? I don't think it's possible, but would be nice...

Only way I can think of is just using input.bypass_change_detection().clear(). Seems like the most non-intrusive solution.

@alice-i-cecile
Copy link
Member

Yeah. The other option would be to add a serious trait bound for T, with an associated type to describe the corresponding event type.

I've wanted this before, but it's much more invasive and I'm not 100% sure that it's the right abstraction.

@mockersf
Copy link
Member

mockersf commented Dec 5, 2022

Only way I can think of is just using input.bypass_change_detection().clear(). Seems like the most non-intrusive solution.

I wouldn't be very against using that...

This pattern to avoid triggering change detection (either with check or with bypass) should also be mentioned in

bevy/crates/bevy_input/src/input.rs

Line 36 in c55d553

/// * Call the [`Input::clear`] method at each frame start, before processing events.

@james7132 @mockersf
Fix typo
cb086c8 
Co-authored-by: François <mockersf@gmail.com>
@james7132 james7132 requested a review from mockersf December 7, 2022 23:09
@james7132 james7132 changed the title Avoid trigger change detection for keyboard inputs Avoid triggering change detection for keyboard inputs Dec 7, 2022
@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 Dec 10, 2022
@mockersf
Copy link
Member

Could you update the title to mention it's not just for keyboard?

@james7132 james7132 changed the title Avoid triggering change detection for keyboard inputs Avoid triggering change detection for inputs Dec 10, 2022
@alice-i-cecile
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Dec 11, 2022
@james7132
Avoid triggering change detection for inputs (#6847)
0d67c32 
# Objective
Fix #5292.

## Solution
Avoid derefencing when clearing to ensure that change detection is not triggered when there is nothing to clear.
@bors
Copy link
Contributor

bors bot commented Dec 11, 2022

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Avoid triggering change detection for inputs [Merged by Bors] - Avoid triggering change detection for inputs Dec 11, 2022
@bors bors bot closed this Dec 11, 2022
@james7132 james7132 deleted the input-change-detection branch December 12, 2022 04:01
alradish pushed a commit to alradish/bevy that referenced this pull request Jan 22, 2023
@james7132 @alradish
Avoid triggering change detection for inputs (bevyengine#6847)
854a92a 
# Objective
Fix bevyengine#5292.

## Solution
Avoid derefencing when clearing to ensure that change detection is not triggered when there is nothing to clear.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
@james7132 @ItsDoot
Avoid triggering change detection for inputs (bevyengine#6847)
5e8cdb7 
# Objective
Fix bevyengine#5292.

## Solution
Avoid derefencing when clearing to ensure that change detection is not triggered when there is nothing to clear.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mockersf mockersf mockersf approved these changes

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Change detection for keyboard input is not working correctly.
3 participants
@james7132 @alice-i-cecile @mockersf