-
Notifications
You must be signed in to change notification settings - Fork 125
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
Remove pygame code (fixes #186) #219
Conversation
@Ditti4, thanks for your contribution! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Ditti4 Thanks!
rose/client/finish.py
Outdated
@@ -11,16 +10,5 @@ def __init__(self): | |||
|
|||
# Component interface | |||
|
|||
def init(self): | |||
self.texture = pygame.image.load(config.finish_line_png) | |||
|
|||
def update(self, info): | |||
self.timeleft = info["timeleft"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is used only for rendering, I think we can remove the entire module.
rose/client/dashboard.py
Outdated
@@ -18,31 +17,6 @@ def __init__(self): | |||
self.players = {} | |||
self.timeleft = config.game_duration | |||
|
|||
def init(self): | |||
self.texture = pygame.image.load(config.dashboard_png) | |||
|
|||
def update(self, players, timeleft): | |||
self.timeleft = timeleft | |||
self.players = players |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The dashboard is used only for rendering, we can remove the entire module.
rose/client/game.py
Outdated
car.init() | ||
self.finish_line.init() | ||
self.draw(self.surface) | ||
|
||
def update(self, info): | ||
self.track.update(info) | ||
self.players = {p["name"]: p for p in info['players']} | ||
self.dashboard.update(self.players, info["timeleft"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can be removed
rose/client/game.py
Outdated
car.init() | ||
self.finish_line.init() | ||
self.draw(self.surface) | ||
|
||
def update(self, info): | ||
self.track.update(info) | ||
self.players = {p["name"]: p for p in info['players']} | ||
self.dashboard.update(self.players, info["timeleft"]) | ||
for player in self.players.itervalues(): | ||
self.cars[player['car']].update(player) | ||
self.finish_line.update(info) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can be removed
Removing those two modules seems to have worked pretty well. I must've overlooked that those weren't used anymore. Sorry about that and thanks for pointing it out! |
number_of_cars = 4 | ||
car_jitter = 10 | ||
play_sound = True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "Dashboard", "Finish line", and "Window" are also not needed now.
The the "Files" section, we can remove the paths to the various resources (e.g. obstables_dir). I think the server is using only the install_dir to serve the resources.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The the "Files" section, we can remove the paths to the various resources (e.g. obstables_dir). I think the server is using only the install_dir to serve the resources.
Yup, just tried that. Only keeping the install_dir
(well, and web_root
and res_root
) seems to work. I wasn't sure about removing that first so I kept it in. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good
@@ -1,5 +1,4 @@ | |||
autobahn | |||
pygame |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We introduced duplication when adding the pipenv configuration. Note to self: open an issue for generating the requirements file from pipenv.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good
@Ditti4, Removing the rest of the unused configuration, it looks ready. |
@Ditti4 but testing show that moving error to server package is incorrect - it is used by common/message. We need to move it back to common.
Unfortunately testing coverage is very low, so you must run both server and clients to check that everything still work.
Thanks! |
Surprisingly enough, I followed those exact steps before every push. Looking at my local code base though, I can still see error.pyc in the common directory, so python must've used that one, ignoring the fact that the original .py file does not exist anymore. |
Hmm, the examples still use the actions module. Should we move that back as well, considering it's not only used by the server this way either? |
@Ditti4 Right, the actions module is used by the driver code, we need to keep this module in the common package as well. For another PR: It would be nice to write a test for client code loading a driver module, detecting such issues. |
@Ditti4 useful way to avoid stale pyc files issue is to clean your tree before testing:
Note that it will delete anything which which git does not know about, including temporary files you may have created ;-) |
Do you mind if I go ahead and just remove the previous commit (which moved the two modules) using a rebase so the history gets cleaned up a little or would you prefer a revert? |
@Ditti4 I like clean history. Best remove the commit using "git rebase -i" and push again using --force. I would like to rebase and merge the commits, without squashing them. |
@nirs Should I also mark the two commits removing more modules and more config options as fixups of their original commits or do you want me to keep those separate? Locally I currently have them marked as fixup so we end up with total amount of three commits. |
@Ditti4 yes, fixing the original commit instead of adding patches is preferred. |
Then there you go, all fixed up and hopefully final. Thanks for your guidance! :) |
Some imports and instance variables are not needed now in the client package, after pygame was removed in RedHat-Israel#219.
Some imports and instance variables are not needed now in the client package, after pygame was removed in #219.
The commits in this pull request will (hopefully) remove all the pygame-related code, including all the rendering code and hopefully leaving no unused code behind, like it was requested in #186.
If you happen to find anything that you don't agree with or that you'd like me to change, just let me know. :)