Team 8 initial world

[Scor23]

Includes a new world, which has tree tiles, water tiles, and grass tiles. The player can walk across any grass tiles and are not able to cross tree tiles or water tiles currently (pending future item implementation). There is also a strength test between agents, which checks whether agents are adjacent and if they are, checks which agent has a higher strength.

[mercere99]

The world objects shouldn't be printing anything -- they should allow the interface to do this. When we are using a graphical, network, or web interface, couts won't be visible. This DOES point out that we need some mechanism for messages to be sent to a user.

[mercere99]

To me, a function called "StrengthCheck" should be just comparing the strengths and indicating who would win, not actually killing off the loser. Perhaps divide this up so that StrengthCheck behaves that way, but something like "DoCombat" is used to actually kill off one individual.

[mercere99]

You can use use structured bindings in the loop.

for (auto & [name, uses] : inventory_map)

Then you won't need to worry about pair.first or second, and the names are cleaner.

[mercere99]

When you pass by reference, you're effectively passing a pointer into the function instead of the full value. For larger objects (like many strings!) this can be a helpful timesaver. If smaller values like size_t that you aren't changing, it's best to just pass by value.

[mercere99]

I would call this AddItem instead of AddEntity because it's going into item_set. The only reason that I didn't create an Item class is because there was nothing to add beyond what was already in Entity, but maybe I should have for better code organization (and future updates to items?)

[mercere99]

What is the goal of having an Interface just return without being removed or taking any other action? One option might be to mark the run as over, but that would cause problems for multi player games. Probably the best option is to remove the interface like you would any other agent and trust that the interface's destructor will take care of everything. A better option might be to have a function that is run to notify an agent or impending removal and then marking them as inactive. This is a topic we should talk about as a class.

[mercere99]

You should avoid C-style cases. Also, why is pos an int? It should be size_t, which wouldn't involve casting. Likewise newType (which should be shifted to new_type, but we only talked about that after your wrote this code) should be a size_t, since that's the type stored by WorldGrid.

[mercere99]

Also, we shouldn't put the conversion of x and y to a position in more than one spot. There is a helper function ToIndex that you can use for this (so we only need to update it in one spot if something changes), or you can use one of the existing functions like At(). Since I don't think we'll keep this function, it doesn't matter as much here, but you should always link in to existing functionality when possible rather than duplicating effort.

[mercere99]

I don't think we need SetCell as a separate function. The brackets operator (or the At() function) will take care of this.

Instead of (for example): grid.SetCell(pos, in_type);
You can already write: grid[pos] = in_type;

I think the current method is clear and concise, so I'd hesitate to add this new member function.

[mercere99]

This is an interesting take on putting a world together and has a lot of potential, but I think your group needs to be working more closely with groups 3 and 5 to handle the user interface aspects.

Currently when agents engage in combat, you are taking control over the interface and using cin and cout to provide feedback about the combat and identify what the player wants to do next. This technique might work well with the TrashInterface (since that's just text itself), but won't work generically with the graphical interface or the network interface (or any others that we might also pull together, like a web interface.) Furthermore, it assumes that only a human player will ever engage in combat, but ideally we an AI should have that ability as well.

I think the easiest way to deal with this issue is to have the combat actions be provided to the agent/player in the same way as the other actions. The question then becomes, do you provide ALL of the actions up front, or do you swap out an agent's available actions based on whether they are in combat or not.

Inline, I mentioned that the StrengthCheck function can be refactored, perhaps into a "DoCombat" that manages combat (like StrengthCheck does now), but then StrengthCheck itself would simply tell you who is stronger. At the moment that involves comparing health, but putting it in its own function allows you to make it more complex later if you want to AND lets it be checked in other contexts (perhaps when agents are deciding how to proceed).

Looking through the rest of that function, it is very long with many components. It should certainly be broken up into a number of smaller functions that are clearly named. In the least, each combat action should call a function, but you can certainly refactor further than that (the loss condition can, for example, have it's own function, as should loot generation etc).

The CheckAround function should also check the current square, assuming two agents are allowed to be in the same position (I don't see anywhere stopping that from happening). At the moment agents can have a square between them and both move into that square in the same turn. So they go from being non-adjacent to co-located. The other alternative is, of course, to ensure that no other agents are on a cell before you let an agent enter it. Then only the agent who goes first will get there.

You have overridden "Run()" to also call HandleNeighbors(), BUT I think a cleaner solution would be to override UpdateWorld(). By default UpdateWorld is empty, and is intended to be overridden for this exact purpose. That said, I'm looking at proposing a change to the API where we can create a whole list of functionality to run every update.

DoAction should also be broken up into smaller functions; it has many of the same issues as StrengthCheck. You don't want to take over the interface, and you DO want to allow non-player agents to have inventory, etc.

To give you a better idea, below is a quick pass at how I might refactor this function. This suggestion would require you to pass a reference to new_position around a lot (or shift new_position to be a property), but would look something like:

int DoAction(cse491::AgentBase & agent, size_t action_id) override {
  // Update Direction property and get new position.
  cse491::GridPosition new_position = DoAction_FindNewPosition(agent, action_id);

  // If agent walks over an item, deal with picking it up
  DoAction_PickUpItems(agent, new_position);

  // Test if step would take agent to terrain that it would interact with (tree, water, etc.)
  DoAction_TestNewPosition(agent, new_position);

  // Update final position, etc. and return success.
  return DoAction_Finalize(agent, new_position);
}

Each of the four DoAction_* helper functions might also be broken down into smaller functions, but this way it is really easy to see the big picture of what is going on.

Next: In order to implement inventory, you modified the AgentBase. In principle I don't have a problem with this, but in practice you should coordinate with the other two world groups about how you all want inventory done. If you are all going to have different systems, it should be done in properties. If you are all happy using the same system, this option looks like a solid candidate.

Good job with adding tests.

I should also say, I really like how items change how an agent can interact with terrain while interacting with enemies. This will allow not only interesting adventure-type scenarios, but even some simple puzzles where you need to collect items to be able to get to the exit while avoiding monsters that can kill you.

Overall, I think there is a lot of good work here and I'm excited to see how it all comes together.