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

Refactored code and created separate classes for Ball, Brick and Bat #20

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

Conversation

jriyyya
Copy link

@jriyyya jriyyya commented Jul 5, 2023

Hi @chimosky and @sourabhaa please review, This PR refactors and restructures the code for this activity. I was trying to implement the powerups but as it turned out it would've been very difficult to do so with the existing code structure. The ball was coded in as a single variable, its velocity and states as well. also the code had redundant variables and many variables were being redeclared every function call which made the entire process hectic.

Thus, I ended up refactoring the code in a manner where implementing the powerups will be easier. I will be having a separate PR for powerups sometime later this month as I will be working on classify-cats now.

- There were lots of unused variables in game.py which were
  decalred in almost every method of BallAndBrick class
  without being used
- The class handles movement, drawing and collisions

- Having a seperate class for the ball will allow for
  having multiple balls at once if needed
- made a new class called Bat which encapsulates the
  scroller functionality. It acts as a bat

- It can be moved, it handles drawing and clamps
  its position inside the screen
- as of now only encapsulates static brick
  functionality, has no collision checks and
  acts as a rectangle with draw method
- added is_lost, set_position
- also changed bounce_against
- gameLoop has been modified
- states are now defined in an enum
- balls are now stored in an array
  this will be helpful if there are multiple
  balls at the same time in future
  for eg: with powerups
- collision checking and State management logic
  has changed slightly:
ball.py Show resolved Hide resolved
ball.py Outdated Show resolved Hide resolved
ball.py Outdated Show resolved Hide resolved
ball.py Outdated Show resolved Hide resolved
bat.py Show resolved Hide resolved
ball.py Outdated Show resolved Hide resolved
brick.py Outdated Show resolved Hide resolved
game.py Outdated Show resolved Hide resolved
game.py Outdated Show resolved Hide resolved
activity.py Outdated Show resolved Hide resolved
Copy link
Member

@chimosky chimosky left a comment

Choose a reason for hiding this comment

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

Flake8 fixes are better left for a different PR, you could open another PR with these.

@chimosky
Copy link
Member

chimosky commented Jul 5, 2023

Reviewed 49eada6, not tested.

@jriyyya
Copy link
Author

jriyyya commented Jul 5, 2023

The flake8 fixes are mostly of the code that is committed in this PR

@chimosky
Copy link
Member

chimosky commented Jul 6, 2023

The flake8 fixes are mostly of the code that is committed in this PR

I'm saying open a different PR for it, this PR is about refactoring and not flake8 fixes.

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

Tested 860901c, activity opens to a black canvas so I've opened #21 as it also happens on master.

@jriyyya
Copy link
Author

jriyyya commented Jul 27, 2023

Can you please explain what you mean here, because it is working fine with me

@quozl
Copy link
Contributor

quozl commented Jul 27, 2023

Black canvas on Sugargame based activities can have a number of causes, and I've had a quick look at master and don't find the obvious causes.

That it is reproducible for one person and not another is common, because it has to do with a race condition.

You can both help push this forward by answering a few questions;

  • how are you starting the activity?
  • have you restarted Sugar since changing the activity files?
  • does the Sugargame test activity cause the same problem on your environment?

Next coding level diagnosis may be to compare the code against the Sugargame test activity. During development of Sugargame we found there were several ways in which it could fail to start.

@chimosky
Copy link
Member

how are you starting the activity?

From the activity ring.

have you restarted Sugar since changing the activity files?

Yes.

does the Sugargame test activity cause the same problem on your environment?

No it doesn't.

Next coding level diagnosis may be to compare the code against the Sugargame test activity. During development of Sugargame we found there were several ways in which it could fail to start.

Haven't done any comparisons yet but will as soon as I can.

@jriyyya
Copy link
Author

jriyyya commented Jul 28, 2023

  • how are you starting the activity?

From the terminal through the sugar-activity3 . command

  • have you restarted Sugar since changing the activity files?

Yes

  • does the Sugargame test activity cause the same problem on your environment?

No

I will also look into the sugargame test activity for diagnosis

@quozl
Copy link
Contributor

quozl commented Jul 28, 2023

Okay, we have seen difference in behaviour like this when using sugar-activity3 directly, because there are missing environment variables and process partners. When testing for black or blank canvas, please also test with the Sugar activity ring and sugar-activity3 separately.

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.

None yet

3 participants