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 from GObject to GLib and Python 2 to 3 #14

Merged
merged 15 commits into from
Jul 15, 2020

Conversation

Saumya-Mishra9129
Copy link
Member

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

Fixes #9, Fixes #15, Fixes #10
@chimosky Kindly review changes.

game.py Outdated Show resolved Hide resolved
game.py Outdated Show resolved Hide resolved
game.py Outdated Show resolved Hide resolved
game.py Outdated Show resolved Hide resolved
game.py Outdated Show resolved Hide resolved
game.py Outdated Show resolved Hide resolved
game.py Outdated Show resolved Hide resolved
game.py Outdated Show resolved Hide resolved
game.py Outdated Show resolved Hide resolved
game.py Outdated Show resolved Hide resolved
@chimosky
Copy link
Member

chimosky commented Mar 24, 2020

Nice work, I think you can improve on your commit messages, making commits should help.

Edited your opening comment, adding Fixes #issueno automatically closes the issue once the PR is merged and that makes the job easier.

@quozl
Copy link
Contributor

quozl commented Mar 25, 2020

Thanks. Tested.

  • during press of lowercase keys, two locos occupied the same coordinates, so it was difficult to read the letters,
  • sometimes a letter for a loco may not be visible against the background; especially the fur of the dog,
  • the toolbar was invisible until the first successful set of rounds,
  • in one of the rounds, a loco became a ghost (green and purple) and stayed on screen for every subsequent round,
  • the toolbar shifted the display down enough such that one of the locos with text could not be fully displayed, so I didn't know what key to press to dismiss it,

Having now experienced it, the toolbar adds no value that couldn't be added by a stop or quit button instead.

Activity logs were noisy, frequent mentions of "keypress event" and 'WARNING: erroneous pipeline: syntax error".

@Saumya-Mishra9129
Copy link
Member Author

@quozl "keypress event" log's noise has been fixed. Working on testing .
@chimosky changes requested by you have been fixed.

@quozl
Copy link
Contributor

quozl commented Mar 25, 2020

Thanks.

@Saumya-Mishra9129
Copy link
Member Author

Saumya-Mishra9129 commented Apr 3, 2020

sometimes a letter for a loco may not be visible against the background; especially the fur of the dog,

Can we change the colour of letter for a loco from white to something else in order to avoid this.?

@Saumya-Mishra9129
Copy link
Member Author

Saumya-Mishra9129 commented Apr 3, 2020

@quozl

the toolbar was invisible until the first successful set of rounds,

This is fixed in 3d64076.

in one of the rounds, a loco became a ghost (green and purple) and stayed on screen for every subsequent round,

I am not getting this one on testing.

@quozl
Copy link
Contributor

quozl commented Apr 4, 2020

Thanks for the update. Is 3d64076 correct way to solve this? Is the activity better always fullscreen, and never show the toolbar? We have quite a few activities that do this. My observation in earlier testing is that the toolbar did appear without being asked for, and also that it affected play. Would a better way to solve this be to find out what is causing fullscreen mode to turn off, and stop doing that?

@Saumya-Mishra9129
Copy link
Member Author

Saumya-Mishra9129 commented Apr 4, 2020

Thanks for the update. Is 3d64076 correct way to solve this? Is the activity better always fullscreen, and never show the toolbar? We have quite a few activities that do this. My observation in earlier testing is that the toolbar did appear without being asked for, and also that it affected play. Would a better way to solve this be to find out what is causing fullscreen mode to turn off, and stop doing that?

Thanks @quozl for review. Actually I thought you are saying the reverse of it. I thought we have to display toolbar every time. So I have done that. There was confusion.
According to me , It is better to have full screen always and show the toolbar when we want a toolbar by clicking.

the toolbar shifted the display down enough such that one of the locos with text could not be fully displayed, so I didn't know what key to press to dismiss it,

the toolbar was invisible until the first successful set of rounds,

aa9fd66 should fixed these both.

@Saumya-Mishra9129
Copy link
Member Author

@quozl Also consider this, What we need to do in this case. According to me changing color is best way for solving this.

@chimosky
Copy link
Member

chimosky commented Apr 4, 2020

According to me , It is better to have full screen always and show the toolbar when we want a toolbar by clicking.

How do I test? Clicked and toolbar still isn't visible.

@Saumya-Mishra9129
Copy link
Member Author

Saumya-Mishra9129 commented Apr 5, 2020

According to me , It is better to have full screen always and show the toolbar when we want a toolbar by clicking.

How do I test? Clicked and toolbar still isn't visible.

@chimosky This was the main goal to make toolbar not visible because it was affecting the play.

@quozl
Copy link
Contributor

quozl commented Apr 5, 2020

@quozl Also consider this, What we need to do in this case. According to me changing color is best way for solving this.

