26

In episode 1 of New Game 2, around 9:40, there is a shot of the code that Nene has written:

enter image description here

Here is it in text form with the comments translated:

// the calculation of damage when attacked
void DestructibleActor::ReceiveDamage(float sourceDamage) 
{
    // apply debuffs
    auto resolvedDamage = sourceDamage;
    for (const auto& debuf:m_debufs)
    {
        resolvedDamage = debuf.ApplyToDamage(resolvedDamage);
        m_currentHealth -= resolvedDamage
        if (m_currentHealth <= 0.f)
        {
            m_currentHealth = 0.f;
            DestroyMe();
        }
    }
}

After the shot, Umiko, pointing at the for loop, said that the reason why the code crashed is that there is an infinite loop.

I don't really know C++ so I'm not sure whether what she is saying is true.

From what I can see, the for loop is just iterating through the debufs that the Actor currently has. Unless the Actor has an infinite amount of debufs, I don't think it can possibly become an infinite loop.

But I'm not sure because the only reason that there is a shot of the code is that they wanted to put an easter egg here, right? We would have just gotten a shot of the back of the laptop and heard Umiko saying "Oh you've got an infinite loop there". The fact that they actually showed some code makes me think that somehow the code is an easter egg of some sort.

Will the code actually create an infinite loop?

closed as off-topic by Logan M, Maroon, Torisuda, Darjeeling, Jon Lin Jul 15 '17 at 13:37

  • This question does not appear to be about anime or manga within the scope defined in the help center.
If this question can be reworded to fit the rules in the help center, please edit the question.

  • 4
    Probably helpful: additional screenshot of Umiko saying that "It was calling the same operation over and over again", which might not be shown in the code. – Aki Tanaka Jul 14 '17 at 12:17
  • Oh! I didn't know that! @AkiTanaka the sub that I watched says "infinite loop" – Sweeper Jul 14 '17 at 12:19
  • 8
    I'm voting to close this question as off-topic because it's a question purely about programming which does not require any context or knowledge of the source anime/manga to answer, as per anime.meta.stackexchange.com/a/669. – Logan M Jul 14 '17 at 21:33
  • 2
    @LoganM I don't really agree. It's not just that OP has a question about some source code that happened to come from an anime; OP's question is about a particular statement made about the source code by a character in the anime, and there is an anime-related answer, namely "Crunchyroll done goofed and mistranslated the line". – senshin Jul 16 '17 at 5:00
  • 1
    @senshin I think you're reading what you want the question to be about, rather than what is actually asked. The question provides some source code and asks whether it generates an infinite loop as real-life C++ code. New Game! is a fictional work; there is no need for code presented in it to conform to real-life standards. What Umiko says about the code is more authoritative than any C++ standards or compilers. The top (accepted) answer has no mention of any in-universe information. I think an on-topic question could be asked about this with a good answer, but as phrased this isn't it. – Logan M Jul 16 '17 at 20:12
20

The code is not an infinite loop but it is a bug.

There's two (possibly three) issues:

  • If no debufs are present no damage will be applied at all
  • Excessive damage will be applied if there is more than 1 debuf
  • If DestroyMe() immediately deletes the object and there are still m_debufs to be processed the loop will be executing over a deleted object and trashing memory. Most game engines have a destroy queue to work around this and more so that may not be an issue.

The application of damage should be outside the loop.

Here's the corrected function:

// the calculation of damage when attacked
void DestructibleActor::ReceiveDamage(float sourceDamage) 
{
    // apply debuffs
    auto resolvedDamage = sourceDamage;
    for (const auto& debuf:m_debufs)
    {
        resolvedDamage = debuf.ApplyToDamage(resolvedDamage);
    }
    m_currentHealth -= resolvedDamage
    if (m_currentHealth <= 0.f)
    {
        m_currentHealth = 0.f;
        DestroyMe();
    }
}
  • 15
    Are we on Code Review? :D – Gallifreyan Jul 14 '17 at 12:45
  • 4
    floats are great for health if you don't go above 16777216 HP. You can even set health to infinite to create an enemy you can hit but won't die, and have an one-kill attack using infinite damage that still won't kill an infinite HP character (result of INF-INF is NaN) but will kill everything else. So it's very useful. – Stephane Hockenhull Jul 14 '17 at 13:14
  • 1
    @cat By convention in many coding standards the m_ prefix means it's a member variable. In this case a member variable of DestructibleActor. – Stephane Hockenhull Jul 14 '17 at 18:50
  • 2
    @HotelCalifornia I agree there's a small chance ApplyToDamage does not work as expected but in the example case you give I would say ApplyToDamage also needs to be reworked to require passing it the original sourceDamage as well so that it can calculate the debuf properly in those cases. To be an absolute pedant: at this point the dmg information should be a struct that includes the original dmg, current dmg, and the nature of the damage(s) as well if the debufs have things like "vulnerability to fire". From experience it's not long before any game design with debufs demand these. – Stephane Hockenhull Jul 14 '17 at 21:38
  • 1
    @StephaneHockenhull well said! – HotelCalifornia Jul 14 '17 at 21:38
