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

Fix event poll in pygame and fix UnboundLocalError #26 #27

Closed
wants to merge 1 commit into from

Conversation

srevinsaju
Copy link
Member

@srevinsaju srevinsaju commented Jun 4, 2020

Resolves #26
@JuiP kindly review

Copy link
Member

@JuiP JuiP left a comment

Choose a reason for hiding this comment

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

I tried doing this before, doesn't work. I spent a lot of time finding a fix but nothing I tried worked so raised an issue.
When you test, the stick doesn't grow. Also this may help.

main.py Outdated Show resolved Hide resolved
@srevinsaju
Copy link
Member Author

srevinsaju commented Jun 4, 2020

Tested and fixed the bug: permanantly 😄

@JuiP kindly review 54f5d4e

@Saumya-Mishra9129
Copy link
Member

Just a suggestion -
@JuiP have you tested commits in #24 . That also worked for me with no error in the log.

However @srevinsaju fixed it. I havent tested.

@srevinsaju
Copy link
Member Author

srevinsaju commented Jun 4, 2020

@Saumya-Mishra9129

Happy to explain

I guess this has some advantage because

The last two are one of the reasons, why I chose this method, you can also see the PR title which corresponds to the same idea. pygame.event.poll returns only one event, which is inefficient to handle both pygame.QUIT event and pygame.KEYDOWN events. In case this game is to be added more features, it will force the new contributor to come back to pygame.event.get.

By more features, I mean, MOUSEEVENT, TOUCHEVENT, DISPLAY_SIZE_CB and display_size_cb is one of the features you will be adding during the python3 port.

Either way

But it all depends on how its implemented 😄

@Saumya-Mishra9129
Copy link
Member

Tested 3a3e6b1, works fine for me . 💯

@srevinsaju
Copy link
Member Author

Great! Thanks @Saumya-Mishra9129 😄

@srevinsaju srevinsaju mentioned this pull request Jun 5, 2020
@JuiP
Copy link
Member

JuiP commented Jun 5, 2020

Tested! Worked just as expected! Thanks @srevinsaju 😁

@chimosky
Copy link
Member

I'm not able to get the traceback after playing for some time so I'll leave @quozl to review.

@quozl
Copy link
Contributor

quozl commented Jun 23, 2020

The change is such that an event queue with multiple key 273 down and up events are compressed into a single boolean flag which is then tested after the event queue is flushed. This doesn't seem right to me; it would be more normal to process the key down and up events as they occur rather than lose the extra ones. Can you explain why you want to lose the extra ones?

@srevinsaju
Copy link
Member Author

@quozl have you played the game?

@quozl
Copy link
Contributor

quozl commented Jun 23, 2020

Yes. Most of the time it works properly, but some of the time the stick starts growing without being requested. This is native execution on a laptop with a keyboard. As the many of you seem to prefer VMs, I'm not sure I'm seeing what others are seeing. I don't expect Sugar to be used in a VM by the end user.

@srevinsaju
Copy link
Member Author

ok @quozl, thats something which we didn't notice. I tested on native device (as I prefer native device over VMs), but I have not been able to reproduce the stick growing thing. This implies, this PR might not be yet ready to merge. I will debug this. Thanks for letting me know

@srevinsaju srevinsaju marked this pull request as draft June 23, 2020 08:51
@srevinsaju
Copy link
Member Author

srevinsaju commented Jun 23, 2020

@quozl I am afraid, I cannot reproduce. Played the game for 30 minutes. I can control the growing of the stick whenever I want, except if the stick grows longer than the total screen width, raises a traceback IndexError which is going to be fixed by @JuiP 's #25. Opening this PR, ready for review again 🤔

I am not using an XO laptop to test the activity, I have tested on two systems on native Sugar 0.117 (G31M-ES2C Gigabyte Motherboard, Intel Core Duo 2) with Mechanical Keyboard (Genius); and on a laptop (Intel Core i7, Butterfly keyboard). I am confused now.

@srevinsaju srevinsaju marked this pull request as ready for review June 23, 2020 12:05
@quozl
Copy link
Contributor

quozl commented Jun 24, 2020

@quozl I am afraid, I cannot reproduce. Played the game for 30 minutes.

Thanks for trying.

I can control the growing of the stick whenever I want, except if the stick grows longer than the total screen width, raises a traceback IndexError which is going to be fixed by @JuiP 's #25. Opening this PR, ready for review again 🤔

So if you agree with @JuiP's patches, and they improve your work, please include them in your pull request branch. I don't see why you would not do so.

@srevinsaju
Copy link
Member Author

srevinsaju commented Jul 3, 2020

Merged to #25

@chimosky chimosky closed this Jul 6, 2020
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.

Empty event queue might crash the activity
5 participants