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

Port to python3 #1

Merged
merged 11 commits into from
Jul 27, 2022
Merged

Port to python3 #1

merged 11 commits into from
Jul 27, 2022

Conversation

ayushnawal
Copy link

@ayushnawal ayushnawal commented Feb 5, 2020

Tested.
All set.

@Aniket21mathur
Copy link

Thanks. Reviewed. No comments.

@ayushnawal
Copy link
Author

@quozl please review!

@quozl
Copy link

quozl commented Feb 7, 2020

Thanks, but given your other pull requests involving the Sugargame API, I'd prefer to see you fix them before I review any further changes.

@ayushnawal
Copy link
Author

I reviewed and fixed them as you suggested, you can review these changes now

NumRush.py Show resolved Hide resolved
Copy link
Member

@srevinsaju srevinsaju left a comment

Choose a reason for hiding this comment

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

Thanks @ayushnawal
Tested,

  • Activity does not terminate properly. This creates an everrunning background process similar to Conzoco activities. On trying to force kill the activiity (ctrl+c) , it creates a LookupError regarding dbus.

found some general bugs that might not have arisen due to porting but, yeah:

  • Unable to press / interact with stop button when the countdown goes on. (this might not be an important bug, but of general notice)

@quozl
Copy link

quozl commented Feb 27, 2020

* Activity does not terminate properly.

Possibly because the stop button does not have a callback that signals the event loop to terminate.

See the Sugargame test activity for how to do this.

@ayushnawal
Copy link
Author

Terminating for me properly now.

@srevinsaju @quozl please review

@srevinsaju
Copy link
Member

Thanks @ayushnawal, the error's fixed.

@quozl
Copy link

quozl commented Feb 27, 2020

Thanks Reviewed as at 240e47c.

  • variable naming; crashed usually means failure, and a request to stop is not a failure; consider the name as used by the Sugargame test activity,
  • although the stop button or accelerator may be used, the three event loops complete a display update and clock delay before returning; it is better to return as soon as the activity is asked to stop,
  • setup.py is still Python 2,
  • NumRush.py still begins with a Python version independent hash bang,
  • README.md refers to Numberrush, but the name of the activity in activity.info and elsewhere is different.

Hope that helps. I didn't have time for a full review.

@ayushnawal
Copy link
Author

Fixes done as suggested. Please review @quozl

README.md refers to Numberrush, but the name of the activity in activity.info and elsewhere is different.

I looked at activities.sugarlabs.org , named it as it was there.

@quozl
Copy link

quozl commented Mar 2, 2020

Thanks. Reviewed again.

  • the ctrl+q accelerator or stop button does not work during the newGameAnimation, because the GTK event source is not being pumped; Sugar pygame activities can't use pygame.time.wait for this reason; instead delays have to be implemented by running the event loop until a time has expired, as I mentioned in another pull request.

@quozl
Copy link

quozl commented Mar 2, 2020

Ah, there it is. My comment there shows how to solve the above problem and keep the game user experience the same.

@ayushnawal
Copy link
Author

Fixed it also 😁

Thanks for the help

Tried to improve code quality, removed repetitive code using a function

Tested. all changes are done.

Please review

@quozl
Copy link

quozl commented Mar 12, 2020

You are yet to

As the code stands now, ctrl+q is ignored during each call to pygame.time.wait.

@ayushnawal
Copy link
Author

Fixed. optimized.

@quozl, please review

@quozl
Copy link

quozl commented Mar 16, 2020

Thanks. Heavy CPU usage during numGameAnimation, please fix.

CPU was heavily used during newGameAnimation, fixed by setting fps=30
@ayushnawal
Copy link
Author

ayushnawal commented Mar 17, 2020

Thanks, Fixed it 😁

verified using htop

@quozl

@quozl
Copy link

quozl commented Mar 18, 2020

Thanks. Tested.

  • the high score is not kept from one instance to the next; in reviewing this activity on 2018-01-29 it was said "make sure that stopping and resuming the activity returns to the same game state; this is what read_file and write_file are for", but other activities use a file in the instance directory for keeping a high score,
  • the box can be moved off the right edge by pressing the right arrow key repeatedly,
  • the falling speed of the numbers increases in a step-wise fashion as the game progresses, and the steps seem large,
  • when using the non-activity mode (python3 NumRush.py) the window has a window manager border which causes the box to be invisible.

https://github.com/Gr33nMayhem/NumberRush/issues has some other issues yet to be worked on.

@JuiP
Copy link
Member

JuiP commented May 8, 2020

@ayushnawal ping

@srevinsaju
Copy link
Member

@JuiP , seems like we lost the push on this PR. If you would like to continue this PR, then you may do it.

@sourabhaa
Copy link

@srevinsaju @quozl @chimosky I'm interested to complete this PR. I cloned it in Activities directory, but I'm not able to run the activity from the homepage. The activity icon doesn't show up on home page. Am I missing anything?

@quozl
Copy link

quozl commented Aug 17, 2021 via email

@chimosky
Copy link
Member

@srevinsaju @quozl @chimosky I'm interested to complete this PR. I cloned it in Activities directory, but I'm not able to run the activity from the homepage. The activity icon doesn't show up on home page. Am I missing anything?

Thanks for picking this up, what's left is mentioned in #1 (comment) and while looking at the other repo, you can keep the flake8 fixes to a different PR.

@sourabhaa sourabhaa mentioned this pull request Apr 7, 2022
3 tasks
@chimosky chimosky merged commit 570f517 into sugarlabs:master Jul 27, 2022
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.

7 participants