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

Maze Generation and Partially Working Tiles #23

Merged
merged 25 commits into from
Oct 19, 2023

Conversation

meehank3
Copy link
Contributor

No description provided.

.idea/
*.iml

../assets/grids/
Copy link
Contributor

Choose a reason for hiding this comment

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

This line seems to be referring to an assets/grids/ folder outside of the project. Either way I think we should not ignore a grids/ directory.

@@ -51,6 +78,49 @@ namespace cse491 {
if (!main_grid.IsValid(new_position)) { return false; }
if (main_grid.At(new_position) == wall_id) { return false; }

// Check if the agent is trying to move onto a spike.
if (main_grid.At(new_position) == spike_id) {
Copy link
Contributor

@mercere99 mercere99 Oct 12, 2023

Choose a reason for hiding this comment

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

Make sure to indent the body of this if-statement (assuming you leave it and don't just use the advice above.

}
~MazeWorld() = default;

/// Allow the agents to move around the maze.
int DoAction(AgentBase & agent, size_t action_id) override {
// Determine where the agent is trying to move.
GridPosition currentPosition = agent.GetPosition();
Copy link
Contributor

Choose a reason for hiding this comment

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

For consistency of style, I would call this variable cur_position or something along those lines.

main_grid.Read("../assets/grids/default_maze.grid", type_options);

randomGenerator.seed(seed);
}
~MazeWorld() = default;

/// Allow the agents to move around the maze.
int DoAction(AgentBase & agent, size_t action_id) override {
Copy link
Contributor

@mercere99 mercere99 Oct 12, 2023

Choose a reason for hiding this comment

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

This DoAction function is getting very long. For cleaner code division, I recommend splitting it apart into various helper functions. You can also save the value of main_grid.At(new_position) rather than call it so many times.

For example, starting with line 79 below (the first test of main_grid.At(new_position)) you could instead have:

const size_t cur_id = main_grid.At(cur_position);
const size_t new_id = main_grid.At(new_position);
if (new_id == wall_id) return false;  // Moving into a wall is not allowed.
else if (new_id == spike_id) { return MoveToSpike(agent); } // Spikes are lethal.
else if (new_id == tar_id && cur_id == tar_id) { return MoveInTar(agent); }

if (cur_id == tar_id) { MoveFromTar(agent); }
if (new_id == tar_id) { MoveToTar(agent); }

You would then put together the set of private member functions MoveToSpike, MoveInTar, MoveFromTar, and MoveToTar, but adding on new tile types will then be much cleaner and easier. I do recommend adding short comments above for what happens when you move through, from, or to tar to make it clear from the code.

Copy link
Contributor

@mercere99 mercere99 left a comment

Choose a reason for hiding this comment

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

The .idea/ and .vscode/ folders should probably not be added to the main repository -- I recommend removing these from your pull request (unless I'm missing some reason to include them...)

The files source.zip, source/simple, and source/simple.exe should definitely not be part of the pull request. In general, executables should never be pushed into a repository and zip files rarely should be.

The file source/.gitignore is redundant; we should only need the .gitignore in the root directory.

Good start with the BiomeGenerator! Some comments to help with that:

  • Please change BiomeGenerator.h to BiomeGenerator.hpp
  • Make sure that the header file also puts your new classes (and the using statements) in your group's namespace. Types like Point are very likely to collide. Note, that you can also probably use GridPosition in place of Point for consistency, unless there is something else you need from Point.
  • Good use of enum for BiomeType.
  • It seems like the BiomeGenerator should be working with WorldGrid rather than using std::vector<std::vector<char>> for the type. Again, type consistency will allow for easier interactions and give you all of the pre-built features.

You should re-name your MazeWorld extensions to leave the original MazeWorld in place since other groups are using that as well; we probably don't want to change the existing classes. Some comments are inline in the file.

Likewise you should rename your version of source/simple_main.cpp since the original version of this is also used by other projects.

Overall, you have a solid coding type; good use of naming, doxygen-style comments, and general formatting.

kevinmeehan13 and others added 3 commits October 13, 2023 13:12
Guarantees a path from the top-left corner of the map to the right side
@FergusonAJ FergusonAJ changed the base branch from main to development October 19, 2023 20:52
@FergusonAJ FergusonAJ merged commit e972675 into MSU-CSE491:development Oct 19, 2023
amantham20 added a commit that referenced this pull request Nov 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants