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

Bugfix/i578 #634

Merged
merged 3 commits into from
Jul 9, 2024
Merged

Bugfix/i578 #634

merged 3 commits into from
Jul 9, 2024

Conversation

Edearth
Copy link
Contributor

@Edearth Edearth commented Jul 5, 2024

Context

I found out issue #578 while writing a test. Then found out there was a PR for it #587 that's been on hold for +2 months. I hope it's alright taking the liberty to finish this. Credit for the fix still goes to @lxkarp, since I just added a couple test on top of their branch and code.

The previous PR asked for more tests to check the is_action_just_pressed and is_action_just_released methods worked ok.

Goal

Close #578

Add the requested just_pressed and just_released tests.

Note on potential issue

I'm not sure if this is expected behavior, but I think some resources are being leaked from one test to the next. I was getting weird results like 1 key pressed and 2 keys released. I think the InputSender.release_all() and InputSender.clear() weren't being processed before the next test started. So when the second test started, the InputSender.release_all() was processed and it detected the key release from the previous test.

I fixed this by waiting for 1 frame after the InputSender.release_all() method is called, in the after_each() method. If this is expected, maybe it should be documented and added to examples. If it isn't, I can write an issue for it. I'll wait for your feedback before doing anything 🙂

@bitwes
Copy link
Owner

bitwes commented Jul 9, 2024

This looks good. Thanks, I'll make sure you both get credit. Sorry for the delay, been a long week and I then had to try and remember what all this was about.

I fixed this by waiting for 1 frame after the InputSender.release_all() method is called, in the after_each() method. If this is expected, maybe it should be documented and added to examples.

I'm pretty sure this is expected and should be documented.

@bitwes
Copy link
Owner

bitwes commented Jul 9, 2024

@Edearth can you resolve the conflicts and then I'll merge it? I tested merging lxkarp's branch and then this one locally, but I think I broke it when I squashed and merged.

Alix Karpinski and others added 3 commits July 9, 2024 23:02
Also changes the after_each method to wait for the InputSender to
release and clear all actions. There were some presses/releases
being leaked to the next test if we don't wait for an extra frame.
@Edearth
Copy link
Contributor Author

Edearth commented Jul 9, 2024

Don't worry about the delay! I assumed you were on vacation :)

can you resolve the conflicts and then I'll merge it?

I think that should be it ✔️

I'm pretty sure this is expected and should be documented.

Is it ok if I open a second PR with the documentation update in the near future?

@bitwes
Copy link
Owner

bitwes commented Jul 9, 2024

Looks good! Thanks.

Is it ok if I open a second PR with the documentation update in the near future?

Definitely

@bitwes bitwes merged commit baafe2b into bitwes:main Jul 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Input does not seem to receive events from an InputSender
3 participants