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 #4

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Saumya-Mishra9129
Copy link
Member

@Saumya-Mishra9129 Saumya-Mishra9129 commented Mar 28, 2020

PR include -

  1. port to python3
    2.GObject to GLib
    @quozl please make a 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.

  • sugargame/init.py : change version to '1.3'
  • f = open(_file_path, 'r') needs to be changed to f = open(_file_path, 'rb') open() under Python 2 return byte strings, while under Python 3 they return unicode strings
  • Add K_KP3 and K_KP5 to event translation table

Checkout the changes made in sugargame activity during port to python3

sugargame/canvas.py Outdated Show resolved Hide resolved
@Saumya-Mishra9129
Copy link
Member Author

Thanks @JuiP ..Changes have made as suggested by you.

@ayushnawal
Copy link

@Saumya-Mishra9129 Thanks for the pull request

Please update sugargame

@quozl
Copy link

quozl commented Mar 29, 2020

Thanks. Reviewed. I also fetched the latest commit and compared to sugargame master, and there are some differences. Please resolve these.

I've also cherry-picked 7b95d4b ("GObject to GLib") so please rebase.

Signed-off-by: James Cameron <quozl@laptop.org>
@quozl
Copy link

quozl commented Mar 30, 2020

Based on the git history, you seem to have just done a git pull from master, whereas it would have been better to git rebase.

So I've done a rebase for you.

Here's how the branches are rendered with gitk just before I push (sm is your repository);

tmp

This shows a few things;

  • your master branch has fallen behind our master branch,
  • you appear to have merged your local repository against your remote repository over https.

Check that you have done a "git remote add" for the sugarlabs repository?

@quozl
Copy link

quozl commented Mar 30, 2020

Reviewed.

Tested in Terminal using sugar-activity3; pygame.error: mixer not initialized.

Fixed and pushed, please test? I've run out of time for today.

@chimosky
Copy link
Member

chimosky commented Mar 30, 2020

5946aff seems to be an update to sugargame for the most part than a port to python 3.

7adaba9 and f4da408 are the same, why both?
And @quozl fixed the pygame.mixer initialisation in f080d3e, seems you didn't test.

c961223 is a merge commit and makes viewing history a bit difficult.

Proyecto.py Outdated Show resolved Hide resolved
Proyecto.py Outdated Show resolved Hide resolved
Proyecto.py Outdated Show resolved Hide resolved
sugargame/event.py Outdated Show resolved Hide resolved
@ayushnawal
Copy link

A completed change must be as close as possible to sugargame master.

Also, there are calls to sys.exit().

Although this could have been taken care of when porting to sugargame v1.2, for now, we should match the semantics used by sugargamev1.3.

@Saumya-Mishra9129
Copy link
Member Author

@chimosky The above commits were necessary. I have removed them.

@Saumya-Mishra9129
Copy link
Member Author

@quozl I have tested, the activity is working without error but The license is not specified for this activity.

@ayushnawal
Copy link

@Saumya-Mishra9129 Thanks for the changes but the port is incomplete.

  • Does not return on quit
  • Avoid sys.exit calls
  • Match semantics used by sugargamev1.3, see test for example. A completed change must be as close as possible to sugargame master.
  • There are still end of line marks, i am not sure why you marked them as resolved but thereare not resolved yet
  • port setup.py (shebang)

@JuiP
Copy link
Member

JuiP commented Mar 30, 2020

Hey, @ayushnawal I have seen your previous ports to sugargame v1.3, I have a silly doubt; What are the impacts of including sugargame as a git submodule? Adding it as submodules might help in updating to the latest sugargame version by just git submodule update Won't this save time or are there any specific issues which might arise due to this, which are being solved by making line-wise changes to sugargame? @chimosky

@ayushnawal
Copy link

ayushnawal commented Mar 30, 2020

@JuiP I gave it a thought once. But many times I have hit activities that were unfinished, or unreleased, or don't work because they have not been maintained against changing library environment. So many times I got my PR stall and then I had to take help from james and srevin. Some of the issues which should be taken care of are: return on quit, pygame module initialization in PygameCanvas, detect stop button, etc. Ultimately what I think is when updating sugargame, we should try to match the semantics of activity with the test activity as close as possible.

In many activities port to sugargame v1.2 was not done properly which needed to be fixed. That's why these days I am reviewing changes made in the codebase of sugargame, toolkit, etc. so that I can complete my remaining PR's too. So in my opinion No, just updating sugargame source files isn't sufficient for activities that are not maintained regularly.

I guess I cleared your doubt because I too faced it many times 😓

