Adapt to display size #25

JuiP · 2020-06-03T10:36:09Z

WIP on Adapt to display size #19
Fixed the calculations of hard coded coordinates. ( + 45) helps to center it
The activity coordinates are hard coded for a system with display resolution as 1280*720. Scaling factor extends support to other screen dimensions.

I'm working on the display resolution fix for the game, just finished fixing the welcomescreen, "how to play" and the scoreboard screen. I will make flake8 fixes in another commit.
@quozl used pygame instead of Gtk this time ;-)

Fixes #17.

@chimosky review changes till de21969 ?

JuiP · 2020-06-04T12:01:47Z

In all the commits above +45 is used to center the activity. The hard coded coordinates were wrongly determined. I determined the +45 shift by using the loaded image dimensions. I have two options-

I can either add it to the coordinates or leave it as +45
Use a global variable for this +45 shift and use it elsewhere.

However, I feel declearing a global variable does not add to code readability for the future (in this case). This shift should have been acounted for while using direct coordinates for 1280*720 display and adding it as another variable will make the code more complicated to understand. So I just added it in the commit message instead. What do you suggest @quozl @chimosky @srevinsaju ?

srevinsaju · 2020-06-04T12:12:44Z

I suggest you to use a variable called SCALE_SHIFT, or SCALE_MULTIPLIER, because this increases the maintainability for future contributors. Alternatively, you may also hardcode the added value, instead of leaving it as +45. I strongly disagree leaving it as +45, because a newbie who is looking into the code will be like, what is this 45?.

I would suggest

Hardcode values
create a global variable

by priority, you may like to choose wither of the two; And we will look for other's suggestion too.

JuiP · 2020-06-04T12:57:11Z

I strongly disagree leaving it as +45, because a newbie who is looking into the code will be like, what is this 45?.

Yes I agree with you, that's exactly why I asked for suggestions! :)

Once this PR is reviewed and merged, @Saumya-Mishra9129 would you like to port it to python3 and update sugargame version to 1.3? We can make a release after this maybe. :)

Saumya-Mishra9129 · 2020-06-04T15:10:44Z

I strongly disagree leaving it as +45, because a newbie who is looking into the code will be like, what is this 45?.

Yes I agree with you, that's exactly why I asked for suggestions! :)

Once this PR is reviewed and merged, @Saumya-Mishra9129 would you like to port it to python3 and update sugargame version to 1.3? We can make a release after this maybe. :)

Yeah Sure.

chimosky · 2020-06-05T01:10:02Z

Tested, thanks. Reviewed 8acaf49.

@JuiP said

I determined the +45 shift by using the loaded image dimensions.

How did you determine it exactly? Also if the hard coded values can be avoided we should avoid them.

@JuiP said

However, I feel declearing a global variable does not add to code readability for the future (in this case).

A global variable should be used because there's too many 45s and it's difficult to keep track of.
The variable can be created in one file and imported wherever it's needed.

8acaf49 doesn't completely fix #17, the only part of #17 that's fixed is After game over, pressing any key restarts the game.

when the help button is clicked it shows the help and no way to go back to the home but it shows a Let's play button and when it's clicked it shows GAME OVER! while game wasn't played. isn't fixed yet, it can be reproduced by;

Start activity.
Play and fail on first try.
Click Home.
Click Help.
Click Let's play.

Game also seems to freeze sometimes, nothing shows in the log

Found a way to bypass levels easily by just creating a stick when I'm in front of one of the red patches on a pillar.

Also can you explain your fix in a commit message and not just have Fixes #issueno as the commit message?

srevinsaju · 2020-06-05T07:23:08Z

A global variable should be used because there's too many 45s and it's difficult to keep track of.
The variable can be created in one file and imported wherever it's needed.

@chimosky Can you please let me know the benefits of keeping it as a variable? I guess most of the width and height is already hardcoded and are not derived from any variable. I would prefer hardcoding them because it would reduce a lot of unwanted mess

Before / now

(int((57 + 45) * scale_x), int((39 + 45) * scale_y))), (birdxfast, 400 * scale_y))

OR

(int((57 + SHIFT_CORRECTOR) * scale_x), int((39 + SHIFT_CORRECTOR) * scale_y))), (birdxfast, 400 * scale_y))

TO (after hardcoding)

(int(102 * scale_x), int(84 * scale_y)), birdxfast, 400 * scale_y)

Game also seems to freeze sometimes, nothing shows in the log

The freeze is because of #28

Found a way to bypass levels easily by just creating a stick when I'm in front of one of the red patches on a pillar.

