-
Notifications
You must be signed in to change notification settings - Fork 2
[1/2] Customers #35
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
base: main
Are you sure you want to change the base?
[1/2] Customers #35
Conversation
…s for container to robotouille.py, made new env json
…ng still buggy regarding stacking predicates
chalo2000
left a comment
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.
thank you for the immense effort on this! I've left a couple of comments and have requested changes just so I can take another look at your responses and minor changes.
backend/customer.py
Outdated
| order to them. | ||
| """ | ||
|
|
||
| customers = {} |
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.
turn these into instance variables so we don't have networking issues
backend/domain.py
Outdated
| raise ValueError(f"Predicate {pred.name} has invalid types.") | ||
|
|
||
| for action in self.actions: | ||
| for action in action_def: |
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.
combine for loop into 1 more for loop
for action_def in [action_def, npc_action_def]:
...
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.
edited accordingly
backend/gamemode.py
Outdated
| @@ -0,0 +1,52 @@ | |||
| from backend.customer import Customer | |||
| class GameMode(object): | |||
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 future, you can import ABC class to make abstract class. look at this reference
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.
used ABC for Gamemode and SpecialEffect
backend/movement/movement.py
Outdated
| player_destinations = [data.path[-1] for name, data in Movement.metadata.items() if data.path and name != player.name] | ||
| player_customer_locations = [p.pos for p in Player.players.values() if p != player] | ||
| player_customer_locations += [c.pos for c in Customer.customers.values() if c != customer and c.in_game] | ||
| # player_customer_destinations = [data.path[-1] for name, data in Movement.metadata.items() if data.path and name != player.name and name != customer.name] |
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.
delete
backend/movement/movement.py
Outdated
| player_customer_locations += [c.pos for c in Customer.customers.values() if c != customer and c.in_game] | ||
| # player_customer_destinations = [data.path[-1] for name, data in Movement.metadata.items() if data.path and name != player.name and name != customer.name] | ||
| player_customer_destinations = [] | ||
| for name, data in Movement.metadata.items(): |
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 unclear, comment more and make it clearer
renderer/canvas.py
Outdated
| """ | ||
| if direction == (0, 1): | ||
| return self.config["player"]["robot"]["back"] | ||
| return self.config[type]["robot"]["back"] |
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.
so ['customer']['robot'][...] is a customer asset? customer isn't a robot. regardless we are hardcoding the "robot" asset anyways; either make this clearer that we are hardcoding by removing ["robot"] and renaming assets or put a TODO to fix this.
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.
Changed the function to take in a name argument
renderer/canvas.py
Outdated
|
|
||
|
|
||
| def _get_player_sprite(self, direction): | ||
| def _get_player_or_customer_image_name(self, direction, type): |
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.
looking throughout the codebase and repeatedly seeing player_or_customer is tiring. it would be good for us to rename this to some catch all like character (customer is a non-playable character while player is a playable character).
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.
replaced with character
| }, | ||
|
|
||
| "customer": { | ||
| "robot": { |
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.
may as well remove the "robot" key for the time being until we have different assets to make this hardcoding more evident.
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.
changed it to girl so that the choose_character_image_name() function can take in a name
| from domain.domain_builder import build_domain | ||
| from utils.robotouille_utils import trim_item_ID | ||
| from backend.gamemodes.classic import Classic | ||
| from .env_utils import build_identity_predicates, build_location_predicates, build_stacking_predicates, build_goal |
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.
thank you for organizing this better. we'll eventually want to reintegrate PDDL and since these are used by builder.py it's good to keep them in one place and switch between creating PDDL predicates or our State predicates.
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.
noted!
robotouille/env.py
Outdated
| animate (bool): Whether or not to animate the movement of the players. | ||
| Returns: | ||
| gamemode(GameMode): The gamemmode of the environment |
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.
space between gamemode (GameMode) and misspelled gamemode.
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.
added space and fixed typo
chalo2000
left a comment
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.
Leaving additional comment after testing the customer environments.
| "goal": [ | ||
| { | ||
| "predicate": "iscookable", | ||
| "args": ["lettuce"], |
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 also just played through these and realized the goal is unrealizable. What is the plan for checking that these environments are done?
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.
My bad, did this to make it easier to debug. Edited the goals accordingly to close when all customers have finished eating.
chalo2000
left a comment
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.
Awesome! Customers seems to be working and everything has been refactored into GameMode classes. Let's make an issue for game optimizations since the multi-customer seems to be slow and we additionally make one for environments with large goals (could both be tackled in the same PR). other optimizations we need are fast environment copying to automate the tests w/ BFS/DFS but this could be left for another PR.
backend/customer.py
Outdated
| # If the customer needs to enter the game, perform the customer_enter action | ||
| # and add the customer to the queue | ||
| elif not self.has_left_queue and self not in gamemode.customer_queue \ | ||
| and time.get_ticks() >= self.enter_time: |
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.
a bit concerned with this comparison time.get_ticks() >= self.enter_time - time.get_ticks() may return a value that is comparable to self.enter_time but using @Henrygao19's menu flow these times would likely not be comparable, including if the game is reset, if the clock is not instantiated at the beginning of the game. this clock dependency could easily cause bugs in the future if the clock is instantiated differently so would be good to store a countdown that will always be relative to self.enter_time
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.
replaced time and clock arguments in the step functions for movement and customers with dt, and keep track of time_passed in customers
backend/gamemodes/classic.py
Outdated
| Args: | ||
| clock (pygame.time): The time object. | ||
| Modifies: |
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.
Side Effects:
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.
fixed
| new_state (State): The successor state. | ||
| done (bool): True if the goal is reached, False otherwise. | ||
| """ | ||
|
|
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.
couple together the related bits of logic and have comments to describe the high level flow
# Step Customers
...
# Step State
...
# Step Movement
...
# Check win condition
...
also why does the Movement update come after the State update? it feels like the State stepping should be the last thing done before checking win condition. does it make a difference at all?
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.
added comments, replaced order of step functions
| Station.build_stations(environment_json, gamemode) | ||
|
|
||
| # (Dict[str, MetaData]) The player movement data, with the key being the player name | ||
| self.metadata = {} |
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 imagine this was done to fix the networking issues with class / instance variables.
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.
Yep, that's correct
backend/movement/movement.py
Outdated
| possible_destinations = self._get_possible_destinations(player_obj, destination_pos) | ||
| state = gamemode.get_state() | ||
|
|
||
| if gamemode.players.get(character.name) is not None: |
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.
character_obj = gamemode.players.get(character.name, gamemode.customers[character.name])
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.
fixed
backend/movement/movement.py
Outdated
| return | ||
| # Get the path to the destination | ||
| path = self._get_player_path(player_obj, possible_destinations) | ||
| path = self._get_path(obj, possible_destinations, gamemode) |
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.
get_character_path. would be helpful to still have a descriptor like before (previously _get_player_path)
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.
fixed
domain/robotouille.json
Outdated
| } | ||
| }, | ||
| { | ||
| "name": "isgirl", |
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.
our customer asset is female but I would prefer to have a gender neutral predicate (iscustomer) since there are no plans for gender-specific interactions. easier to code 😎
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.
replaced girl with customer
renderer/canvas.py
Outdated
| self._draw_item(surface, obs) | ||
| self._draw_container(surface, obs) | ||
| self._draw_player(surface, gamemode) | ||
| self.draw_customer(surface, gamemode) |
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.
underscore in front for private functions.
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.
fixed
robotouille/env.py
Outdated
| gamemode = Classic(state, domain_json, environment_json, recipe_json) | ||
| gamemode.movement = Movement(layout, animate, environment_json, gamemode) | ||
| return gamemode | ||
| return None |
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.
Instead of returning None, add an assert that the name is inside a list containing just "classic" for now. the game should crash if the gamemode isn't set properly.
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.
fixed
Overview
In this PR, I implement Gamemodes and Customers into Robotouille.
Previously, stepping in the environment was controlled using the
step()function within theStateclass. However, this was problematic because it was controlling more than just updates to the state, especially with new modules like Movement. Now, there is a new module calledGameModethat controls all modules within Robotouille.The
GameModeclass also lets Robotouille have win conditions. In this PR, I implement a basic gamemode called `Classic' where the player must serve a number of customers within a certain amount of time.Changes Made
Customers
Customerclass which keeps track about information about the customers and steps them in the environmentdomain.pyanddomain_builder.pyto now keep track ofnpc_actions, all actions that the npc can do as defined in the domainmovement.pyto also move customersstate.pyto keep track of customers and customer_actionsrecipes.jsonwhere recipes can be specifiedrobotouille.jsonto include Customer type, predicates, and actionsbuilder.pyandobject_enmus.pyto include Customer typecustomer.jsonandmulti_customer.jsonto test customersrenderer.py,canvas.py,robotouille_env.pyandrobotouille_config.jsonto render customersGameMode
GameModeclass that serves as the overall control of the game, keeps track of win conditions, and steps all the modules in the gameClassicsubclass where a player must satisfy all recipes before a given timestate.pytoclassic.pyenv_utils.pyto prevent circular imports when creating the GameMode and Customersrobotouille_simulator.pyto passpygame.timeinto the gamemode classChanges to Containers
robotouille.jsonso that containers can now be picked up and placed with items on itinput.jsoncanvas.pyto properly render containers and items stacked on containersChanges to Robotouille Domain
Test Coverage
Tested with all environments
Next Steps
Screenshots