-
Notifications
You must be signed in to change notification settings - Fork 3
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
incorporate objects defined in yaml configuration into gridworld #248
Conversation
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 a good start on the food
to object
conversion, but there's still a lot of work to be done to make this all work the way it should where the object behavior (especially spawning) is determined by object properties instead of global experiment config.
dlgr/griduniverse/experiment.py
Outdated
'food_planting': bool, | ||
'food_planting_cost': int, | ||
'food_probability_distribution': unicode, | ||
'object_count': int, |
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.
@cguardia I don't think any of these should be in the experiment config, they should be object properties in the YAML and don't need to be registered here.
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'm going to propose a generic plantable
property unless someone can think of a better word. Seems generic enough (eg: you can "plant evidence", 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.
Sure, for now we'll only be using it for food, so it doesn't much matter I think.
dlgr/griduniverse/experiment.py
Outdated
self.food_maturation_threshold = kwargs.get( | ||
'food_maturation_threshold', 0.0) | ||
# Objects | ||
self.object_count = kwargs.get('object_count', 8) |
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.
These shouldn't be coming in via __init__
but being read from self.object_config
since they are per object configuration. Generally speaking, I think we don't need to set them here since they will differ per object and need to be looked up per object when doing spawning, etc.
@@ -670,23 +679,23 @@ def instructions(self): | |||
text += """ Movement commands will be ignored almost all of the 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.
This is eventually going to have to be re-done. See #250.
dlgr/griduniverse/experiment.py
Outdated
food = self.food_locations[position] | ||
if position in self.object_locations: | ||
food = self.object_locations[position] | ||
# any object with calories is food for now. Other objects are ignored. |
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 don't think this is quite right. "Food" is any non-interactive object with calories, so:
item_info = self.object_config.get(obj_id)
is_food = item_info.get('calories', 0) and not item_info.get('interactive', True)
Maybe this can be a dynamic property of Object
.
dlgr/griduniverse/experiment.py
Outdated
else: | ||
self.num_food -= 1 | ||
self.object_count -= 1 |
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 needs to be tracked per object type, I think.
dlgr/griduniverse/experiment.py
Outdated
self.probability_function_args = [] | ||
parts = self.food_probability_distribution.split() | ||
parts = self.probability_distribution.split() |
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 should also be object specific.
dlgr/griduniverse/experiment.py
Outdated
player.score -= self.grid.food_planting_cost | ||
self.grid.spawn_food(position=position) | ||
self.grid.spawn_object(position=position) |
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 think this needs an object_id
, specifically the object_id for "Food". Maybe that's a new experiment config parameter, maybe a required plantable_object_id
when food_planting
is enabled. Choosing an object type at random doesn't really work for this.
dlgr/griduniverse/experiment.py
Outdated
if self.grid.food_maturation_threshold and (count % 100) == 0: | ||
self.grid.food_updated = True | ||
# Log object updates every hundred rounds to capture maturity changes | ||
if self.grid.maturation_threshold and (count % 100) == 0: |
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 think self.grid.maturation_threshold
needs to be computed at launch using something like any([o.get(
maturation_threshold, 0) for o in self.object_config])
. That way this step will happen if any object type has a maturation_threshold
set.
0437d7d
to
0bb507d
Compare
0bb507d
to
02cbb59
Compare
@@ -1,82 +1,120 @@ | |||
--- | |||
item_defaults: |
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 like the idea of being able to configure the defaults across all 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.
Spoke too soon with my last review. There's still a couple small things that look like they need fixes. It is looking much better!
dlgr/griduniverse/experiment.py
Outdated
# Items | ||
self.respawn = kwargs.get("respawn", True) | ||
self.public_good_multiplier = kwargs.get("public_good_multiplier", 1) | ||
self.growth_rate = kwargs.get("growth_rate", 1.00) |
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 should definitely be item level, I think.
@property | ||
def limited_player_colors(self): | ||
return self.player_colors[:self.num_colors] | ||
return self.player_colors[: self.num_colors] |
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 black
doing here?
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.
Also, maybe we should put these colors in game_config.yml
?
logger.info("Spawning items") | ||
for item_type in self.item_config.values(): | ||
for i in range(item_type["item_count"]): | ||
if (i % 250) == 0: |
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 wonder if we need to adjust this sleep check to use an overall loop counter to adjust for the fact that any given item type is likely to have a lower count. Probably doesn't matter for now, but doing the check per item type seems odd to me.
dlgr/griduniverse/experiment.py
Outdated
def spawn_player(self, id=None, **kwargs): | ||
"""Spawn a player.""" | ||
# For player position, use same distribution as first configured item. | ||
item_id = list(self.item_config.keys())[0] |
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.
Why are we doing this instead of leaving it to the default random distribution? Very unintuitive to me. We can add configuration for player distribution later, right?
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.
Yeah, I think we should add a config for that.
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.
All my initial comments have been addressed. I've added some new comments, but they are minor optimization or clarification suggestions. I think this PR is in good shape. I may test a little and implement my suggestions before merge.
@@ -1231,6 +1241,8 @@ def configure(self): | |||
(t["actor_start"], t["target_start"]): t | |||
for t in self.game_config.get("transitions", ()) | |||
} | |||
|
|||
self.player_config = self.game_config.get("player_config") |
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 think that at least for now, we'll need to JSON serialize self.player_config
into self.player_config_json
so the front-end can read it, but I'm not certain this is the the case (these may not actually be actually be used).
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 already pass the player color stuff directly, and the probability distribution is definitely server only. I think we're OK here for now, but if we eventually have client side player config in this structure, we'll probably need it. For now I don't think we do.
"""Select an empty cell at random, using the configured probability | ||
distribution.""" | ||
rows = self.rows | ||
columns = self.columns | ||
empty_cell = False | ||
if item_id: | ||
prob_func = self.item_config[item_id]["probability_function"] |
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.
Doesn't this need to be getattr(distributions, self.item_config[item_id]["probability_function"])
(the config value is just a string)?
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.
Oh, I see... you've already created a map to the actual functions earlier. 👍🏼
Not sure how I closed this. |
Description
Use configurable object types instead of only food to populate the grid. For now all objects with a positive calories attribute behave as food. Other objects should show up, but do not respond to user movements.
Covers #232 and #233