I'm not sure changing colour alone is best way.

Number or letter is to be distinguished from the background image. There are several ways to do this. Here are some of them;

  • draw the number or letter using an XOR or inversion rather than in a specific colour,
  • choose a colour for the number or letter based on a calculation of the average brightness in the vicinity of the drawing,
  • reserve one range of brightnesses for background image (e.g. 0% to 80%), and another range of brightnesses for the number or letter (e.g.100% only).

I'm not going to be prescriptive on which way to use. You may think up some other method.

@quozl
Copy link
Contributor

quozl commented Apr 6, 2020

@chimosky This was the main goal to make toolbar not visible because it was affecting the play.

I'm fine with a toolbar, or without a toolbar, as long as these conditions can be satisfied;

  • the toolbar is either present or absent, and doesn't change between states without explicit command from the user,
  • the loco is not positioned in a way that hides the number or letter, (this happened in my earlier test once the toolbar became visible, in turn because the canvas was shifted downward and the positioning did not take that into account),
  • the behaviour of the toolbar is not changed without some reason, otherwise teachers or children who upgrade the activity face a change they did not expect.

@Saumya-Mishra9129
Copy link
Member Author

@quozl

during press of lowercase keys, two locos occupied the same coordinates, so it was difficult to read the letters,

I am not sure how to fix this issue but as I looked at code of game.py, it uses random.uniform for generating location of a loco, as numbers are generated randomly and uniform method having very less range, it can possibly increases chances of collapsing of two locos. and Also I found same issue during press of uppercase keys. It may be possible that increasing the range of uniform can solve this, but I am not sure about it.

@quozl
Copy link
Contributor

quozl commented Apr 14, 2020

I've not examined the code. A collision detection algorithm should be used; so that when a location is randomly generated it is checked to see if it is close to another location already used, and if so the algorithm would try again.

@quozl
Copy link
Contributor

quozl commented Apr 15, 2020

I've examined the code, the history of commits, and made some more tests. The collision is expected and the solution is to drag the Loco. The same for when the letter under the Loco is drawn against a white background. I've marked those test results as resolved.

@quozl
Copy link
Contributor

quozl commented Apr 15, 2020

Tested e013fd5.

  • the "WARNING: erroneous pipeline: syntax error" continues to occur,
  • the toolbar appears at the end of the game and can't be dismissed; if the child continues to play the game there's possibility of Locos positioned such that their letters are invisible, but they can be dragged. Should the toolbar appear?

@Saumya-Mishra9129
Copy link
Member Author

Saumya-Mishra9129 commented Apr 15, 2020

@quozl You were supposed to test the latest commit aa9fd66 .
When I checked with aa9fd66 ,

the toolbar appears at the end of the game and can't be dismissed; if the child continues to play the game there's possibility of Locos positioned such that their letters are invisible, but they can be dragged. Should the toolbar appear?

The toolbar does not appear at the end of the game. I suggest you to check once with aa9fd66 .
Also I am not getting the WARNING: erroneous pipeline: syntax error anywhere. I am not able to understand why it is happening.

@quozl
Copy link
Contributor

quozl commented Apr 16, 2020

Thanks. Tested aa9fd66 on Ubuntu 20.04 on an OLPC NL3 laptop. The toolbar did not appear.

  • the background does not cover the display, which is 1366x768 on this laptop, and the uncovered area is grey. The greeting message window is not fully within the display (because the vertical size of the display is much less than the 900 pixels the program is designed for). The moving Loco in the first level of the game limits position to the area of the background. The drag to the right game used the centre of display rather than the centre of the background image.

Screenshot of LocoSugar Activity

  • "WARNING: erroneous pipeline: syntax error" does occur, and by using strace -ff -o ~/ls -s 64 -i sugar-activity3, followed by a search for the message in the strace logs, followed by analysis of the logs, found that the processes that emit the message were running the program /usr/bin/gst-launch-1.0, which is called by play_audio.py making a subprocess call with an incorrect pipeline; there's also no sound heard. I recommend the player module be replaced with a more modern one that calls the PyGObject Gst module instead; you can find several in maintained activities throughout Sugar Labs.
$ grep WARNING ls*
ls.1794:[00007f4c7e6e9057] write(2, "WARNING: erroneous pipeline: syntax error\n",
42) = 42
$ grep execve ls.1794 | grep -v ENOENT
ls.1794:[00007f10b3df216b] execve("/usr/bin/gst-launch-1.0", ["gst-launch-1.0", "filesrc",
"location=/usr/share/sugar/activities/LocoSugar.activity/sounds/d"..., "! oggdemux",
"! vorbisdec", "! audioconvert", "! alsasink"], 0x11cfb30 /* 64 vars */) = 0
  • the ghost Loco happened again,

@Saumya-Mishra9129
Copy link
Member Author