Wow; that's an important bug to fix. There is some chance that it might get fixed by merging #27. It was supposed to fix some loop errors.

chimosky · 2020-06-05T11:15:27Z

@srevinsaju said

@chimosky Can you please let me know the benefits of keeping it as a variable? I guess most of the width and height is already hardcoded and are not derived from any variable. I would prefer hardcoding them because it would reduce a lot of unwanted mess

The benefit being as they're hard coded, the values could also be changed again and having to change every 45 is definitely hectic and unnecessary.

@srevinsaju said

The freeze is because of #28

If it was I should have been able to reproduce it but there was nothing in the logs.

@srevinsaju said

Wow; that's an important bug to fix. There is some chance that it might get fixed by merging #27. It was supposed to fix some loop errors.

I'm yet to test #27, I'll get to it soon.

JuiP · 2020-06-05T11:52:47Z

How did you determine it exactly?

You can see the coordinates and widths of the black rectangles in this line I used them to determine the shift to center it.

when the help button is clicked it shows the help and no way to go back to the home but it shows a Let's play button and when it's clicked it shows GAME OVER! while game wasn't played. isn't fixed yet, it can be reproduced by;

Could you please have a look at this comment?

Game also seems to freeze sometimes, nothing shows in the log

I agree with @srevinsaju. This is because of #28. Each time the game freezed, I got this traceback.

The benefit being as they're hard coded, the values could also be changed again and having to change every 45 is definitely hectic and unnecessary.

👍 So I will change the +45 to SHIFT_CORRECTOR.

Found a way to bypass levels easily by just creating a stick when I'm in front of one of the red patches on a pillar.
Wow; that's an important bug to fix. There is some chance that it might get fixed by merging #27. It was supposed to fix some loop errors.

From looking at the code, @srevinsaju merging #27 will not fix this. This bug is due to the coordinates, will look at it!

Also can you explain your fix in a commit message and not just have Fixes #issueno as the commit message?

Noted!

JuiP · 2020-06-12T13:28:42Z

I'm mostly done working on this activity, made changes as requested.
@chimosky @quozl please review and merge :)

srevinsaju · 2020-06-12T15:09:15Z

welcomescreen.py


            pygame.display.update()
            clock.tick(60)

-            if crashed == True:                                   # Game crash or Close check
+            if crashed == True:
+                # Game crash or Close check
                pygame.quit()
                sys.exit()


I would suggest sys.exit() to be removed. Using sys.exit is not probably the right way to exit the loop. Use a return to terminate the loop gracefully. (I am not sure, if its applicable under this PR, but anyway, it would have to be removed before the last python2 release, I suppose)

Thanks for reviewing! :)
I don't agree. welcomescreen.py , rules.py and scorescreen.py return certain values. These values are used in if constructs and have a meaning. You can see here and here too. Using a return statement instead of sys.exit() might complicate how a contributor interprets code.

Also I'm not aware of the benefits of using return instead of sys.exit(). Could you please tell me why you suggested this, might help in some other activity :)

chimosky · 2020-06-15T18:15:04Z

Thanks, tested d567207.

On first run the game hung after I failed and this was shown in the logs;

Traceback (most recent call last):
  File "/home/ibiam/Activities/stick-hero-activity/main.py", line 1083, in make
    catch = a.make(gameDisplay, back, score, fruitscore)
  File "/home/ibiam/Activities/stick-hero-activity/scorescreen.py", line 202, in make
    if event.type == pygame.MOUSEBUTTONUP:
UnboundLocalError: local variable 'event' referenced before assignment
1592244603.460986 ERROR root: Event queue full!
1592244603.477404 ERROR root: Event queue full!
1592244603.494011 ERROR root: Event queue full!
1592244603.527771 ERROR root: Event queue full!
1592244603.543533 ERROR root: Event queue full!
1592244603.561056 ERROR root: Event queue full!
Traceback (most recent call last):
  File "/home/ibiam/Activities/stick-hero-activity/sugargame/event.py", line 110, in _quit_cb
    pygame.event.post(pygame.event.Event(pygame.QUIT))
pygame.error: Event queue full
Traceback (most recent call last):
  File "/home/ibiam/Activities/stick-hero-activity/sugargame/event.py", line 107, in _screen_changed_cb
    self.update_display()
  File "/home/ibiam/Activities/stick-hero-activity/sugargame/event.py", line 96, in update_display
    pygame.event.post(pygame.event.Event(pygame.VIDEOEXPOSE))
