Conversation
alejandromarcu
requested review from
ablasdel,
adamantivm,
jonbinney and
muralx
deep_quoridor/src/play.py
Outdated
| nargs="+", | ||
| choices=Renderer.names(), | ||
| default=["progressbar", "arenaresults"], | ||
| default=["arenaresults", "pygame"], |
There was a problem hiding this comment.
probably I will revert this
don't use pygame by default because it hangs github actions
oops, now doing it correctly
muralx
left a comment
muralx
left a comment
There was a problem hiding this comment.
Thanks Cucu, leaving some comments. As Julian suggested, having the HumanAgent that we can use and that can interact with the UI would be nice
deep_quoridor/src/play.py
Outdated
| r.main_thread() | ||
| break | ||
|
|
||
| # thread.join() |
There was a problem hiding this comment.
Any reason to not wait for the thread to finish?
There was a problem hiding this comment.
Probably we should, making the app terminate correctly was a bit tricky, because you can just close the UI window, the game may finish, etc.
deep_quoridor/requirements.txt
Outdated
| prettytable | ||
| torch No newline at end of file | ||
| torch | ||
| pygame No newline at end of file |
| } | ||
|
|
||
| def update_board(self, game, current_player: str, result=None): | ||
| self._msg_to_gui.put( |
There was a problem hiding this comment.
nit: this will likely change. Why not wrap it with something that hides the actual data structure?
There was a problem hiding this comment.
Do you mean that for example game.positions would change once Board is ready? the thing is that I'd need to write some kind of adapter to the new data structure and then change the adapter anyway once board is ready, so I was thinking just to use whatever Board provides, I think it's ok if this class is coupled with Board
There was a problem hiding this comment.
Sounds good, I am also waiting on the new board to do the change.
| m = self.board_size - 1 | ||
|
|
||
| # Vertical walls | ||
| for col in range(m): |
There was a problem hiding this comment.
can you just get the positions from the walls array directly? I think in that case you can avoid the skips too
There was a problem hiding this comment.
I did it thinking that we'll soon switch to Jon's implementation of Board, where we don't store the walls like we do now (position and orientation per 2 segment wall), and instead we have a bitmap of where the walls are.
| button["method"]() | ||
|
|
||
| def _handle_start_pause(self): | ||
| if self.game_state in [GameState.READY, GameState.PAUSED]: |
There was a problem hiding this comment.
should this come from the actual game instead of the UI ? I am referring to the status of the game
There was a problem hiding this comment.
I can try it, let me see how it looks, because usually we won't have another thread controlling it so it will need to account for that, but it can be useful if we want a different Renderer to control the states
There was a problem hiding this comment.
I haven't yet found a better way for this. In the game it would be a bit messy. Maybe in some kind of controller, I'm still not sure what's a good way to do it, but if you don't mind I wanted to proceed with this PR and see if I can figure out a better way later
There was a problem hiding this comment.
No worries, it was not blocking, it was a sense that it might bring issues in the future, but we can fix / figure out what to do when it is needed.
muralx
left a comment
muralx
left a comment
There was a problem hiding this comment.
Thanks Cucu, I did not really review the rendering code (If it draws well, it is good for me)
| pygame_renderer = next((r for r in renderers if isinstance(r, PygameRenderer)), None) | ||
|
|
||
| if pygame_renderer is None: | ||
| self._play_games(players, times) |
There was a problem hiding this comment.
you could in theory use the same code path, and only call pygame renderer if the renderer is selected.
There was a problem hiding this comment.
My idea was to keep it running in the main thread, just because I'm afraid of thread stuff :p. I'll leave it like this for now and consider the change for another time
| } | ||
|
|
||
| def update_board(self, game, current_player: str, result=None): | ||
| self._msg_to_gui.put( |
There was a problem hiding this comment.
Sounds good, I am also waiting on the new board to do the change.
| button["method"]() | ||
|
|
||
| def _handle_start_pause(self): | ||
| if self.game_state in [GameState.READY, GameState.PAUSED]: |
There was a problem hiding this comment.
No worries, it was not blocking, it was a sense that it might bring issues in the future, but we can fix / figure out what to do when it is needed.
2 participants
This is an initial version that allows to run play or replay in pygame. You can pause it or go step by step.
I'll add more features (human players, NN visualization, etc) in the future