Port to Python3 and Update sugargame

[Saumya-Mishra9129]

Tested, Worked as expected. @quozl @chimosky please review.

[quozl]

Thanks. Reviewed.

• 4d0290f has extra parentheses in the change to welcomescreen.py,
• 832bb7c too many of the files have a shebang,
• dac603a is unusual in that it causes a call to sugar3.activity.Activity to get the activity root before the activity __init__ is called; how is this verified to be safe use of the API?

[Saumya-Mishra9129]

Thanks. Reviewed.

4d0290f has extra parentheses in the change to welcomescreen.py,
832bb7c too many of the files have a shebang,
dac603a is unusual in that it causes a call to sugar3.activity.Activity to get the activity root before the activity init is called; how is this verified to be safe use of the API?

Thanks @quozl Please review till 8a322bc.

dac603a is unusual in that it causes a call to sugar3.activity.Activity to get the activity root before the activity init is called; how is this verified to be safe use of the API?

Yeah It wasn't correct way to use API . Tried it another way in 8a322bc.

[quozl]

Thanks. I'm in the middle of reviewing this after rebasing on the new master; progress in my branch quozl@382201c.

Please confirm you tested this in Python 2 before porting? I'm seeing some odd behaviour that I've not found cause of yet.

Back in a few hours.

[quozl]

I've tested 8a322bc, and the game is not playable. There's also a division by zero;

Traceback (most recent call last):
  File "/usr/share/sugar/activities/StickHero.activity/main.py", line 831, in make
    acc = abs(((pillarfast) - (sx(429, 1) + pillardist)) / time)
ZeroDivisionError: float division by zero

I've not found the cause of either problem. Porting to Sugargame v1.3 usually requires some changes to the activity, but I can't see any.

My review comments are illustrated by my branch;

• your four Sugargame commits (86b6538, 8328be7, d75599b and 4897e03) should be three commits, and could be more clearly identified as taken from Sugargame itself,
• your multiple Port to Python 3 commits (37bfa60, 7989279, 07d350b) can be one commit (40159dd),
• your score path fix (8a322bc) can be slightly different (382201c).

[quozl]

I've tested master, it is not playable in the same way as 8a322bc.

[Saumya-Mishra9129]

I've tested 8a322bc, and the game is not playable.

I was able to play game after this commit as well. I don't know why it didnt work for you.

I've tested master, it is not playable in the same way as 8a322bc.

Do you mean 382201c didn't work also. What's the error, is it same as division by zero error.?

Please confirm you tested this in Python 2 before porting? I'm seeing some odd behaviour that I've not found cause of yet.

I have tested with master before Port to Python3. and it was working fine for me.

[Saumya-Mishra9129]

I have tested 382201c , worked fine for me. No error in the log , even not one zero division error. I don't know why It didnt work for you. I have tested 3-4 times.

[Saumya-Mishra9129]

@quozl Tested your master as well, Working fine for me as expected.

[quozl]

Interesting, thanks. The incorrect behaviour is spontaneous stopping of the plank extension, without my releasing a key. It might have something to do with keyboard repeat.

[Saumya-Mishra9129]

Interesting, thanks. The incorrect behaviour is spontaneous stopping of the plank extension, without my releasing a key. It might have something to do with keyboard repeat.

Okay Thanks I couldn't get such behaviour , I don't know what the problem is. I will try out with gdb , maybe it can catch error.

Update - gdb also couldn't catch error unfortunately.

[quozl]

While looking at the code for the cause of the spontaneous stopping, detected unrelated review comments;

• pygame.mixer.init is called in the wrong place, it should be called by adding to the modules argument of PygameCanvas, otherwise initialisation happens at a different time,
• main.py reads mouse position (pygame.mouse.get_pos) without reason; the variables are not referenced,
• the program often ignores mouse position or state in events, instead using get_pos or get_pressed to obtain them after an event,
• there is use of pygame.quit and sys.exit, in contravention of Sugargame design,

Also in testing 8a322bc more extensively;

• when played on my display, the PLAY button is not a circle,
• ZeroDivisionError continues to occur, which causes main.py:run to abort, so a subsequent call to run will be made; unless you see this error you will not see the spontaneous stopping of the plank extension, so it needs to be fixed, ... it was the first problem I mentioned above,
• pygame.error: No free channels available occurs during _mousemove_cb,

Hope that helps.