pygame.error: Event queue full

After restarting the game, on second run the game freezes and this is shown in the logs;

Traceback (most recent call last):
  File "/home/ibiam/Activities/stick-hero-activity/main.py", line 1083, in make
    catch = a.make(gameDisplay, back, score, fruitscore)
  File "/home/ibiam/Activities/stick-hero-activity/scorescreen.py", line 202, in make
    if event.type == pygame.MOUSEBUTTONUP:
UnboundLocalError: local variable 'event' referenced before assignment

I understand the second error might have been fixed in #27, I'll test it again as I wasn't able to get any errors when I tested earlier.

JuiP · 2020-06-15T18:25:41Z

Thanks for testing :)
I think the errors which you encountered in both of your tests are due to the same reason.... might be fixed by #27 .

I'll test it again as I wasn't able to get any errors when I tested earlier.

I'm not sure what you are suggesting, did you get these errors only after I pushed the recent commits? I can reproduce the same errors on master. But they happen occassionally not on each test

JuiP · 2020-06-17T17:50:13Z

I guess this PR is good to be merged? @chimosky @quozl ?

quozl · 2020-06-22T01:40:33Z

I've had a brief look. Review is made harder by flake8 changes. Are these needed within the pull request or can they be done later?

JuiP · 2020-06-22T04:42:53Z

Thanks! In the last commit d567207 , I changed +45 to SHIFT_CORRECTOR that significantly increased the line length and gave a lot of flake8 warnings. So I thought fixing them in this PR might actually help.... Should I delete the last commit and open another PR for that? @quozl

quozl · 2020-06-22T07:06:39Z

Thanks. I've had another brief look. The new X and Y scaling feature for adapting to display size is good, but sloppy. You have a global constant defined in several files. You have a repeating pattern of using the constant and some other constants that aren't defined. It's the sort of situation where a new pair of functions (e.g. mx() and my()) might be used instead.

One or more of your commits fixes problems introduced in your earlier commits; you could squash those and not mention the problem.

Some of your commit messages don't follow our guide to making commits.

Hope that helps.

JuiP · 2020-06-22T07:28:14Z

Some of your commit messages don't follow our guide to making commits.

I'll be more careful next time! :)

It's the sort of situation where a new pair of functions (e.g. mx() and my()) might be used instead.

I'm not too sure how exactly do you want me to use the functions instead of the constants. I understand, that by explaining "how" you would be solving a part of the issue, I'm sorry I didnt quite understand how the functions can be implemented.

quozl · 2020-06-22T08:06:53Z

I'm not too sure how exactly do you want me to use the functions instead of the constants. I understand, that by explaining "how" you would be solving a part of the issue, I'm sorry I didnt quite understand how the functions can be implemented.

I don't want to fall into the trap of being prescriptive, because then I'm the one doing the work instead of you, and all you become is a slave. I'm not a university lecturer. 😀 Can you think of how to avoid (a) having the constant in several different files, and (b) having a common scaling algorithm repeated throughout the program? (x = ax + b)

JuiP · 2020-06-22T13:32:50Z

Can you think of how to avoid (a) having the constant in several different files, and (b) having a common scaling algorithm repeated throughout the program? (x = ax + b)

😄 Thanks! This makes it clearer.... working on it now! :)

JuiP · 2020-06-23T05:57:12Z

@quozl Implemented what you suggested and also squashed commits :) Please have a look now

quozl · 2020-06-23T06:50:33Z

Do you like it better? I do. Further review;

scale_img is always preceded by an image load, should it do the image load as well?
documentation for sx, sy, and scale_img are misplaced a few times,
"Fixes 17" is (a) not imperative mood, and (b) doesn't describe the problem. GitHub issues are archived automatically when closed, and so referencing issues without describing them makes history distort later.

Tested briefly. Still some actions by user are ignored, along with traceback of UnboundLocalError: local variable 'event' referenced before assignment, in line 706 of main.py.

welcomescreen.py

JuiP · 2020-06-23T07:05:21Z

Thanks 😄 @quozl

scale_img is always preceded by an image load, should it do the image load as well?

Yes, I think it would be a good idea to put it in scale_img.

documentation for sx, sy, and scale_img are misplaced a few times

By misplaced, did you refer to the lines I marked in this comment?

"Fixes 17" is (a) not imperative mood, and (b) doesn't describe the problem. GitHub issues are archived automatically when closed, and so referencing issues without describing them makes history distort later.

I have described the problem in the short commit message below, but I do understand your point... I will add a one line summary instead :)