the background does not cover the display, which is 1366x768 on this laptop, and the uncovered area is grey. The greeting message window is not fully within the display (because the vertical size of the display is much less than the 900 pixels the program is designed for). The moving Loco in the first level of the game limits position to the area of the background. The drag to the right game used the centre of display rather than the centre of the background image.

I am not getting as we need to fix only for 1200x900 resolution or for all other systems as well. the background does not cover the display, which is 1366x768 on this laptop, and the uncovered area is grey. this could happen because the width is greater than 1200 for what it is designed for.

@quozl
Copy link
Contributor

quozl commented Apr 30, 2020

I can see how you could get confused. Many Sugar activities were designed for a fixed resolution of 1200x900 on the OLPC XO laptop, with no intention at the time for the activity to be resolution independent, adaptable, or responsive. This is certainly one of those kinds of activities, as it has bitmap instead of vector graphics.

The OLPC XO laptop is unlikely to run Python 3 activities because of the huge cost of upgrading the rest of the software stack; firmware, kernel, graphics drivers, and so forth. So as soon as an activity tries to transition from Python 2 to Python 3, it brings with it (a) not having to maintain the 1200x900 resolution support, and (b) having to maintain other resolutions and responsiveness.

There are many ways to adapt this activity to other resolutions; best solution is one that works on all expected resolutions, by calculating where to place the pixmaps, and how to either wrap, tile, or place them within letterboxes.

@JuiP
Copy link
Member

JuiP commented Jun 6, 2020

Hi, I'm testing on a 800*600 VM and here is a screenshot of what the display looks like.
Loco1

@Saumya-Mishra9129
Copy link
Member Author

Saumya-Mishra9129 commented Jun 6, 2020 via email

@JuiP
Copy link
Member

JuiP commented Jun 6, 2020

Thanks. I actually edited my comment. Can you tell me your test display size?

@Saumya-Mishra9129
Copy link
Member Author

Thanks. I actually edited my comment. Can you tell me your test display size?

Yeah , Tested on 1920X1080 . The background image is perfect displaying in yours just the problem is with greeting message window size. We need to fix that.

@JuiP
Copy link
Member

JuiP commented Jun 6, 2020

@Saumya-Mishra9129 , I have fixed it in 88fadc8, you can cherry-pick the commit. :)

The ghost loco happened for me too. I tested master, couldnt reproduce on master.

Ghost_loco

@Saumya-Mishra9129
Copy link
Member Author

Saumya-Mishra9129 commented Jun 6, 2020

@Saumya-Mishra9129 , I have fixed it in 88fadc8, you can cherry-pick the commit. :)

The ghost loco happened for me too. I tested master, couldnt reproduce on master.

Ghost_loco

Thanks I need to test it. I still don't know how it is coming. However I tested 88fadc8 . It is not working for me look
Screenshot from 2020-06-06 12-49-59

Update- look into comments of 88fadc8.

@JuiP
Copy link
Member

JuiP commented Jun 6, 2020

Sorry, that was a reckless mistake. Fixed in 0cc356e

@Saumya-Mishra9129
Copy link
Member Author

Saumya-Mishra9129 commented Jun 6, 2020

The ghost loco happened for me too. I tested master, couldnt reproduce on master.

I am worried about it. I did subsequent rounds of testing, It happens sometimes, not every time. What I think is that at the end of a particular level, whenever a loco couldn't delete, It happens as a ghost logo. I don't think it could be because of Python3 changes. It happens in master as well.
I tested master several times, 3rd time I got a loco which couldn't get delete.

@srevinsaju
Copy link
Member

Screenshot_20200606_120859

the ghost loco (I assume its called like that) gets overlapped over each other. Afaik, that ghost loco appears on the third or fourth level of clicking

Tested on 1024 x 768. Testing on other resolutions, will update shortly

@srevinsaju
Copy link
Member

Before the next release, it would be good, if an opaque shape is placed behind the letters to make it readable
image

@srevinsaju
Copy link
Member

srevinsaju commented Jun 6, 2020

I was not able to reproduce the ghost loco getting stuck on screen bug


Tested on 640x480: no bugs
Screenshot_20200606_123919


Tested on 768x1366 (portrait) Looks good
Screenshot_20200606_123642

@Saumya-Mishra9129
Copy link
Member Author

Saumya-Mishra9129 commented Jun 6, 2020

I was not able to reproduce the ghost loco getting stuck on screen bug

Tested on 640x480: no bugs

Tested on 768x1366 (portrait) Looks good

Thanks a lot @srevinsaju You did really a great work while reviewing.

Before the next release, it would be good, if an opaque shape is placed behind the letters to make it readable

Yeah I am thinking it as a good suggestion, we can do it, to increase visual impact on user.

@JuiP
Copy link
Member

JuiP commented Jun 12, 2020