Mentoring; this isn't an activity I would have recommended for GSoC 2020, but I wasn't involved in selecting activities. Bit of history; the activity was written in 2015, wrapped in Sugargame and contributed in 2016, but did not receive a comprehensive code review. Same author contributed flappy birds and there was an argument on sugar-devel@ about him not responding to code review, and he didn't reply to other threads on sugar-devel@ later in the year. Author went on to make significant contributions such as download progress icon in Browse. I took this (stick hero) to be a drop and forget development. It's up to us to make what we can of what we have been left with. It is code written by someone who would probably now write it very differently, and it shows, badly.

[Saumya-Mishra9129]

Mentoring; this isn't an activity I would have recommended for GSoC 2020, but I wasn't involved in selecting activities. Bit of history; the activity was written in 2015, wrapped in Sugargame and contributed in 2016, but did not receive a comprehensive code review. Same author contributed flappy birds and there was an argument on sugar-devel@ about him not responding to code review, and he didn't reply to other threads on sugar-devel@ later in the year. Author went on to make significant contributions such as download progress icon in Browse. I took this (stick hero) to be a drop and forget development. It's up to us to make what we can of what we have been left with. It is code written by someone who would probably now write it very differently, and it shows, badly.

Thanks but leaving this in middle is not good, I will try to fix all the errors you mentioned above.

[Saumya-Mishra9129]

@quozl I have tried to fix some of the things you mentioned above. However the source is too much different from what we write now a days in other activities. The errors -

ZeroDivisionError continues to occur, which causes main.py:run to abort, so a subsequent call to run will be made; unless you see this error you will not see the spontaneous stopping of the plank extension, so it needs to be fixed, ... it was the first problem I mentioned above,
pygame.error: No free channels available occurs during _mousemove_cb,

I am not able to reproduce them. I don't know how to solve them. The source code contains lots of global and local variables along with lot of if-else statements, which is not a good practice to write a code, and causing ambiguous errors.

[quozl]

@quozl I have tried to fix some of the things you mentioned above. However the source is too much different from what we write now a days in other activities. The errors -

ZeroDivisionError continues to occur, which causes main.py:run to abort, so a subsequent call to run will be made; unless you see this error you will not see the spontaneous stopping of the plank extension, so it needs to be fixed, ... it was the first problem I mentioned above,
pygame.error: No free channels available occurs during _mousemove_cb,

I am not able to reproduce them. I don't know how to solve them.

You don't need to reproduce problems in order to solve them.

For the division by zero, can you explain under what conditions the number after the division will be zero?

For the no free channels during mousemove_cb, it may be consequence of the division by zero. If not, can you explain what the callback is needed for? From the source of Pygame can you explain why creating an event would report this error?

The source code contains lots of global and local variables along with lot of if-else statements, which is not a good practice to write a code, and causing ambiguous errors.

Yes, that's a valid criticism.

[Saumya-Mishra9129]

For the division by zero, can you explain under what conditions the number after the division will be zero?

The error exists on line

stick-hero-activity/main.py

Line 833 in 87ad9fb

acc = abs(((pillarfast) - (sx(429, 1) + pillardist)) / time)

when time becomes zero , i.e. when we divide the whole expression by 0, Hence Zero Division error.
time is initialized to

stick-hero-activity/main.py

Line 830 in 87ad9fb

time = abs((heropointer) / speed)

,
heropointer is initialized as zero many times in main.py . Hence by any way heropointer will become zero, error will be raised. We can use exception handler to set time value to some constant whenver it becomes zero.

For the no free channels during mousemove_cb, it may be consequence of the division by zero. If not, can you explain what the callback is needed for? From the source of Pygame can you explain why creating an event would report this error?

Looked at Pygame source code, couldn't find anything related to this error, but I guess it is raised by pygame.mixer. Documentation says The mixer module has a limited number of channels for playback of sounds. Usually programs tell pygame to start playing audio and it selects an available channel automatically. The default is 8 simultaneous channels, but complex programs can get more precise control over the number of channels and their use. , Do we need to specify number of channels 😕 , I dont know my speculation is correct or not.

[quozl]

Thanks. The problem I see with your explanation about the division by zero is that it is happening in the condition the hero has to stop. When heropointer is zero, the hero has not yet moved. How is it that the hero has to stop before it has moved? I think something is wrong with the hero has to stop test or the variables it depends on. You did change the conditional expression.

Tested faa007e on Ubuntu 20.04;