@Saumya-Mishra9129
Copy link
Member Author

@ayushnawal I have removed sys.exit calls from Proyecto.py. End of Line and shebang as well. I have tried to match the semantics used by sugargamev1.3 and used stick-hero-activity commit of you so that event queue will not become full. Review once.

@quozl
Copy link

quozl commented Mar 30, 2020

Thanks for the question about using submodules. Here's our history and current situation;

  • we tried using submodules for CollabWrapper on Browse and some other activities, because it was a cool new feature,
  • some new developers failed to do recursive clones, and couldn't run activities until they got their panic questions answered,
  • some downstreams chose that moment to try using the GitHub .tar.gz files from git release tags, which do not contain the submodule files, didn't test what they had packaged, and it took whole six month release cycles to recover from it,
  • we would still need to update the submodule, which becomes an unusual operation as opposed to a well-understood operation with a small number of files.

Submodules can be fantastic, but they aren't a good solution to the type of problems we have. Most of our problems are inattention, and requiring more attention to complicated tools works against that.

@quozl
Copy link

quozl commented Mar 30, 2020

Tested 209a09b.

  • it takes extra time for the activity to render the menu image,
  • with no framerate limitation, the CPU goes to maximum and fans start up,
  • the activity does not fit on the 1024x768 display I'm using for this test; so after pressing the first menu option there is nothing to press in the next screen,
  • the text is in Spanish; it needs an English translation,
  • cannot exit the activity any more; pressing esc just restarts the music, as does the third menu option; in replacing sys.exit you must cause the activity to close.

@JuiP
Copy link
Member

JuiP commented Mar 31, 2020

Thanks! :) @ayushnawal and @quozl

@ayushnawal
Copy link

@Saumya-Mishra9129 sys.exit was used to close the activity, you must exit the activity after replacing it. We should allow sugar toolkit to cleanup.

See the Sugargame test activity for how to exit properly. The event loop must be fixed. Calling sys.exit bypasses any of Sugar Toolkit activity closure actions.

@Saumya-Mishra9129
Copy link
Member Author

fixed sys calls in 5f4e8af .
@quozl I am working on translations. I have to complete proposal so I did not do yet.

@ayushnawal
Copy link

@Saumya-Mishra9129 you haven't tested the changes you made.

  • It is taking much time for menu screen to appear along with very heavy cpu usage.
  • I am not able to quit the activity using escape key or ctrl+q, again as I asked above how are you causing the activity to close?

I would suggest you to please test the changes you made once

@quozl
Copy link

quozl commented Apr 1, 2020

@Saumya-Mishra9129, have a look at the control flow in the Sugargame test activity; both files.

@Saumya-Mishra9129
Copy link
Member Author

Saumya-Mishra9129 commented Apr 11, 2020

Tested 209a09b.

  • it takes extra time for the activity to render the menu image,
  • with no framerate limitation, the CPU goes to maximum and fans start up,
  • the activity does not fit on the 1024x768 display I'm using for this test; so after pressing the first menu option there is nothing to press in the next screen,
  • the text is in Spanish; it needs an English translation,
  • cannot exit the activity any more; pressing esc just restarts the music, as does the third menu option; in replacing sys.exit you must cause the activity to close.

@quozl I have tried to fix CPU framerate and escape exit call in 981eea8.

the activity does not fit on the 1024x768 display I'm using for this test; so after pressing the first menu option there is nothing to press in the next screen

The pygame here uses hardcoded screen sizes. It is designed for 1200X900 display . However, it should be working on all screens . This issue needs to be fixed first in order to ensure smooth functioning of activity.

@quozl
Copy link

quozl commented Apr 15, 2020

Thanks.

The framerate limitation is effective, as is exit on escape key.

Reviewed as at 981eea8 briefly, and observed some surprises;

  • the modules argument to PygameCanvas includes pygame.font but there is no use of that module elsewhere; why initialise it?
  • the main argument to PygameCanvas is a function that calls pygame.init, but that's not the execution pattern that is used in Sugargame; it is related to the extra time for the activity to render the menu image (because of all the images that are loaded after the music starts), please refer to the execution pattern of the Sugargame test activity for how this should be done.

On the other hand, the music happening at the same time as image loading delay could be a feature. An alternate way to solve this is to have two phases of loading; first phase loads the main menu image, and second phase loads all the other images once the main menu is flipped to the display.

During the delay loading, a transparent window has covered the screen and isn't rendered until loading finishes. On particularly slow systems, or systems with disk I/O contention, a child may switch to another app because of the delay.

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.

5 participants