Before the next release, it would be good, if an opaque shape is placed behind the letters to make it readable
Yeah I am thinking it as a good suggestion, we can do it, to increase visual impact on user.

I agree @Saumya-Mishra9129 @srevin. What do you think @chimosky @quozl ?

I am worried about it. I did subsequent rounds of testing, It happens sometimes, not every time. What I think is that at the end of a particular level, whenever a loco couldn't delete, It happens as a ghost logo. I don't think it could be because of Python3 changes. It happens in master as well.
I tested master several times, 3rd time I got a loco which couldn't get delete.

Yes, you are probably right. @Saumya-Mishra9129 Have you found a way to fix it? I don't mind helping if you haven't already found a fix :)

@Saumya-Mishra9129
Copy link
Member Author

Yes, you are probably right. @Saumya-Mishra9129 Have you found a way to fix it? I don't mind helping if you haven't already found a fix :)

Thanks ,@JuiP I haven't fixed it yet. If you could do it would be great.

@srevinsaju
Copy link
Member

@Saumya-Mishra9129 are you getting the ghost loco freqjuently; like more than 5 times while testing 10 times (approx); and is the ghost loco getting stuck at the same position?
I tried looking into it, but was unable to reproduce. I have strong suspicion if its an internal library difference like how it happened in puklanapac and FractionBounce. Are you testing it on Sugar Live Build (debian)?

@JuiP
Copy link
Member

JuiP commented Jun 12, 2020 via email

@Saumya-Mishra9129
Copy link
Member Author

@Saumya-Mishra9129 are you getting the ghost loco freqjuently; like more than 5 times while testing 10 times (approx); and is the ghost loco getting stuck at the same position?
I tried looking into it, but was unable to reproduce. I have strong suspicion if its an internal library difference like how it happened in puklanapac and FractionBounce. Are you testing it on Sugar Live Build (debian)?

I have tested with both packaged and live sugar . I am able to reproduce in both systems. I haven't tested puklanapac and FractionBounce.

@JuiP
Copy link
Member

JuiP commented Jun 19, 2020

@Saumya-Mishra9129 @srevinsaju Update on the ghost loco bug - I tried reproducing it again, but couldn't. I'm not sure what has changed. I also looked at the code and couldn't find a possible reason for the ghost loco.

@Saumya-Mishra9129
Copy link
Member Author

@Saumya-Mishra9129 @srevinsaju Update on the ghost loco bug - I tried reproducing it again, but couldn't. I'm not sure what has changed. I also looked at the code and couldn't find a possible reason for the ghost loco.

Thanks for Update. Actually Why ghost loco happens and even when it happens, it's highly unpredictable here. The problem is still ambiguous for me, I also tried to look at code, I couldn't also find the reason. It also happens in master as well. So I guess better way could be to open an issue here. We can look for merge here then because the work here is completed and you can look for release also then.

@quozl
Copy link
Contributor

quozl commented Jun 20, 2020

My guess is a logic error, or API balance. Sprites are being created, and the list of sprites is lost in some way. It is the kind of problem that should be solved before release. If one method of finding the cause does not work, try another method.

If you've not yet used the Python debugger, now is one of those times it would be of benefit to have the skills.

@Saumya-Mishra9129
Copy link
Member Author

Saumya-Mishra9129 commented Jul 8, 2020

@Saumya-Mishra9129 @srevinsaju Update on the ghost loco bug - I tried reproducing it again, but couldn't. I'm not sure what has changed. I also looked at the code and couldn't find a possible reason for the ghost loco.

Update - @JuiP I have tried 16 times not able to reproduce ghost loco even once. I don't know what has happened now.
Last change we did is 791af63. We have also done some changes in sprites.py, maybe that has solved the problem.

@shaansubbaiah
Copy link
Member

shaansubbaiah commented Jul 8, 2020

I found the original game trailer? at https://www.youtube .com/watch?v=0m_d8_Nfi28, the last line of the description int he site it was published says "Clicking, double-clicking, dragging and typing a few sentences makes cuckoos get better and landscapes more friendly.".

Maybe the by landscapes more friendly they mean, less ghost locos? The links to the original windows executable and XO are dead.

EDIT: Removed the link to the site, the page is likely infected

@quozl
Copy link
Contributor

quozl commented Jul 9, 2020

Thanks. Tested. Approved. No ghost loco in two cycles tested.

@JuiP
Copy link
Member

JuiP commented Jul 9, 2020

Yes I agree, No ghost loco for me too.

@chimosky
Copy link
Member

Reviewed, thanks.

@chimosky chimosky merged commit ab0acd0 into sugarlabs:master Jul 15, 2020
@JuiP JuiP mentioned this pull request Jul 21, 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.

Port to Python 3. PyGI warnings Port from GObject to GLib
6 participants