6

The code does not seem to create an infinite loop.

The only way the loop would be infinite would be if

debuf.ApplyToDamage(resolvedDamage);

or

DestroyMe();

were to add new items to the m_debufs container.

This seems unlikely. And if it were the case, the program could crash because of changing the container while being iterated.

The program would most likely crash because of the call to DestroyMe(); which presumably destroys the current object that is currently running the loop.

We can think of it as the cartoon where the 'bad guy' saws a branch to have the 'good guy' fall with it, but realizes too late that he's on the wrong side of the cut. Or the Midgaard Snake eating it's own tail.


I should also add that the most common symptom of an infinite loop is that it freezes the program or makes it not responsive. It will crash the program if it allocates memory repeatedly, or does something that ends up dividing by zero, or the likes.


Based on the comment by Aki Tanaka ,

Probably helpful: additional screenshot of Umiko saying that "It was calling the same operation over and over again", which might not be shown in the code.

"It was calling the same operation over and over again" This is more likely.

Assuming that DestroyMe(); is not designed to be called more than once, it is more likely to cause a crash.

A way to fix this issue would be to change the if for something like this:

    if (m_currentHealth <= 0.f)
    {
        m_currentHealth = 0.f;
        DestroyMe();
        break;
    }

This would exit the loop when the DestructibleActor is destroyed, making sure that 1) the DestroyMe method is called only once and 2) don't apply buffs uselessly once the object is already considered dead.

  • 1
    Breaking out of the for loop when health <= 0 is definitely a better fix than waiting until after the loop to check health. – trlkly Jul 14 '17 at 20:13
  • I think I'd probably break out of the loop, and then call DestroyMe(), just to be safe – HotelCalifornia Jul 14 '17 at 20:36
1

There are several issues with the code:

  1. If there are no debufs, no damage would be taken.
  2. DestroyMe() function name sounds dangerous. Depending on how its implemented, it might or might not be an issue. If it's just a call to the destructor of the current object wrapped in a function, then there is an issue, as the object would be destroyed in the middle of it executing code. If it's a call to a function which queues the deletion event of the current object, then there is no issue, as the object would be destroyed after it completes its execution and the event loop kicks in.
  3. The actual issue that seems to be mentioned in the anime, the "It was calling the same operation over and over again" -- it will call DestroyMe() as long as m_currentHealth <= 0.f and there are more debuffs left to iterate, which might result in DestroyMe() being called multiple times, over and over again. The loop should stop after the first DestroyMe() call, because deleting an object more than once results in memory corruption, which will likely result in a crash in the long run.

I'm not really sure why every debuf takes away the health, instead of the health being taken away only once, with the effects of all debuffs being applied on the initial damage taken, but I will assume that's the correct game logic.

The correct code would be

// the calculation of damage when attacked
void DestructibleActor::ReceiveDamage(float sourceDamage) 
{
    // apply debuffs
    auto resolvedDamage = sourceDamage;
    for (const auto& debuf:m_debufs)
    {
        resolvedDamage = debuf.ApplyToDamage(resolvedDamage);
        m_currentHealth -= resolvedDamage
        if (m_currentHealth <= 0.f)
        {
            m_currentHealth = 0.f;
            DestroyMe();
            break;
        }
    }
}
  • I should point out that as I have written memory allocators in the past, deleting the same memory doesn't have to be an issue. It could also be redundant. It all depends on the allocator's behavior. Mine just acted like a low level linked list so the "node" for the deleted data either gets set as free several times or redeleted several times (which would just correspond to redundant pointer redirections). Good catch though. – The Great Duck Jul 15 '17 at 0:17
  • Double-free is a bug, and generally leads to undefined behavior and crashes. Even if you have a custom allocator that somehow disallows reuse of the same memory address, double-free is a smelly code as it makes no sense and you will get yelled at by static code analyzers. – BananaBlender Jul 16 '17 at 7:26
  • Of course! I didn't design it for that purpose. Some languages just require an allocator due to lacks of features. No no no. I was merely stating that a crash isn't guaranteed. Certain design classifications don't always crash. – The Great Duck Jul 17 '17 at 0:41

Not the answer you're looking for? Browse other questions tagged or ask your own question.