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

Salmon and Turkey Task #44

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Salmon and Turkey Task #44

wants to merge 9 commits into from

Conversation

tragicoverture
Copy link

Overview

In this pr, I integrated grilling salmon and roasting turkey into Robotouille by implementing new items, new types, new actions and assets into the game.

Changes Made

Changes For implementing new features into Robotouille

  • Added new assets
  • Edited asset config json to update new and current assets
  • Edited input json to include new actions
  • Edited robotouille config json to include new predicates and actions
  • Updated builder.py, object_enums.py with new objects

Changes for new objects

  • Updated builder.py, object_enums.py with updated stove asset
  • Updated canvas.py to be able to render updated stove asset

Test Coverage

Manual testing using new test files

Related PRs or Issues

Condiment changes from Hot Dog task will be reflected in this PR soon.

Screenshots

roastturkeys.mov
roast turkeys!
Screen.Recording.2024-11-09.at.01.22.34.mov
grill salmon!

… and actions into robotouille json, added corresponding gameplay instructions, added new objects into appropriate enums class, cannot draw condiment and condiment logic work in progress
…dded condiment problem jsons, need to add proper assets that correspond to hot dog with added condiments
…predicates and actions into robotouille json, updated input json to include new actions, added new objects into objects_enums.py
…be under with stove asset in config json, created new actions to put in oven and take out items from oven, updated canvas.py to account for multiple assets aside from default asset for stations
Copy link
Contributor

@lsuyean lsuyean left a comment

Choose a reason for hiding this comment

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

Good job on this PR! I tested out all the environments and they look great. I just have a few comments about the predicates and the rendering, particularly regarding the condiments on the hot dogs. Do let me know if you have any questions or my comments are unclear

{
"name": "has_ketchup_mustard",
"param_types": ["item"]
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to not include the has_ketchup_mastard predicate? I believe the has_ketchup and has_mustard predicates are sufficient, and you can use those for rendering the item with both condiments

{
"name": "item_on",
"param_types": ["item", "station"]
},
{
"name": "item_in",
"param_types": ["item", "station"]
Copy link
Contributor

Choose a reason for hiding this comment

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

can you change the name of this predicate to in_station? we currently have another predicate called in for items in containers, so item_in is vague. Would be better to change item_in to in_station, and in to in_container

{
"predicate": "item_on",
"params": ["i1", "s1"],
"is_true": false
Copy link
Contributor

Choose a reason for hiding this comment

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

The item_on predicate should already be false before the action is taken, so this effect is unnecessary.

"sfx": [
{
"type": "conditional",
"param": "i1",
Copy link
Contributor

Choose a reason for hiding this comment

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

If it's possible to render both the ketchup and mustard without the has_ketchup_mustard predicate, this sfx can be removed.

"sfx": [
{
"type": "conditional",
"param": "i1",
Copy link
Contributor

Choose a reason for hiding this comment

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

If it's possible to render both the ketchup and mustard without the has_ketchup_mustard predicate, this sfx can be removed.

"is_true": true
},
{
"predicate": "can_add_salt",
Copy link
Contributor

Choose a reason for hiding this comment

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

you use a can_add_salt predicate here, but for ketchup and mustard you check it an item is_grilled. Is it possible to use can_add_ketchup and can_add_mustard instead? so that in the future we can add ketchup and mustard to other items that may not necessarily be grilled, like fries which are fried instead of grilled

@@ -132,7 +132,10 @@ def _choose_item_asset(self, item_image_name, obs):
chosen_asset = asset_config[asset]["asset"]
elif matches == max_matches:
chosen_asset = asset_config["default"]

# if asset == "grilled":
Copy link
Contributor

Choose a reason for hiding this comment

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

remove comments if unnecessary

"hotdog": {
"assets": {
"default": "hotdog.png",
"grilled": {
Copy link
Contributor

@lsuyean lsuyean Dec 3, 2024

Choose a reason for hiding this comment

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

To make it so that you do not need the commented portion in canvas.py, make all of these separate dictionaries. so one for grilled, one for grilled_with_ketchup, grilled_with_mustard, grilled_with_ketchup_mustard, and add multiple predicates to the list. So for example, a grilled hotdog with mustard and ketchup would be ["isgrilled", "has_mustard", "has_ketchup"]. Something similar to your salt on salmon rendering

"ketchupbottle": {
"assets": {
"default": "ketchupbottle.png",
"predicates": ["isketchup"]
Copy link
Contributor

Choose a reason for hiding this comment

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

The default images shouldn't need any predicates, can refer to items like bread or topbun with only one image to see how it should be done

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.

2 participants