along with traceback of UnboundLocalError: local variable 'event' referenced before assignment, in line 706 of main.py.

I think that's out of scope for this PR, @srevinsaju made another PR to fix this in #27

Still some actions by user are ignored

Could you please mention the actions that were ignored? My guess is it is to be fixed in #27

quozl · 2020-06-23T07:10:08Z

along with traceback of UnboundLocalError: local variable 'event' referenced before assignment, in line 706 of main.py.

I think that's out of scope for this PR, @srevinsaju made another PR to fix this in #27

I don't understand. If you think his fix is worth having, why isn't it in your branch?

Still some actions by user are ignored

Could you please mention the actions that were ignored? My guess is it is to be fixed in #27

Keyboard actions.

JuiP · 2020-06-23T07:15:55Z

Yes, that's actually easier, I'll cherry-pick the commit but I'm slightly confused about your comment. We (@srevinsaju @JuiP @chimosky @Saumya-Mishra9129) tested the change, it worked. Do you think that it is the correct way to do it and I should add the commit here?

quozl · 2020-06-23T07:18:08Z

You have the power. Use it wisely? I've not tested the change. I'm worried that by changing to handling one event per iteration the event loop is entirely different to what it was before, but if everyone is happy with it I guess it should be okay. I just haven't seen anyone say anything about it.

srevinsaju · 2020-06-23T07:24:43Z

I never got it to work in the past, so I am not sure of the mechanism it used previously. @quozl How better can it be reimplemented? Do you have anything in your mind which you would like to propose? I did not understand the difference in the current implementation, and that of which was (attempted) to be implemented in the past. But I can surely say the difference between #27 and #24

I'm worried that by changing to handling one event per iteration the event loop is entirely different to what it was before

TL;DR > what was it before? I might have misunderstood the idea then.

quozl · 2020-06-23T07:28:09Z

Yeah, it was always wrong. But it was a slightly better wrong. 😁 I'll comment over in #27.

- Fixed the calculations of hard coded coordinates. ( + 45) helps to center it - The activity coordinates are hard coded for a system with display resolution as 1280*720. Scaling factor extends support to other screen dimensions. - Centre the activity canvas and support multiple resolutions. Fixes sugarlabs#19

- Pressing any key after game over cannot restart the game - This fixes : After clicking help button followed by Let's Play button, it takes you back to the scoreboard without playing a game - Reinitializing variables after Game Over ensures that it does not get into the if(fall == 1 or ext ==1) block - Fix sugarlabs#17

…on documentation

JuiP · 2020-07-03T12:59:16Z

@quozl @chimosky @srevinsaju Have a look at the recent changes :) Maybe you could also update this comment

Mostly finished work on this PR, could you please review and merge?

chimosky · 2020-07-03T14:54:25Z

Tested, reviewed.

Activity works as expected, thanks.

edited opening comment

In relation to @quozl's statement here, in 3ae945 you added a "Fix #" and his comment was to point out that that shouldn't be included in the commit message as it's recognized by github but not git so it's not needed.

I'll wait for @srevinsaju and @quozl who're involved to review.

- Fixes sugarlabs#26

JuiP · 2020-07-03T17:32:09Z

In relation to @quozl's statement here, in 3ae945 you added a "Fix #" and his comment was to point out that that shouldn't be included in the commit message as it's recognized by github but not git so it's not needed.

Ohh got it, won't include that next time at all 👍

@quozl asked me to add changes in #27 if I agree to it, so I added it here.

srevinsaju

Tested until 16a7f63 . No errors in logs.

srevinsaju · 2020-07-03T17:54:00Z

Closes #27

chimosky · 2020-07-06T13:05:58Z

Tested, works as expected. Thanks.

JuiP marked this pull request as ready for review June 4, 2020 12:02

Saumya-Mishra9129 mentioned this pull request Jun 4, 2020

Port to Python3 and Update sugargame JuiP/stick-hero-activity#1

Closed

srevinsaju reviewed Jun 12, 2020

View reviewed changes

JuiP force-pushed the Fixes branch from d567207 to 58dd71b Compare June 22, 2020 13:55

JuiP marked this pull request as draft June 22, 2020 13:56

JuiP force-pushed the Fixes branch from 58dd71b to 516e6dc Compare June 23, 2020 05:51

JuiP marked this pull request as ready for review June 23, 2020 05:51

JuiP force-pushed the Fixes branch from 516e6dc to a2921d6 Compare June 23, 2020 05:55

JuiP commented Jun 23, 2020

View reviewed changes