• starting the activity and then pressing ctrl+q caused this error;

Traceback (most recent call last):
  File "/usr/share/sugar/activities/StickHero.activity/main.py", line 348, in make
    gameDisplay.fill(white)
pygame.error: display Surface quit

my guess is that this is caused by one of the several ways in which the activity is not complying with the Sugargame API; in particular there's no handler for the stop button, and the Pygame display is being initialised within the iterating function instead of the class initialiser.

• sometimes the activity logs this message

Traceback (most recent call last):
  File "/usr/share/sugar/activities/StickHero.activity/main.py", line 1157, in make
    pygame.display.update()
pygame.error: video system not initialized

my guess is that the pygame.quit has been called but in failing to return False the Sugargame API has called the iterating function again. But I'm really not sure.

Incidental discovery while reading code;

• there are two calls to pygame.event.get(); this will cause some events to be lost,

Lastly, the most important problem as I've already mentioned;

• the game cannot be played, because the plank ceases to extend when the up arrow key is held down.

I did some analysis. The iterating function (game.make) does not properly handle keyboard repeat events. I proved this by adding event logging and I saw that holding down the up arrow key generated "273 keyup, 273 keydown, 273 keyup, 273 keydown" until I released it. Switching to using the mouse button seems to fix it; I can make the plank as long or short as I wish;

diff --git a/main.py b/main.py
index 2f619db..1543c48 100755
--- a/main.py
+++ b/main.py
@@ -19,6 +19,7 @@
 # Contact information:
 # Utkarsh Tiwari    iamutkarshtiwari@gmail.com
 
+import logging
 import os
 import gi
 gi.require_version('Gtk', '3.0')
@@ -334,8 +335,14 @@ class game:
                     crashed = True
                 elif event.type == pygame.KEYDOWN and event.key == 273:
                     keydown_pressed = True
+                    logging.error('273 keydown')
                 elif event.type == pygame.KEYUP and event.key == 273:
                     keyup_pressed = True
+                    logging.error('273 keyup')
+                elif event.type == pygame.MOUSEBUTTONDOWN:
+                    keydown_pressed = True
+                elif event.type == pygame.MOUSEBUTTONUP:
+                    keyup_pressed = True
 
             mos_x, mos_y = pygame.mouse.get_pos()
 
@@ -1163,10 +1170,6 @@ class game:
 
         # Just a window exception check condition
 
-        event1 = pygame.event.get()
-        if event1.type == pygame.QUIT:
-            crashed = True
-
         if crashed == True:
             pygame.quit()
             return

Of course, switching to using mouse button keys isn't necessarily the right thing to do, I just did it to demonstrate that the game is vulnerable to keyboard repeat events. Mouse buttons don't issue repeat events.

[Saumya-Mishra9129]

I did some analysis. The iterating function (game.make) does not properly handle keyboard repeat events. I proved this by adding event logging and I saw that holding down the up arrow key generated "273 keyup, 273 keydown, 273 keyup, 273 keydown" until I released it. Switching to using the mouse button seems to fix it; I can make the plank as long or short as I wish;

Thanks @quozl , I am able to play with keys and mouse both. I still don't know why my system doesnt respond with an error. However if switching to mousebuttons does solve error, isn't it a good idea to shift to mouse buttons for the game, if keys aren't responding perfectly.

[quozl]

Re: ee98c5f there is already a ctrl+q accelerator in the definition of StopButton, and you're replacing this with a ctrl+shift+q ... why?

I still don't know why my system doesn't respond with an error.

Different system and configuration, I guess.

In another test, I removed and event.key == 273 and then used the shift key, which is not an auto repeating key. It worked fine, which shows that it can work with keys.

In my case;

• my keyboard auto repeat delay is 240 mS, default is 660 mS,
• my keyboard auto repeat rate is 40 Hz, default is 25 Hz,

You can change keyboard auto repeat parameters using xset r.

I switched to default, and the problem went away. You can ignore the problem now, unless you want to fix it.

If the code is properly designed for the sequence of events that arrives, this difference shouldn't be a problem.

I also tried calling pygame.key.set_repeat with no arguments, per https://www.pygame.org/docs/ref/key.html but no change.

[quozl]

With GSoC over, I'm merging this so that further development can occur with others.

[JuiP]

Thanks @quozl
Just an observation : The recent changes for stick-hero activity can also be applied to sonic jump. It suffers from similar issues.