-
Notifications
You must be signed in to change notification settings - Fork 1
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
Multiplayer over the network #32
base: main
Are you sure you want to change the base?
Conversation
…ons to env files; empty fields in problem files not not required
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.
For context, when we tested this code there was a state bug across sessions where previously done actions were unable to be completed. I poked around with the networking and looked through the code and do not think this is a networking issue - rather an issue with the environment itself. We can ignore this for now - I will have @lsuyean look into a fix for this. In the meantime, the code needs to be reorganized. Can we have the server and client functionality split into separate files and have some documentation for the networking options, preferably in a networking README.md
. Rather than intertangling networking with Robotouille, it would be good to have an option in main.py
to run the default simulator
which a researcher trying to run an RL agent would prefer while having an option for the networking server-client for those wanting to run user studies.
robotouille/robotouille_simulator.py
Outdated
keydown_events = list(filter(lambda e: e.type == pygame.KEYDOWN, pygame_events)) | ||
action, args = create_action_from_control(env, shared_state["obs"], player, mousedown_events + keydown_events, renderer) | ||
|
||
online = not (pygame.key.get_mods() & pygame.KMOD_CAPS) |
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.
what is this for?
robotouille/robotouille_simulator.py
Outdated
|
||
while not done: | ||
# Wait for messages from any client | ||
# lol this is causing a memory leak |
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.
make this is a TODO to fix 😃 format should be # TODO(username): ...
robotouille/robotouille_simulator.py
Outdated
async for message in websocket: | ||
await q.put(message) | ||
|
||
#start_server = websockets.serve(handle_connection, "0.0.0.0", 8765) |
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.
clear comments
robotouille/robotouille_simulator.py
Outdated
continue | ||
obs, reward, done, info = env.step(actions, interactive=interactive) | ||
async def handle_connection(websocket): | ||
# simple lobby code; will break if anyone disconnects, probably has race conditions lol, etc. |
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.
TODO to fix
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 need one more iteration on this PR before we can merge in; mainly we need to ensure anything networking related is in the networking
directory (see comments, especially on the 'biggest feedback').
Other than that, left some comments for code clarity and improvements to make the README.md
better for others to quickly play with networking.
Finally, please add docstrings to all modified functions explaining the purpose of the function and a description of all parameters. Be consistent with the formatting of other functions in the repository.
networking/README.md
Outdated
3. `single` | ||
4. `replay` | ||
5. `render` | ||
6. `simulator` |
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.
nit: call simulator local and move it to 1 so explanations are in order and the first explanation is how to run Robotouille without networking
networking/server.py
Outdated
|
||
while not done: | ||
# Wait for messages from any client | ||
# TODO(aac77): make more robust |
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.
make this TODO more specific - what does 'more robust' mean? create an actionable github issue and refer to it in this PR so we know to address this in the future.
networking/server.py
Outdated
pickle.dump(recording, f) | ||
|
||
async def handle_connection(websocket): | ||
# TODO(aac77): make more robust |
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.
ditto
robotouille/robotouille_simulator.py
Outdated
|
||
def simulator(environment_name: str, seed: int=42, noisy_randomization: bool=False): | ||
async def single_player(environment_name: str, seed: int=42, noisy_randomization: bool=False): |
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 we move the networking related code to a utils
directory under the networking
directory (functions single_player
, replay
, render
)
|
||
def simulator(environment_name: str, seed: int=42, role: str="simulator", display_server: bool=False, host: str="ws://localhost:8765", recording: str="", noisy_randomization: bool=False): | ||
if recording != "" and role != "replay" and role != "render": |
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.
add a comment to this if statement since it takes too long to reason about why we're defaulting to replay if a recording is specified for a gameplaying role
@@ -0,0 +1,40 @@ | |||
# Robotouille Networking README |
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.
I'd add some common usecase commands so others can copy-paste to get a desired result like
- server command with option to display screen
- client command with a specific host to connect to
- how to replay game data onto the simulator
- how to render game data into a video
main.py
Outdated
@@ -4,7 +4,11 @@ | |||
parser = argparse.ArgumentParser() | |||
parser.add_argument("--environment_name", help="The name of the environment to create.", default="original") | |||
parser.add_argument("--seed", help="The seed to use for the environment.", default=None) | |||
parser.add_argument("--role", help="\"simulator\" for vanilla simulator, \"client\" if client, \"server\" if server, \"single\" if single-player, \"replay\" if replaying, \"render\" if rendering video", default="simulator") |
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.
separate the Robotouille arguments and the networking arguments with a newline and have a comment above the networking parameters explaining where to find the networking README.md
robotouille/robotouille_simulator.py
Outdated
|
||
def simulator(environment_name: str, seed: int=42, role: str="simulator", display_server: bool=False, host: str="ws://localhost:8765", recording: str="", noisy_randomization: bool=False): |
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.
biggest feedback is we want to move as much of the networking related code to the networking directory. I'd like to keep this file as clean as possible to the original so someone that looks at this function can easily reason how Robotouille works without needing to have a coupled understanding on the networking. I suggest moving these if statements to a function call within the networking
directory and calling simulate
immediately if the role is appropriate. Here is some psuedocode for what I have in mind to
def simulator(...):
"""...docstring..."""
if role != "local":
# Run networked Robotouille
networked_simulate(...)
simulate(...)
roles replay
and render
also make sense under networking since they're utilities for replaying or rendering data generated by the networking class.
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.
LGTM - left minor comments to resolve before squashing and merging.
main.py
Outdated
@@ -2,9 +2,17 @@ | |||
import argparse | |||
|
|||
parser = argparse.ArgumentParser() | |||
|
|||
# Robotuille game parameters |
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.
Robotouille
networking/README.md
Outdated
This mode runs Robotouille without any networking overhead. | ||
|
||
E.g. | ||
```python main.py --environment_name original``` |
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.
try adding sh
so it looks like a shell command like below (with grave accents ` replacing apostrophe ')
'''sh
python main.py --environment_name original
'''
networking/server.py
Outdated
|
||
while not done: | ||
# Wait for messages from any client | ||
# TODO(aac77): |
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.
make a Git issue and refer to it in the PR as well in the code by the number, e.g. #32
Overview
Implemented client and server functionality to enable cooperative multiplayer. Players are automatically matchmaked into games when enough player connect. Upon the successful conclusion of a game, a replay of the game is saved on the server. Command line arguments configure game and connection settings.
Changes Made
Implemented client and server roles: clients connect to servers
Robotouille game can be played over the internet
Recordings of successful games are saved by the server
Recordings can be played back by the application or rendered into a video
Multiagent environments are supported via multiplayer
Basic matchmaking system assigns multiple players to a game
Test Coverage
Tested manually. Use command line arguments to set up the server and connect as a client.
Next Steps
Making the netcode more resilient to network and computer failures.
Related PRs or Issues
Multiagent
Screenshots
Screenshot of different instances in the same game