Just before the release of the "Amnesia: Rebirth" game, the vendor "Fractional Games" opened the source code of the legendary "Amnesia: The Dark Descent" and its sequel "Amnesia: A Machine For Pigs". Why not use the static analysis tool to see what dreadful mistakes are hidden in the inside of these cult horror games? After seeing the news on Reddit that the source code of the games " " and " " was released, I couldn't pass by and not check this code using PVS-Studio, and at the same time write an article about it. Especially since the new part of this game-series - " " is released on the 20th of October (and at the moment of posting this article the game is already released). Amnesia: The Dark Descent Amnesia: A Machine for Pigs Amnesia: Rebirth "Amnesia: The Dark Descent" was released in 2010 and became a cult game in the survival horror genre. Frankly speaking, I have never been able to play through it, even a bit. The reason is that in horror games I play by one algorithm: install, run for five minutes, exit by "alt+f4" at the first creepy moment and delete the game. But I liked watching this game run through videos on YouTube. In case someone is not familiar with PVS-Studio yet, this is a static analyzer that looks for errors and suspicious places in source code of programs. I especially like delving into source code of games. So, if you are interested in what errors are made in games, you can read my previous articles. After checking, it turned out that a large amount of code overlaps between "The Dark Descent" and "A Machine For Pigs", and the reports for these two projects were very similar. So almost all the errors that I will cite further take place in both projects. The better half of errors found by the analyzer in these projects was copy-paste errors. This explains the title of the article. The main reason for these errors is the " ". last line effect Let's get right to it. Copy-paste errors There were a lot of suspicious places that looked like inattentive copying. Some cases may be due to the internal logic of the game itself. Once they confused both the analyzer and me, at least a comment could be of use. After all, other developers may be as clueless as I am. Fragment 1. Let's start with an example where the entire function consists of comparing method results and fields values of two objects and . I will cite the entire function for clarity. Try to see for yourself where the error was made in the function: aObjectDataA aObjectDataB Here's a pic for you to avoid accidentally spying on the answer: Did you find a bug? So, in the last , there is a comparison using on both sides. Note that all the expressions in the original code were written in a line. Here I broke lines so that everything fits exactly in the width of the line. Just imagine how hard it will be to look for such a flaw at the end of the working day. Whereas the analyzer will find it immediately after building the project and running incremental analysis. return aObjectDataA There are identical sub-expressions 'aObjectDataA.mpObject->GetVertexBuffer()' to the left and to the right of the '<' operator. WorldLoaderHplMap.cpp 1123 V501 As a result, such an error will be found almost at the time of writing the code, instead of hiding in the depths of the code from several stages of Q&A, making your search much more difficult. Note by my colleague Andrey Karpov. Yes, this is a classic "last line effect" error. Moreover, this is also a classic pattern of the error related to comparing two objects. See the article " ". The Evil within the Comparison Functions Fragment 2. Let's take a quick look at the code that triggered the warning Here is a screenshot of the code for clarity. This is how the warning looks like: There are identical sub-expressions 'lType == eLuxJournalState_OpenNote' to the left and to the right of the '||' operator. LuxJournal.cpp 2262 V501 The analyzer found that there is an error in the check of the variable value. The equality with the same element of the enumerator is checked twice. lType eLuxJournalState_OpenNote First, I wish this condition would be written in a table-like format for better readability. In this form, it becomes much easier to notice the error even without the analyzer. Anyway, here comes a question - does such an erroneous check lead to logic distortion of the program? After all, one may need to check some other value, but the check was missed due to a copy-paste error. So, let's take a look at the enumeration itself: lType There are only three values with the word "Open" in their name. All three are present in the check. Most likely, there is no logic distortion here, but we can hardly know for sure. So, the analyzer either found a logical error that the game developer could fix, or found an "ugly" written snippet that would be worth rewriting for better elegancy. Fragment 3. The following case is generally the most obvious example of a copy-paste error. Two similar code fragments were found. Perhaps, this is a typo and 'mvSearcherIDs' variable should be used instead of 'mvAttackerIDs'. LuxSavedGameTypes.cpp 615 V778 In the first loop, the pointer (obtained via ) is handled. If the condition is not met, a debug message is issued for the same . However, in the next loop, which is formed the same as the previous code section, is obtained using . While the warning is still issued with the mention of . pEntity mvAttackerIDs mvAttackerIDs pEntity mvSearcherIDs mvAttackerIDs Most likely, the code block with the note "Searchers" was copied from the "Attackers" block, was replaced with , but the block was not changed. As a result, the error message uses an element of the wrong array. mvAttackerIDs mvSearcherIDs else This error does not affect the logic of the game, but this way you can play a dirty trick on a person who will have to debug this place, and waste time working with the wrong element. mvSearcherIDs Fragment 4. The analyzer indicated the next suspicious fragment with as many as three warnings: Expression 'pEntity == 0' is always false. LuxScriptHandler.cpp 2444 V547 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains function return. This means that the second 'if' statement is senseless. Check lines: 2433, 2444. LuxScriptHandler.cpp 2444 V649 Consider checking for misprints. It's possible that the 'pTargetEntity' should be checked here. LuxScriptHandler.cpp 2444 V1051 Take a look at the code: The warning was issued for the second check. For the analyzer, this check will always be , since if this condition were , the function would exit earlier due to a previous similar check. V547 pEntity == NULL false true The next warning ( ) was issued just for the fact that we have two identical checks. Usually this case may not be a mistake. Who knows, may be one part of the code implements the same logic, and another part of the code must do something else based on the same check. But in this case, the body of the first check consists of , so it won't even get to the second check if the condition is . By tracking this logic, the analyzer reduces the number of false messages for suspicious code and outputs them only for very strange logic. V649 return true The error indicated by the last warning is very similar in nature to the previous example. Most likely, all checks were duplicated from the first check, and then the object being checked was replaced with the required one. In the case of the and objects, the replacement was made, but the object was forgotten. As a result, this object is not checked. if(pEntity == NULL) pBody pTargetBody pTargetEntity If you dig a bit deeper into the code of the example we are considering, it turns out that such an error will not affect the program performance. The pointer gets its value from the function: pTargetBody GetBodyInEntity The first passed argument here is an unchecked pointer that is not used anywhere else. Fortunately, inside this function there is a check of the first argument for : NULL As a result, this code works correctly in the end, although it contains an error. Fragment 5. Another suspicious place with copy-paste! In this method, the fields of the class object are zeroed. cLuxPlayer But for some reason, the two variables and are zeroed twice: mfRollSpeedMul mfRollMaxSpeed The 'mfRollSpeedMul' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 298, 308. LuxPlayer.cpp 308 V519 The 'mfRollMaxSpeed' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 299, 309. LuxPlayer.cpp 309 V519 Let's look at the class itself and at its fields: Interestingly, there are three similar variable blocks with related names: , , and . In , these three blocks are reset to zero, except for the last two variables from the third block, and . Just in place of these two variables, duplicated assignments are found. Most likely, all these assignments were copied from the first assignment block, and then the variable names were replaced with the necessary ones. mfRoll mfLeanRoll mvCamAnimPos Reset mfCamAnimPosSpeedMul mfCamAnimPosMaxSpeed It may be that the two missing variables should not have been reset, but the opposite is also very likely. In any case, repeated assignments will not be of great help in supporting this code. As you can see, in a long set of identical actions, you may not notice such an error, and the analyzer helps you out here. Fragment 5.5. The code is very similar to the previous one. Let me give you a code snippet and a warning from the analyzer for it right away. The 'mfTimePos' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 49, 53. AnimationState.cpp 53 V519 The variable was set to 0 twice. As in the previous example, let's get into in the declaration of this field: mfTimePos You may notice that this declarations block also corresponds to the assignment order in the erroneous code snippet, as in the previous example. Here in the assignment, gets the value instead of the variable. Except for in this case, the error can't be explained by copying the block and the "last line effect". may not need to be assigned a new value, but this piece of code is still dubious. mfTimePos mfLength mfLength Fragment 6. This part of code from "Amnesia: A Machine For Pigs" sparked off the analyzer to issue a truckload of warnings. I will give only a part of the code that triggered errors of the same kind: Where is the error here? Here are the analyzer warnings: The enumeration constant 'eLuxEnemyMoveState_Jogging' is used as a variable of a Boolean-type. LuxEnemyMover.cpp 672 V768 The enumeration constant 'eLuxEnemyMoveState_Walking' is used as a variable of a Boolean-type. LuxEnemyMover.cpp 680 V768 The enumeration constant 'eLuxEnemyMoveState_Jogging' is used as a variable of a Boolean-type. LuxEnemyMover.cpp 688 V768 The if-else-if sequence in the original code is repeated, and further these warnings were issued for each body of each . else if Let's consider the line that the analyzer points to: No surprise, an error crept into such an expression, originally written in line. And I'm sure you've already noticed it. The enumeration element is not compared with anything, but its value is checked. Most likely, the expression 'prevMoveState == eLuxEnemyMoveState_Jogging'was meant. eLuxEnemyMoveState_Jogging Such a mistake may seem quite harmless. But in another article about checking the Bullet Engine, among the commits to the project, I found a of the same kind, which led to the fact that forces were applied to objects from the wrong side. As for this case, this error was made several times. Well, note that the ternary condition is completely meaningless since it will be applied to the boolean results of logical operators in the last place. fix for an error Fragment 7. Finally, the last couple of examples of copy-paste errors. This time again in a conditional statement. The analyzer issued a warning for this piece of code: I think that in such a separate fragment from the entire code, it is quite easy to notice an awkward place. Nonetheless, the error found its way to hide from the developers of this game. The analyzer issued the following message: There are identical sub-expressions to the left and to the right of the '||' operator: avSubDiv.x > 1 || avSubDiv.x > 1 ParticleEmitter.cpp 199 V501 The second parenthesis in the condition indicates that both and fields are checked. But in the first parenthesis, for some reason, this point was missed and only the field is checked. In addition, judging by the review comment, both fields should have been checked. So it is not the "last line effect" that has worked here, but rather the "first line effect", since in the first parenthesis the author forgot to replace accessing the field with accessing the field. x y x x y Obviously, such errors are very insidious, since in this case even the explanatory comment to the condition did not help the developer. In such cases, I would recommend making it a habit to record related checks in tabular form. This way, it is easier both to edit, and notice a defect: Fragment 7.5. An absolutely similar error was found in a different place: Did you get a chance to see where it was hiding? It is not for nothing that we've already dealt with so many examples :) The analyzer has issued a warning: There are identical sub-expressions to the left and to the right of the '==' operator: edge1.tri1 == edge1.tri1 Math.cpp 2914 V501 We'll sort this fragment through one part after another. Obviously, the first check checks the equality of the fields and , and at the same time the equality of and : edge1.tri1 edge2.tri2 edge1.tri2 edge2.tri2 In the second check, judging by the correct part of the check 'edge1.tri2 == edge2.tri1', equality of these fields had to be checked in a crosswise way: But instead of checking for , there was a pointless check . By the way, all this is in the function, I removed nothing. Still such an error has wormed into the code. edge1.tri1 == edge2.tri2 edge1.tri1 == edge1.tri1 Other errors Fragment 1. Here is the following code snippet with the original indents. PVS-Studio warning: It is possible that this 'else' branch must apply to the previous 'if' statement. CharacterBody.cpp 1591 V563 This example can be confusing. Why does have the same indent as the outer one at the level? Is it implied that is for the outermost condition? Well, then one has to place braces correctly, otherwise refers to the right-front . else if else else if Or is it not so? When writing this article, I changed my mind several times about which version of the actions sequence for this code was most likely. If we dig a bit deeper into this code, it turns out that the variable , which is compared in the lower , can't have a value less than zero, since it gets the value from the method: fForwardSpeed if Length Then, most likely, the point of these checks is that we first check whether the element is greater than zero, and then check its value relative to . Moreover, the last two statements correspond to each other in terms of the wording. mfMoveSpeed fForwardSpeed if In this case, the original code will work as intended! But it will definitely make the one who comes to edit/refactor it rack their brains. I thought I'd never come across code that looked like this. Out of interest, I looked at our found in open-source projects and described in articles. Examples of this error were also found in other projects - you can look at them . collection of errors yourself Please, don't write like this, even if you are clear about it yourself. Use braces, or correct indentation, or better - both. Don't make those who come to understand your code suffer, or yourself in the future ;) Fragment 2. This error has taken me aback, so it took a while to find logic here. In the end, it still seems to me that this is most likely a mistake, a quite whopping one. Take a look at the code: It is dangerous to create a local variable within a loop with a same name as a variable controlling this loop. BinaryBuffer.cpp 371 V711 So, we have a variable, which controls the exit from the loop. But inside this loop, instead of assigning a new value to this external variable, a new variable named is declared. As a result, it overrides the external variable, and the variable that is checked in the loop condition will never change. ret do-while ret ret With mishap coinciding, such a loop could become infinite. Most likely, in this case, it is an internal condition that saves this code. It checks the value of the internal variable and leads to the exit of the function. ret Conclusion Very often, developers do not use static analysis regularly, but with long breaks. Or even run the project through the analyzer only once. As a result of this approach, the analyzer often does not detect anything serious or finds something like the examples we are considering, which may not particularly affect the game's performance. One gets the impression that the analyzer is not really useful. Well, it found such places, but everything still works. The fact is that there were similar places where a mistake was on the surface, and definitely resulted in a program error. These fragments have already been refined due to long hours of debugging, test runs, Q&A department. As a result, when the analyzer checks the project only once, it shows only those problems that did not manifest themselves in any way. Sometimes such problems comprise critical issues that actually affected the program but will not likely to follow their scenario. Therefore, this error was unknown to the developers. That's why it is extremely important to evaluate the usefulness of static analysis only after its regular use. Once a one-time run through PVS-Studio revealed such suspicious and sloppy fragments in the code of this game, imagine how many obvious errors of this kind had to be localized and fixed in course of development. Use a static analyzer regularly! Previously published at https://www.viva64.com/en/b/0769/

Fixing Code Errors in Amnesia: The Dark Descent

About Author

Comments

TOPICS

THIS ARTICLE WAS FEATURED IN

Related Stories

Untitled Story

Bugs from the 90's: The Code of Command and Conquer [Volume 2]

Decentral Games and Amnesia Ibiza Partner to Stage a Mind-Blowing Metaverse DJ Event

#10 Rules of Bug Bounty

The Noonification: Do Leaders Really Need Emotional Intelligence? (10/14/2022)

3 Web Design Factors to Ponder Before Starting an E-commerce Venture

Bugs from the 90's: The Code of Command and Conquer [Volume 2]

Decentral Games and Amnesia Ibiza Partner to Stage a Mind-Blowing Metaverse DJ Event

#10 Rules of Bug Bounty

The Noonification: Do Leaders Really Need Emotional Intelligence? (10/14/2022)

3 Web Design Factors to Ponder Before Starting an E-commerce Venture

Light-Mode

Classic

Newspaper

Dark-Mode

Neon Noir

Minty

HN StartUps