welcomescreen.py Outdated Show resolved Hide resolved

srevinsaju mentioned this pull request Jun 23, 2020

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

Closed

JuiP added 3 commits July 3, 2020 18:18

Change style from wildcard imports to explicit imports and fix functi…

b0656a6

…on documentation

JuiP force-pushed the Fixes branch from 40e7943 to b0656a6 Compare July 3, 2020 12:55

Fix event poll in pygame and fix UnboundLocalError

16a7f63

- Fixes sugarlabs#26

srevinsaju approved these changes Jul 3, 2020

View reviewed changes

chimosky merged commit 6390820 into sugarlabs:master Jul 6, 2020

JuiP mentioned this pull request Jul 21, 2020

IndexError: pixel index out of range #28

Closed

Adapt to display size #25

Adapt to display size #25

JuiP commented Jun 3, 2020 •

edited by chimosky

JuiP commented Jun 4, 2020

srevinsaju commented Jun 4, 2020 •

edited

JuiP commented Jun 4, 2020 •

edited

Saumya-Mishra9129 commented Jun 4, 2020

chimosky commented Jun 5, 2020 •

edited

srevinsaju commented Jun 5, 2020

chimosky commented Jun 5, 2020 •

edited

JuiP commented Jun 5, 2020

JuiP commented Jun 12, 2020

srevinsaju Jun 12, 2020

JuiP Jun 13, 2020

chimosky commented Jun 15, 2020

JuiP commented Jun 15, 2020

JuiP commented Jun 17, 2020

quozl commented Jun 22, 2020

JuiP commented Jun 22, 2020

quozl commented Jun 22, 2020

JuiP commented Jun 22, 2020

quozl commented Jun 22, 2020

JuiP commented Jun 22, 2020

JuiP commented Jun 23, 2020

quozl commented Jun 23, 2020

JuiP commented Jun 23, 2020

quozl commented Jun 23, 2020

JuiP commented Jun 23, 2020 •

edited

quozl commented Jun 23, 2020

srevinsaju commented Jun 23, 2020 •

edited

quozl commented Jun 23, 2020

JuiP commented Jul 3, 2020

chimosky commented Jul 3, 2020 •

edited

JuiP commented Jul 3, 2020

srevinsaju left a comment •

edited

srevinsaju commented Jul 3, 2020

chimosky commented Jul 6, 2020

Adapt to display size #25

Adapt to display size #25

Conversation

JuiP commented Jun 3, 2020 • edited by chimosky

JuiP commented Jun 4, 2020

srevinsaju commented Jun 4, 2020 • edited

JuiP commented Jun 4, 2020 • edited

Saumya-Mishra9129 commented Jun 4, 2020

chimosky commented Jun 5, 2020 • edited

srevinsaju commented Jun 5, 2020

chimosky commented Jun 5, 2020 • edited

JuiP commented Jun 5, 2020

JuiP commented Jun 12, 2020

srevinsaju Jun 12, 2020

Choose a reason for hiding this comment

JuiP Jun 13, 2020

Choose a reason for hiding this comment

chimosky commented Jun 15, 2020

JuiP commented Jun 15, 2020

JuiP commented Jun 17, 2020

quozl commented Jun 22, 2020

JuiP commented Jun 22, 2020

quozl commented Jun 22, 2020

JuiP commented Jun 22, 2020

quozl commented Jun 22, 2020

JuiP commented Jun 22, 2020

JuiP commented Jun 23, 2020

quozl commented Jun 23, 2020

JuiP commented Jun 23, 2020

quozl commented Jun 23, 2020

JuiP commented Jun 23, 2020 • edited

quozl commented Jun 23, 2020

srevinsaju commented Jun 23, 2020 • edited

quozl commented Jun 23, 2020

JuiP commented Jul 3, 2020

chimosky commented Jul 3, 2020 • edited

JuiP commented Jul 3, 2020

srevinsaju left a comment • edited

Choose a reason for hiding this comment

srevinsaju commented Jul 3, 2020

chimosky commented Jul 6, 2020

JuiP commented Jun 3, 2020 •

edited by chimosky

srevinsaju commented Jun 4, 2020 •

edited

JuiP commented Jun 4, 2020 •

edited

chimosky commented Jun 5, 2020 •

edited

chimosky commented Jun 5, 2020 •

edited

JuiP commented Jun 23, 2020 •

edited

srevinsaju commented Jun 23, 2020 •

edited

chimosky commented Jul 3, 2020 •

edited

srevinsaju left a comment •

edited