-
Notifications
You must be signed in to change notification settings - Fork 71
Conversation
Useful link: https://github.com/ocadotechnology/aimmo/wiki/Levels |
I would squash (or rename?) a few commits there, to get a clean history if possible. Commits like "Fixed small merge problem." are not really helpful for instance... |
players/views.py
Outdated
@@ -143,7 +143,7 @@ def watch_level(request, num): | |||
LOGGER.debug('Displaying game with id %s', game.id) | |||
return _render_game(request, game) | |||
|
|||
|
|||
# This function adds the Levels to the database |
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.
might be better as a documentation below the title? But the title is explicit enough I think?
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 we can just remove the comment.
@@ -8,7 +8,7 @@ | |||
from simulation.map_generator import get_random_edge_index | |||
from simulation.world_map import WorldMap |
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 you need this import anymore :)
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.
The WorldMap is not. The get_random_edge_index function is, as we test it.
7323f72
to
6bdb7ae
Compare
I squashed some commits and removed the prints in tests. Hopefully now is more clean. |
aimmo-game/simulation/game_state.py
Outdated
@@ -1,6 +1,5 @@ | |||
from simulation.avatar import fog_of_war | |||
|
|||
|
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.
two lines please https://www.python.org/dev/peps/pep-0008/#blank-lines
from simulation.pickups import HealthPickup | ||
from simulation.pickups import InvulnerabilityPickup | ||
from simulation.pickups import DamagePickup | ||
|
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.
two lines please https://www.python.org/dev/peps/pep-0008/#blank-lines
if "sprite" in json: | ||
return json["sprite"] | ||
return {} | ||
|
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.
two lines please https://www.python.org/dev/peps/pep-0008/#blank-lines
@abc.abstractmethod | ||
def decode(self, json, world_map): | ||
pass | ||
|
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.
two lines please https://www.python.org/dev/peps/pep-0008/#blank-lines
|
||
def get_sprite(json): | ||
if "sprite" in json: | ||
return json["sprite"] |
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.
please use https://docs.python.org/2/library/stdtypes.html#dict.get to get the value here :)
def get_objects(self): | ||
self.parse_json() | ||
return self.map | ||
|
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.
two lines please https://www.python.org/dev/peps/pep-0008/#blank-lines
@@ -0,0 +1,29 @@ | |||
import hashlib | |||
|
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.
two lines please https://www.python.org/dev/peps/pep-0008/#blank-lines
|
||
def get_game_state(self, avatar_manager): | ||
return GameState(self.get_map(), avatar_manager, self.check_complete) | ||
LOGGER = logging.getLogger(__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.
you get the idea :)
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.
Just to check, "two lines please https://www.python.org/dev/peps/pep-0008/#blank-lines "?
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.
yes :D
players/views.py
Outdated
@@ -143,7 +143,6 @@ def watch_level(request, num): | |||
LOGGER.debug('Displaying game with id %s', game.id) | |||
return _render_game(request, game) | |||
|
|||
|
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?
all_tests.py
Outdated
@@ -15,7 +15,6 @@ | |||
BASE_DIR = os.path.abspath(os.path.dirname(__file__)) | |||
APPS = ('', 'aimmo-game/', 'aimmo-game-worker/', 'aimmo-game-creator/') | |||
|
|||
|
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 did the changes. For some reason GitHub does not show the comments outdated for enters. |
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.
sorry a few things had escaped me at first
|
||
return main_avatar.score > 1000 | ||
|
||
COMPLETION_CHECKS = { |
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't that be templated somehow and linked to the max number of levels defined somewhere?
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.
The idea is that if I remove the completion check, it will just use the default one, that is return False
.
aimmo-game/simulation/custom_map.py
Outdated
return GameState(self.get_map(), avatar_manager, self.check_complete) | ||
|
||
def check_complete(self, game_state): | ||
return 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.
huh?
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.
The default behavior for the check complete is to to never end the level. We override that in levels with the specific checks.
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 abstracting it?
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.
@CelineBoudier @danalex97 I'm not sure about python as it's still quite new to me, but I found the readibility of abstract methods/classes really difficult. Maybe we should use something like
@abc.abstractmethod
?
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, that's how I generally do it. But I wanted to make that a method with a default behavior, as all the classes EmptyMapGenerator, Main and JSONLevelGenerator use check_complete. I think it is better this way.
Why we have the parameter game state is that we might want some custom completion checks that do something like check the score and so on.
class TestAutomaticLevelGenerators(unittest.TestCase): | ||
def test_generate_level_class(self): | ||
def completed_check(self, y): | ||
return 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.
same, why a parameter here if it's only to return 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.
I think the only reason is to fit the interface.
dc762b8
to
4daf763
Compare
…w gets generated.
…e constructor actually a function, but we will move the logic to the level generator.
…asses will be placed in only one file.
…t the class generators at some point.
…ng/removing a level from maps will work.
…prite field is not passed along, as the refactoring code has not been added.
…t both back-end and front-end generated levels.
…re_location fields with a new common class.
…s composed with a cell.
…annot be mutated. Now we need to get rid of _habitable and _generates_score fields.
…. Now we can pass the content from the level_generation through the new made classes.
…ld that is passes along with the models. Added documentation to parsers.
…ut the rows and columns seem the be the other way round so we should look into it.
…le thing work the same but the responsability is seaprated.
…generator from WorldMap.
…ed parsers with the new transform.
…t is yet to be deleted.
… to see how to change the tests in custom_map.
… instead of dictionary lookup.
4daf763
to
94be4e1
Compare
94be4e1
to
6194d8e
Compare
Any updates on merging this? |
This was reviewed at some point, but then I rebased. @CelineBoudier reviewed this besides my last commit, As I rebased, git automatically dismissed the stale review. Now there seem to be some conflicts. @OlafSzmidt I can rebase again or merge and someone can review this afterwards... |
@OlafSzmidt Maybe we should rebase this after you finish the refactoring for MLP? |
I'm gonna close this as we are redoing it to work with 3D rendering |
This is a pull request meant to complete and extend the #220 pull request. The new features are support for levels exported from Unity(see https://github.com/ocadotechnology/aimmo-unity/tree/level_generation for more deails). Now both .txt levels and .json levels are supported.
Another important change is a refactoring for the cells: we can now pass a sprite description metadata formatted as a json. Some parts of the world_map may seem boilerplate, but this is due to the fact that #223 is yet to be merged.