In Code Review, a bunch of fansubbers from different technical backgrounds will discuss the code we see in anime. Have a scene you want us to review? Let us know!
Please note that these articles contain spoilers!
New Game is a cute slice of life comedy about a group of Japanese game developers. It follows Aoba, a recent new grad that starts work at a company she's been idolizing for years. The company produces one of her favorite games, and their character models inspired her to pursue game development as a career. As the show progresses, you are introduced to some of her colleagues who each have a unique role in game development (manager, artist, programmer, etc). The show is a relaxed but fun watch, and was well received.
In Season 2 Episode 1 we see a scene where Nene (an aspiring game developer) is struggling with a bug and asks for Umiko (a professional programmer) for help fixing it. Nene has created a simple game that she can run and interact with, but eventually we see it crash. She asks for help debugging the problem, and for a few seconds around 9:32, we see this snippet of code.
The code is as follows (comments removed):
void DestructibleActor::ReceiveDamage(float sourceDamage)
{
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();
}
}
}It should again be noted that in the show it is explicitly mentioned that there is a bug in this code. Umiko looks at the code and points out that the if block is running the same operations continuously. Indeed, we can see that to be true. In this case, if the player has several debuffs and has their health drop below 0 the DestroyMe method will be called multiple times. But why is that a problem?
Understanding what's happening here requires a few reasonable assumptions to be made. Typically, in low level languages and in game development when an object is removed it cleans itself up by deleting instances of itself in memory (failure to do this commonly results in memory leaks). In other words, not only is something removed from the screen, but it is removed from the game as well. A very common bug that developers come across is when subsequent code tries to perform an operation on this object that no longer exists. Because it has been removed not just from the screen, but also in memory, the program doesn't know how to do what the code is asking and crashes.
In this case, we might have a player object with two or more debuffs take fatal damage, which would result in two or more invocations of the DestroyMe method. The first would remove the player. The second would attempt to remove them again, discover there is no longer a player to remove, and subsequently crash.
Beyond the error pointed out by Umiko, the code does have other issues. Should the user have no debuffs on them, then the for loop will never run. This, essentially, would make the player immune to damage as the logic to apply damage to their health only exists within the loop.
Each debuff loop also subtracts damage from the character in a way that likely results in excessive damage being applied. This is a little more theoretical without being able to see the methods being called, but the function appears to indicate that each debuff would result in a damage value being removed from the player's health. For instance, say the player currently has a Burn debuff that increases damage by 20%, and a Blind debuff that has no impact on damage. The code would run in this way:
Call method with (sourceDamage = 100)
For debuffs in [Burn, Blind]
- Burn:
resolvedDamage = 120 (100 * 1.2)
currentHealth -= 120;
- Blind:
resolvedDamage = 120 (last loop: 120 * 1.0)
currentHealth -= 120
Total Damage: 240The player would take 240 damage because each of the for loops deals the compounding damage instead of the function applying the total damage only at the end. In this example, the player takes the original 120 damage from the burn, but an additional 120 from the blind as well. The reason the player takes 120 from blind and not 100 is because the Blind loop references the last value in the loop, which was 120 from the Burn damage.
The code could potentially return the differential damage rather than the total, but that could result in the following bug:
Call method with (sourceDamage = 100)
For debuffs in [Blind, Burn]
- Blind:
resolvedDamage = 0 (100 * 0) // No additional
currentHealth -= 0
- Burn:
resolvedDamage = 0 (0 * 0.2) // Only the 20% additional
currentHealth -= 0;
Total Damage: 0The order of debuffs here would still result in a bug because the loop refers to the previous resolvedDamage instead of the sourceDamage. It also would never remove the sourceDamage from the player's health.
Ultimately, even if Nene fixes the runtime error crash, there will be bugs left in her code that she'll need to fix. A correct implementation might look like this:
void DestructibleActor::ReceiveDamage(float sourceDamage)
{
m_currentHealth -= sourceDamage;
for (const auto& debuf : m_debufs)
{
// Assuming this returns a modifier rather than the sum
auto resolvedDamage = debuf->ApplyToDamage(sourceDamage);
m_currentHealth -= resolvedDamage;
}
if (m_currentHealth <= 0.f)
{
m_currentHealth = 0.f;
DestroyMe();
}
}New Game puts an interesting spin on this code by creating the premise that there is a bug in it. I think that's pretty cool, and I want to take a moment to give them props for finding not only code that fits the show and genre by clearly being game-related, but also in structuring it in such a way that there is a clear logical error in it which results in the runtime error Nene shows Umiko. The fact that the issue can be understood by looking at this snippet without additional context from the rest of the game code is particularly notable.
New Game gets "Changes Requested" on their Pull Request, but a big thumbs up because they were supposed to.
Questions or comments? Tweet us at @fansubbing! You can also follow us for updates on the latest articles, too.