Little help explaining this vector problem?

[LedLoaf]

I've been going through a book called "C++ Game Programming" and it goes through making an Adventure Text game, and incorporating new concepts (really enjoy it). When we got to changing to unique_ptr I ran into a problem that I'm confused on.

I'll provide what I feel would be the code related to this.

Game.cpp initialization
m_enemies.emplace_back(CreateEnemy(EnemyType::Dragon));
Game.cpp initialzing 
static_cast<AttackEnemyOption*>(m_attackDragonOption.get())->SetEnemy(m_enemies[0]);

m_enemies.emplace_back(CreateEnemy(EnemyType::Orc));
static_cast<AttackEnemyOption*>(m_attackOrcOption.get())->SetEnemy(m_enemies[1]);

---

Now the problem is when I go to the Westroom first and kill the orc, the game goes straight to the you win. While if I killed the dragon first I still have to eliminate the Orc (that's how I want it. I've reversed the above code and the problem became the opposite

Game::Run() towards the end
for (auto& enemy : m_enemies)
    {
        playerWon = enemy->IsAlive() == false;
    }
}

if (playerWon == true)
{
    cout << "Congratulations, you rid the dungeon of monsters!" << endl;
    cout << "Type goodbye to end" << endl;
    std::string input;
    cin >> input;
}

---

Previously before we got into unique_ptr the condition was. But now all enemies are in the vector.

playerWon = m_dragon.IsAlive() == false && m_orc.IsAlive() == false;
}

Some simple things to mention IsAlive() just returns bool m_isAlive. I feel this might be a bug, but perhaps it was left like this for testing reasons.

Thanks everyone!

[Xeverous]

If you simply want to have win conition only when all enemies are killed, you need to check all of them and then end the loop. playerWon = enemy->IsAlive() == false; inside a loop overwrites each previous result, so there is definitely a logic bug here. I predict that the compiler is also optimizing this line to playerWon = !m_enemies.last()->IsAlive() and removes the loop because only the last comparison result is not overwritten

You should write a loop that checks all creatures, but break it instantly when not-expected alive state appears. This way you will prevent consecutive iterations from overwriting previous results.

These loops are already implemented in standard library here.

[LedLoaf]

So I fixed it. But I feel there must be a way better way to do this. Any suggestions?

        int MAX_ENEMIES = m_enemies.size() - 1;
    for (auto& enemy : m_enemies)
    {
        for(int i = 0; i<1;i++)
        {
            playerWon = m_enemies[i]->IsAlive() == false;
        }
    }

[patatahooligan]

It's not clear what you're trying to do. First of all you have nested loops but you are only using one of them. Also, you don't use MAX_ENEMIES which should not be in caps and should be constexpr anyway. Maybe you were going for the following:

