It's cool when enthusiastic developers create a working clone of a famous game. It's even cooler when people are ready to continue the development of such projects! In this article, we check TheXTech with PVS-Studio. TheXTech is an open implementation of the game from the Super Mario universe.
TheXTech is the SMBX 1.3. game engine rewritten on C++. The original SMBX (Super Mario Bros. X) was written on Visual Basic 6 by Andrew Spinks in 2009. It allows to create levels from the elements of the Nintendo's Super Mario Bros games. TheXTech accurately reproduces the original game's behavior. It also includes optional bug fixes. It runs not only on Windows, but also on macOS, and Linux systems with x86, ARM or PowerPC processors. Some developers also ported it on 3DS and PS Vista
The TheXTech developer — Vitaliy Novichkov (Wohlstand) — described the development process in detail on Habr. He also described the techniques he used to smooth out the differences when porting the project from VB6 to C++. There's a disclaimer on the GitHub page that explains why the source code is not in the best condition. It's because the original code is unstructured something fierce. Its fragments you'll see below.
Fragment one
Can you see the error the analyzer found below?
V547 Expression 'NPC[A].Type == 54 && NPC[A].Type == 15' is always false. Probably the '||' operator should be used here. thextech npc_update.cpp 1277
Of course not :) The error hides in the middle of the condition in string that has a 1400 character long. You have to scroll 5 screens to the right to find it. Let's format the code:
else if(
NPC[A].Type == 21 || NPC[A].Type == 22 || NPC[A].Type == 25
|| NPC[A].Type == 26 || NPC[A].Type == 31 || NPC[A].Type == 32
|| NPC[A].Type == 238 || NPC[A].Type == 239 || NPC[A].Type == 35
|| NPC[A].Type == 191 || NPC[A].Type == 193
|| (NPC[A].Type == 40 && NPC[A].Projectile == true) || NPC[A].Type == 49
|| NPC[A].Type == 58 || NPC[A].Type == 67 || NPC[A].Type == 68
|| NPC[A].Type == 69 || NPC[A].Type == 70
|| (NPCIsVeggie[NPC[A].Type] && NPC[A].Projectile == false)
|| (NPC[A].Type == 29 && NPC[A].Projectile == true)
|| (NPC[A].Projectile == true
&& (NPC[A].Type == 54 && NPC[A].Type == 15)) // <=
|| .... )
{ .... }Now you can see it. The NPC[A].Typevariable cannot be equal to two different values at the same time. Apparently, the condition was intended to be true for projectiles of types 54 and 15. However, now this part of the condition is always false. The developer should have changed the AND logical operator to the OR logical operator. Another option is to delete this part of the expression.
A couple of error examples in too long lines:
Fragment two
The next code fragment was formatted for reading. Despite the greater chance of noticing errors here, someone missed them. Even 4 of them:
You can click on the picture to see the highlighted errors.
There is no point in double checking the same values here. Unnecessary comparisons can be removed.
No screenshots needed further.
Fragment three
V501 There are identical sub-expressions '(evt.AutoSection) >= (0)' to the left and to the right of the '&&' operator. thextech layers.cpp 568
#define IF_INRANGE(x, l, r) ((x) >= (l) && (x) <= (r))
else if( IF_INRANGE(evt.AutoSection, 0, maxSections)
&& IF_INRANGE(evt.AutoSection, 0, maxEvents))
{
// Buggy behavior, see https://github.com/Wohlstand/TheXTech/issues/44
AutoX[evt.AutoSection] = Events[evt.AutoSection].AutoX;
AutoY[evt.AutoSection] = Events[evt.AutoSection].AutoY;
}In this code fragment the analyzer was confused by the duplication of expressions. This duplication appeared as a result of the macro expansion:
((evt.AutoSection) >= (0) && (evt.AutoSection) <= (maxSections)) &&
((evt.AutoSection) >= (0) && (evt.AutoSection) <= (maxEvents))Such warnings can be suppressed. The developer can also rewrite the condition like this:
IF_INRANGE(evt.AutoSection, 0, min(maxSections, maxEvents))This string also triggered the V590 rule.
V590 Consider inspecting this expression. The expression is excessive or contains a misprint. thextech layers.cpp 568
If we fix those warnings, it won't fix any bugs. The compilers delete unnecessary constructions anyway. However, we can clean up the code this way.
By the way, you can find an interesting moment, in this code fragment. Just follow the link from the code fragment's comment and look at the issue. A user named ds-sloth suggested the following fix — to change this line:
AutoX[Events[A].AutoSection] = Events[Events[A].AutoSection].AutoX;into this:
AutoX[Events[A].AutoSection] = Events[A].AutoX;This change would fix the auto-scroll mechanism that is controlled by in-game events:
You can click on the picture to see the animation.
However, this fix is disabled by default because it changes or breaks the game behavior:
Wohlstand: I guess the fix of this bug doesn't break levels that intended to work it, but breaks level that contains an absolutely invalid data and didn't intended to auto-scroll at all (knowing it was broken), in the result the issue #65. I guess since the 38A's per-section auto-scroll setup was utilized, when I will implement the way at PGE Editor to set them, I'll set this autoscrolling compat.ini value into 'false' by default.
Wohlstand: Okay, I had to set the auto-scroll fix into 'false', mainly because there are several faulty levels that kept bad auto-scroll data after unsuccess experiments done by users, they making much more pain than proper use of this thing.
Therefore, in some cases, fixing the error requires consideration — fixing some of them may break the bug compatability :). The following examples show such cases.
Fragment four
V501 There are identical sub-expressions to the left and to the right of the '!=' operator: NPC[A].Projectile != NPC[A].Projectile thextech npc_hit.cpp 2105
else if ( NPC[A].Location.SpeedX != oldNPC.Location.SpeedX
|| NPC[A].Location.SpeedY != oldNPC.Location.SpeedY
|| NPC[A].Projectile != NPC[A].Projectile // <=
|| NPC[A].Killed != oldNPC.Killed
|| NPC[A].Type != oldNPC.Type
|| NPC[A].Inert != oldNPC.Inert)
{ .... }This code fragment compares a set of data members in the NPC[A] and oldNPC objects. In the middle of this fragment the Projectile members of NPC[A] is compared with itself. Looks like a sloppy copypaste. Classic. However, only testing (or a full understanding of the game's logic) shows what would happen after we fix this condition. Maybe there's just an redundant check.
Similar error:
Fragment five
The last V501 error for today:
V501 There are identical sub-expressions 'MenuMode == MENU_SELECT_SLOT_1P_DELETE' to the left and to the right of the '||' operator. thextech menu_main.cpp 1004
// Delete gamesave
else if( MenuMode == MENU_SELECT_SLOT_1P_DELETE
|| MenuMode == MENU_SELECT_SLOT_1P_DELETE)
{
if(MenuMouseMove)
s_handleMouseMove(2, 300, 350, 300, 30);
....It's unclear whether only the first player should have the right to delete the save slot. In this case, the additional check for MENU_SELECT_SLOT_1P_DELETE is unnecessary here. Nevertheless, the code has the MENU_SELECT_SLOT_2P_DELETE constant. Probably, this constant should have been used in the right part of the expression.
This condition block has the same warning just below:
Fragment six
V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 1561, 1570. thextech player_update.cpp 1561
if(Player[A].Character == 2) // luigi doesn't fly as long as mario
Player[A].FlyCount = 300; // Length of flight time
else if(Player[A].Character == 3) // special handling for peach
{
Player[A].FlyCount = 0;
Player[A].RunCount = 80;
Player[A].CanFly2 = false;
Player[A].Jump = 70;
Player[A].CanFloat = true;
Player[A].FlySparks = true;
}
else if(Player[A].Character == 3) // special handling for peach
Player[A].FlyCount = 280; // Length of flight time
else
Player[A].FlyCount = 320; // Length of flight timeIn this fragment, several else-if constructs with the same condition (Player[A].Character == 3) do subsequent checks. This leads to the unreachable code in the second else-if construct. Looks like this code fragment prevents Princess Peach from flying in some places. We can try to remove the extra branch and simply assign 280 to the Player[A].FlyCount variable.
Fragment seven
The analyzer has detected suspicious code duplication in the then and else branches of condition:
V523 The 'then' statement is equivalent to the 'else' statement. thextech npc_hit.cpp 1546
if(NPC[C].Projectile && !(NPC[C].Type >= 117 && NPC[C].Type <= 120))
{
if(!(NPC[A].Type == 24 && NPC[C].Type == 13))
NPC[A].Killed = B;
else
NPC[A].Killed = B;
}Maybe some special exception is broken where this function determines whether a projectile can kill a specific type of NPC.
Fragment eight
The analyzer has detected an impossible condition:
V547 Expression 'A == 48' is always false. thextech effect.cpp 1652
else if(A == 16) // Dead Giant Bullet Bill
{
numEffects++;
Effect[numEffects].Shadow = Shadow;
....
Effect[numEffects].Location.SpeedY = Location.SpeedY;
Effect[numEffects].Location.SpeedX = Location.SpeedX;
if(A == 48) // <=
Effect[numEffects].Location.SpeedY = -8;
Effect[numEffects].Life = 120;
Effect[numEffects].Type = A;
}Since the program can enter this block only if the A variable equals 16, the A == 48 condition is never fulfilled. As a result the effect will have the wrong vertical velocity. So the Giant Bullet Bill's death will not be dramatic enough. :)
Fragment nine
Another example of a useless conditional operator:
V547 Expression 'tempPlayer == 0' is always true. thextech blocks.cpp 576
// don't spawn players from blocks anymore
tempPlayer = 0;
if(tempPlayer == 0) // Spawn the npc
{
numNPCs++; // create a new NPC
NPC[numNPCs].Active = true;
NPC[numNPCs].TimeLeft = 1000;
....Apparently, after refactoring, the tempPlayer variable is always initialized to zero. We can reduce the code nesting by removing an unnecessary condition.
Fragment ten
Here is an extra check that the logical result of the comparison is not equal to 0:
V562 It's odd to compare a bool type value with a value of 0. thextech editor.cpp 102
if(!MagicHand)
{
if((getKeyState(vbKeyPageUp) == KEY_PRESSED) != 0) // <=
{
if(ScrollRelease == true)
....We can write simply:
if(getKeyState(vbKeyPageUp) == KEY_PRESSED)More of such warnings:
Fragment eleven
The following example may contain a logical error. The condition first checks the value of the array by the whatPlayer index. Only after that the fragment checks the whatPlayer variable's range:
V781 The value of the 'whatPlayer' index is checked after it was used. Perhaps there is a mistake in program logic. thextech blocks.cpp 159
if(b.ShakeY != 0 || b.ShakeY2 != 0 || b.ShakeY3 != 0)
{
if( b.RapidHit > 0
&& Player[whatPlayer].Character == 4 && whatPlayer > 0) // <=
{
b.RapidHit = (iRand() % 3) + 1;
}
return;
}This may result in undefined behavior.
Fragment twelve
A bit weird fragment. After the developer commented the part of an expression, the variable began to assign itself the same value:
V570 The 'NPC[A].Location.X' variable is assigned to itself. thextech npc_hit.cpp 1995
else
{
NPC[A].Location.Y = NPC[A].Location.Y + NPC[A].Location.Height;
NPC[A].Location.X = NPC[A].Location.X; // - (32 - .Location.Width) / 2
....
}The program's behavior doesn't change from such expressions. However, this code fragment may indicate logical errors. For example, a logical error appears if, after debugging, the developer doesn't put the commented fragment back.
There are examples of unnecessary assignment:
Fragment thirteen
A weird loop in a function that tries to read a JPEG image:
V654 The condition 'chunk_size > 0' of loop is always true. thextech image_size.cpp 211
static bool tryJPEG(SDL_RWops* file, uint32_t *w, uint32_t *h)
{
....
size_t chunk_size = 0;
....
do
{
SDL_memset(raw, 0, JPEG_BUFFER_SIZE);
pos = SDL_RWtell(file);
chunk_size = SDL_RWread(file, raw, 1, JPEG_BUFFER_SIZE);
if(chunk_size == 0)
break;
head = findJpegHead(raw, JPEG_BUFFER_SIZE);
if(head)
{
if(head + 20 >= raw + JPEG_BUFFER_SIZE)
{
SDL_RWseek(file, -20, RW_SEEK_CUR);
continue; /* re-scan this place */
}
if(SDL_memcmp(head, "\xFF\xE1", 2) == 0) /* EXIF, skip it!*/
{
const Sint64 curPos = pos + (head - raw);
Sint64 toSkip = BE16(head, 2); //-V629
SDL_RWseek(file, curPos + toSkip + 2, RW_SEEK_SET);
continue;
}
*h = BE16(head, 5);
*w = BE16(head, 7);
return true;
}
} while(chunk_size > 0); // <=
return false;
}The chunk_size variable is updated almost at the very beginning of the loop iteration. If the variable equals zero, the loop breaks. After that the variable goes to checking the exit condition of the loop. However, it is guaranteed to be greater than zero. Here we can use the infinite while (true) loop.
Fragment fourteen
This code fragment has the bitwise OR operator instead of the logical one. This operator is used between calls of functions that return bool. As a result, both functions are always executed, which is less effective:
V792 The 'vScreenCollision' function located to the right of the operator '|' will be called regardless of the value of the left operand. Perhaps, it is better to use '||'. thextech gfx_update.cpp 1007
bool vScreenCollision(int A, const Location_t &Loc2)
....
// warp NPCs
if(Player[A].HoldingNPC > 0 && Player[A].Frame != 15)
{
if(( vScreenCollision(Z, NPC[Player[A].HoldingNPC].Location)
| vScreenCollision(Z, newLoc(....))) != 0 // <=
&& NPC[Player[A].HoldingNPC].Hidden == false)
{
....The same error appears in other places:
Fragment fifteen
In the following example the developer constructs an unnecessary string, passing the result of calling the c_str() member function. The developer passes it to the function which accepts a reference to std::string. The code is less efficient that way. When the developer converts std::string into char*, information about the current length of the string is lost. When subsequently constructing new std::string, the program has to recalculate the length by a linear search for terminal null character. The compiler doesn't optimize this moment — we checked it with Clang with -O3 optimizations.
V811 Decreased performance. Excessive type casting: string -> char * -> string. Consider inspecting first argument of the function open_file. thextech graphics_funcs.cpp 63
bool FileMapper::open_file(const std::string& path)
{
return d->openFile(path);
}
FIBITMAP *GraphicsHelps::loadImage(std::string file, bool convertTo32bit)
{
....
if(!fileMap.open_file(file.c_str())) // <=
return nullptr;
....
}Fragment sixteen
In this loop the length of the same strings is repeatedly calculated. The developer should declare it as constants of the std::string type and use the size() method:
V814 Decreased performance. The 'strlen' function was called multiple times inside the body of a loop. thextech menu_main.cpp 1027
#define For(A, From, To) for(int A = From; A <= To; ++A)
if(MenuMouseMove)
{
For(A, 0, optionsMenuLength)
{
if(MenuMouseY >= 350 + A * 30 && MenuMouseY <= 366 + A * 30)
{
if(A == 0)
menuLen = 18 * std::strlen("player 1 controls") - 4; // <=
else if(A == 1)
menuLen = 18 * std::strlen("player 2 controls") - 4; // <=
....This pattern is quite common:
According to Wikipedia (ru), TheXTech was first publicly released just a month after the SMBX source code was published. It's really cool for a full cross-platform project porting to another language. Especially on C++.
Developers planning a major code revision can try PVS-Studio. We provide a free license for open source projects.
As a bonus — here's the Mario-themed video from our YouTube channel: