If you are a software developer working in the video game industry and wondering what else you could do to improve the quality of your product or make the development process easier and you don't use static analysis – it's just the right time to start doing so. You doubt that? OK, I'll try to convince you. And if you are just looking to see what coding mistakes are common with video-game and game-engine developers, then you're, again, at the right place: I have picked the most interesting ones for you.
Although video-game development includes a lot of steps, coding remains one of the basic ones. Even if you don't write thousands of code lines, you have to use various tools whose quality determines how comfortable the process is and what the ultimate result will be. If you are a developer of such tools (such as game engines), this shouldn't sound new to you.
Why is static analysis useful in software development in general?
The main reasons are as follows:
But it's all theory, and you are probably interested in real-life examples. Well then, I've got some.
If you have read this far, I assume you don't need me telling you about Unreal Engine or the Epic Games company – and if you don't hold these guys in high regard, I wonder whom you do.
The PVS-Studio team has cooperated with Epic Games a few times to help them adopt static analysis in their project (Unreal Engine) and fix bugs and false positives issued by the analyzer. I'm sure both parties found this experience interesting and rewarding.
One of the effects of this cooperation was adding a special flag into Unreal Engine allowing the developers to conveniently integrate static analysis into the build system of Unreal Engine projects.
The idea is simple: the guys do care about the quality of their code and adopt various techniques available to maintain it, static analysis being one of them.
John Carmack, one of the most renowned video-game developers, once called the adoption of static analysis one of his most important accomplishments as a programmer: "The most important thing I have done as a programmer in recent years is to aggressively pursue static code analysis." The next time you hear someone say that static analysis is a tool for newbies, show them this quote. Carmack described his experience in this article, which I strongly recommend checking out – both for motivation and general knowledge.
One of the best ways to prove that static analysis is a useful method is probably through examples showing it in action. That's what the PVS-Studio team does while checking open-source projects.
It's a practice that everyone benefits from:
So, isn't that proof of the effectiveness of this approach?
While some are pondering introducing static analysis into their development process, others have long been using and benefiting from it! These are, among others, Rocksteady, Epic Games, ZeniMax Media, Oculus, Codemasters, Wargaming (source).
I should point right off that this is not some ultimate top list, but simply bugs which were found by PVS-Studio in video games and game engines and which I found most interesting.
As usual, I recommend trying to find the bug in each example on your own first and only then go on reading the warning and my comments. You'll enjoy the article more that way.
Tenth place
Source: Anomalies in X-Ray Engine
The tenth place is given to the bug in X-Ray Engine employed by the S.T.A.L.K.E.R game series. If you played them, you surely remember many of funny (and not quite funny) bugs they had. This is especially true for S.T.A.L.K.E.R.: Clear Sky, which was impossible to play without patches (I still remember the bug that 'killed' all my saves). The analysis revealed there were many bugs indeed. Here's one of them.
BOOL CActor::net_Spawn(CSE_Abstract* DC)
{
....
m_States.empty();
....
}PVS-Studio warning: V530 The return value of function 'empty' is required to be utilized.
The problem is quite simple: the programmer is not using the logical value returned by the empty method describing whether the container is empty or not. Since the expression contains nothing but a method call, I assume the programmer intended to clear the container but called the empty method instead of clear by mistake.
You may argue that this bug is too plain for a Top-10 list, but that's the nice thing about it! Even though it looks straightforward to someone not involved in writing this code, 'plain' bugs like that still appear (and get caught) in various projects.
Ninth place
Source: Long-Awaited Check of CryEngine V
Going on with bugs in game engines. This time it's a code fragment from CryEngine V. The number of bugs I have encountered in games based on this engine was not as large as in games based on X-Ray Engine, but it turns out it has plenty of suspicious fragments too.
void CCryDXGLDeviceContext::
OMGetBlendState(...., FLOAT BlendFactor[4], ....)
{
CCryDXGLBlendState::ToInterface(ppBlendState, m_spBlendState);
if ((*ppBlendState) != NULL)
(*ppBlendState)->AddRef();
BlendFactor[0] = m_auBlendFactor[0];
BlendFactor[1] = m_auBlendFactor[1];
BlendFactor[2] = m_auBlendFactor[2];
BlendFactor[2] = m_auBlendFactor[3];
*pSampleMask = m_uSampleMask;
}PVS-Studio warning: V519 The 'BlendFactor[2]' variable is assigned values twice successively. Perhaps this is a mistake.
As we mentioned many times in our articles, no one is safe from mistyping. Practice has also shown more than once that static analysis is very good at detecting copy-paste-related mistakes and typos. In the code above, the values of the m_auBlendFactor array are copied to the BlendFactor array, but the programmer made a mistake by writing BlendFactor[2] twice. As a result, the value at m_auBlendFactor[3] is written to BlendFactor[2], while the value at BlendFactor[3] remains unchanged.
Eighth place
Source: Unicorn in Space: Analyzing the Source Code of 'Space Engineers'
Let's change course a bit and take a look at some C# code. What we've got here is an example from the Space Engineers project, a 'sandbox' game about building and maintaining various structures in space. I haven't played it myself, but one guy said in the comments, "I'm not much surprised at the results :)". Well, we did manage to find some bugs worth mentioning, and here's two of them.
public void Init(string cueName)
{
....
if (m_arcade.Hash == MyStringHash.NullOrEmpty &&
m_realistic.Hash == MyStringHash.NullOrEmpty)
MySandboxGame.Log.WriteLine(string.Format(
"Could not find any sound for '{0}'", cueName));
else
{
if (m_arcade.IsNull)
string.Format(
"Could not find arcade sound for '{0}'", cueName);
if (m_realistic.IsNull)
string.Format(
"Could not find realistic sound for '{0}'", cueName);
}
}PVS-Studio warnings:
As you can see, it's a common problem, both in C++-code and C#-code, where programmers ignore methods' return values. The String.Format method forms the resulting string based on the format string and objects to substitute and then returns it. In the code above, the else-branch contains two string.Format calls, but their return values are never used. It looks like the programmer intended to log these messages in the same way as they did in the then-branch of the if statement using the MySandboxGame.Log.WriteLine method.
Seventh place
Source: Analyzing the Quake III Arena GPL project
Did I tell you already that static analysis is good at detecting typos? Well, here's one more example.
void Terrain_AddMovePoint(....) {
....
x = ( v[ 0 ] - p->origin[ 0 ] ) / p->scale_x;
y = ( v[ 1 ] - p->origin[ 1 ] ) / p->scale_x;
....
}PVS-Studio warning: V537 Consider reviewing the correctness of 'scale_x' item's usage.
The variables x and y are assigned values, yet both expressions contain the p->scale_x subexpression, which doesn't look right. It seems the second subexpression should be p->scale_y instead.
Sixth place
Source: Checking the Unity C# Source Code
Unity Technologies recently made the code of their proprietary game engine, Unity, available to the public, so we couldn't ignore the event. The check revealed a lot of interesting code fragments; here's one of them:
public override bool IsValid()
{
....
return base.IsValid()
&& (pageSize >= 1 || pageSize <= 1000)
&& totalFilters <= 10;
}PVS-Studio warning: V3063 A part of conditional expression is always true if it is evaluated: pageSize <= 1000.
What we have here is an incorrect check of the range of pageSize. The programmer must have intended to check that the pageSize value was within the range [1; 1000] but made a sad mistake by typing the '||' operator instead of '&&'. The subexpression actually checks nothing.
Fifth place
Source: Discussing Errors in Unity3D's Open-Source Components
This place was given to a nice bug found in Unity3D's components. The article mentioned above was written a year prior to revealing Unity's source code, but there already were interesting defects to find there at the time.
public static CrawledMemorySnapshot Unpack(....)
{
....
var result = new CrawledMemorySnapshot
{
....
staticFields = packedSnapshot.typeDescriptions
.Where(t =>
t.staticFieldBytes != null &
t.staticFieldBytes.Length > 0)
.Select(t => UnpackStaticFields(t))
.ToArray()
....
};
....
}PVS-Studio warning: V3080 Possible null dereference. Consider inspecting 't.staticFieldBytes'.
Note the lambda expression passed as an argument to the Where method. The code suggests that the typeDescriptions collection could contain elements whose staticFieldBytes member could be null – hence the check staticFieldBytes != null before accessing the Length property. However, the programmer mixed up the '&' and '&&' operators. It means that no matter the result of the left expression (true/false), the right one will also be evaluated, causing a NullReferenceException to be thrown when accessing the Length property if staticFieldBytes == null. Using the '&&' operator could help avoid this because the right expression won't be evaluated if staticFieldBytes == null.
Although Unity was the only engine to hit this top list twice, it doesn't prevent enthusiasts from building wonderful games on it. Including one(s) about fighting bugs.
Fourth place
Source: Analysis of Godot Engine's Source Code
Sometimes we come across interesting cases that have to do with missing keywords. For example, an exception object is created but never used because the programmer forgot to add the throw keyword. Such errors are found both in C# projects and C++ projects. There was one missing keyword in Godot Engine as well.
Variant Variant::get(const Variant& p_index, bool *r_valid) const
{
....
if (ie.type == InputEvent::ACTION)
{
if (str =="action")
{
valid=true;
return ie.action.action;
}
else if (str == "pressed")
{
valid=true;
ie.action.pressed;
}
}
....
}PVS-Studio warning: V607 Ownerless expression 'ie.action.pressed'.
In the given code fragment it is obvious that a programmer wanted to return a certain value of the Variant type, depending on the values ie.type and str. Yet only one of the return statements – return ie.action.action; – is written properly, while the other is lacking the return operator, which prevents the needed value from returning and forces the method to keep executing.
Third place
Source: PVS-Studio: analyzing Doom 3 code
Now we've reached the Top-3 section. The third place is awarded to a small code fragment of Doom 3's source code. As I already said, the fact that a bug may look straightforward to an outside observer and make you wonder how one could have made such a mistake at all shouldn't be confusing: there are actually all sorts of bugs to be found in the field...
void Sys_GetCurrentMemoryStatus( sysMemoryStats_t &stats ) {
....
memset( &statex, sizeof( statex ), 0 );
....
}PVS-Studio warning: V575 The 'memset' function processes '0' elements. Inspect the third argument.
To figure this error out, we should recall the signature of the memset function:
void* memset(void* dest, int ch, size_t count);If you compare it with the call above, you'll notice that the last two arguments are swapped; as a result, some memory block that was meant to be cleared will stay unchanged.
Second place
The second place is taken by a bug found in the code of the Xenko game engine written in C#.
Source: Catching Errors in the Xenko Game Engine
private static ImageDescription
CreateDescription(TextureDimension dimension,
int width, int height, int depth, ....) { .... }
public static Image New3D(int width, int height, int depth, ....)
{
return new Image(CreateDescription(TextureDimension.Texture3D,
width, width, depth,
mipMapCount, format, 1),
dataPointer, 0, null, false);
}PVS-Studio warning: V3065 Parameter 'height' is not utilized inside method's body.
The programmer made a mistake when passing the arguments to the CreateDescription method. If you look at its signature, you'll see that the second, third, and fourth parameters are named width, height, and depth, respectively. But the call passes the arguments width, width, and depth. Looks strange, doesn't it? The analyzer, too, found it strange enough to point it out.
First place
Source: A Long-Awaited Check of Unreal Engine 4
This Top-10 list is led by a bug from Unreal Engine. Just like it was with the leader of "Top 10 Bugs in the C++ Projects of 2017", I knew this bug should be given the first place the very moment I saw it.
bool VertInfluencedByActiveBone(
FParticleEmitterInstance* Owner,
USkeletalMeshComponent* InSkelMeshComponent,
int32 InVertexIndex,
int32* OutBoneIndex = NULL);
void UParticleModuleLocationSkelVertSurface::Spawn(....)
{
....
int32 BoneIndex1, BoneIndex2, BoneIndex3;
BoneIndex1 = BoneIndex2 = BoneIndex3 = INDEX_NONE;
if(!VertInfluencedByActiveBone(
Owner, SourceComponent, VertIndex[0], &BoneIndex1) &&
!VertInfluencedByActiveBone(
Owner, SourceComponent, VertIndex[1], &BoneIndex2) &&
!VertInfluencedByActiveBone(
Owner, SourceComponent, VertIndex[2]) &BoneIndex3)
{
....
}PVS-Studio warning: V564 The '&' operator is applied to bool type value. You've probably forgotten to include parentheses or intended to use the '&&' operator.
I wouldn't be surprised if you read the warning, looked at the code, and wondered, "Well, where's the '&' used instead of '&&'?" But if we simplify the conditional expression of the if statement, keeping in mind that the last parameter of the VertInfluencedByActiveBone function has a default value, this will clear it all up:
if (!foo(....) && !foo(....) && !foo(....) & arg)Take a close look at the last subexpression:
!VertInfluencedByActiveBone(Owner, SourceComponent, VertIndex[2])
&BoneIndex3This parameter with the default value has messed things up: but for this value, the code would have never compiled at all. But since it's there, the code compiles successfully and the bug blends in as successfully. It's this suspicious fragment that the analyzer spotted – the infix operation '&' with the left operand of type bool and the right operand of type int32.
I hope I have convinced you that static analysis is a very useful tool when developing video games and game engines, and one more option to help you improve the quality of your code (and thus of the final product). If you are a video game industry developer, you ought to tell your coworkers about static analysis and refer them to this article. Wondering where to start? Start with PVS-Studio.