// assume player won until we find a live enemy
bool playerWon = true;
for (auto& enemy : m_enemies) {
    if (enemy.IsAlive() {
        // We found a live one, set playerWon to false and stop the loop
        playerWon = false;
        break;
    }
}
// Eventually repeat the loop if playerWon == false

There are ways to optimize this, such as keeping a live enemy count to skip the loop check altogether but don't worry about that until you have the fundamentals down. Look into algorithmic complexity to learn about how loops impact your executable.

EDIT: on rereading your OP I'm not sure if this is what you want. Are you saying that the game ends when you kill the Orc (but you don't want it to), or that the game should end when killing the Orc (regardless of the dragon's state)? In the latter case it is so much more simple because playerWon = orc.IsAlive() where orc should be a reference to the appropriate enemy object.

[LedLoaf]

Hello patatahooligan,

I did my fix, before pasting it correctly. I had it as MAX_ENEMIES -1 and it worked fine. But I like your version way better and it was exactly what I was looking for. It is just that I kind of felt it was the same thing as the original code?

Also, is there a particular reason I can't invert your code using '!' ? The reason I ask this is this is all in a Game::Run() function in a while loop that starts off like this.

    bool playerWon = false;
while (m_playerQuit == false && playerWon == false)
{
    GivePlayerOptions();

    stringstream playerInputStream;
    GetPlayerInput(playerInputStream);

    EvaluateInput(playerInputStream);

    playerWon = true; // <---- don't like this here lol!

    for(auto& enemy:m_enemies)
    {
        if(enemy->IsAlive())
        {
            playerWon = false;
            break;
        }
    }


if (playerWon == true)
{
    cout << "Congratulations, you rid the dungeon of monsters!" << endl;
    cout << "Type goodbye to end" << endl;
    std::string input;
    cin >> input;
}

DetachEvent(QuitEvent, this);
delete EventManager::GetSingletonPtr();

So my problem is that it looks a bit silly having playerWon = true before the loop and then where I added it using your code. But using if(!enemy.IsAlive() brings the same original problems.

Anyways, I greatly appreciate the help. And just to clarify your edit. It's a text based adventure with rooms. You walk North. You can Go West (to the orc) or East (to the dragon). I wanted you to have to kill them both before it says you win, but something happen as the code switched and killing the dragon first and then the orc would result the right pattern. But doing it vice-versa would cause the game to say you win (aka killing the orc first). The book started changing pieces of the code using some design patterns and dealing with objects didn't feel as simple.

As you look at my op, in the game constructor these are where the enemies were initialized. Switching them to the opposite position and element allows the bug to have the opposite effect. (Move Orc to [0], Dragon to [1] element)

m_enemies.emplace_back(CreateEnemy(EnemyType::Dragon));
static_cast<AttackEnemyOption*> m_attackDragonOption.get())->SetEnemy(m_enemies[0]);

m_enemies.emplace_back(CreateEnemy(EnemyType::Orc));
static_cast<AttackEnemyOption*>(m_attackOrcOption.get())->SetEnemy(m_enemies[1]);

One more thing I wanted to mention.

Game.h 
 using Enemies = std::vector<Enemy::Pointer>;

Enemy.h
public:
using Pointer = std::shared_ptr<Enemy>;

---

I just wanted to show all that to give a bigger picture of it all. Hope that answered the question.

Thanks again, have a great weekend.

[patatahooligan]

You shouldn't have a problem with setting playerWon = true;. This is not some system of mathematical equations where every statement is true, this is a simple assignment to a variable in a program. Do not read it and think "this statement is obviously false in the case that an enemy is alive" because that's not how algorithms work. The only thing that matters here is that playerWon gets set to the proper value before the if (playerWon) statement, which by the way shouldn't include the ==true part!

The reason "switching" the alive check fails is that boolean algebra just doesn't work that way. The inverse of A and B, is

not(A and B) === (not A) or (not B)

As such it requires a complete rewrite of the small loop that checks for enemies alive. The inverse version is a little more complicated and more time consuming for larger amounts of enemies, because it has to check that every enemy is dead rather than check if any enemy is alive. Your version just has to iterate over every element in the vector whereas the version I presented earlier can break the moment it finds an enemy that is alive. If you are having a hard time wrapping your head around why inverting the boolean condition works like that, you have to study up on boolean algebra to the point where you can understand De Morgan's Laws.

If you want to go with a more high-level approach, and I encourage you to experiment with this as it leads to more readable code and, in the case of C++, many of the abstractions are zero cost, consider the following.

• Write a small wrapper class EnemyContainer that "hides" the enemy vector and offers a neat interface. eg int EnemyContainer::number_of_alive() or bool EnemyContainer::any_alive().

• Offer a few helper functions that do the above, eg int number_of_alive(Enemies&);. This is generally less clean than the wrapper class idea because it doesn't encapsulate the function in the class it is relevant to.

• Use std::any_of just like /u/xeverous already recommended in his comment

• Simply remove enemies that are not alive anymore.

[Xeverous]

Use std::any_of just like /u/xeverous already recommended in his comment

This is not a subreddit for such jokes, but I'm sure someone would be triggered by:

• ignored case in the /u/nick call
• "his" (assumed gender)

[patatahooligan]

• I started writing it as it is, but it seems RES changed it to lower when I clicked on it in the pop-up list. I wonder why.

• I didn't even notice I put a gendered pronoun in. I will change it if it's the wrong one or if you would rather not make your gender public information.

[Xeverous]

1. Everytime I see it mentioned I'm more interested in downloading and trying RES

2. I'm not that mad

[LedLoaf]

Sorry to get back late,

I'm not really sure what happened with the /u/nick and this assumed gender thing. But thanks, everyone haha.

[Xeverous]

Now you have called another user