Open-source engines in C++ are much more common than those written in C#. But there are exceptions: Xenko is one such engine, open-source and written in C#. In this article, you will learn about interesting issues we managed to find in this project.
Xenko (formerly known as Paradox) is a cross-platform game engine for game development in C#. The engine allows developing both 2D and 3D games for various platforms: Android, iOS, Windows Desktop, Windows Phone, PlayStation 4. The developers also plan on adding support for macOS X and Linux in future versions. The engine's source code can be downloaded from the GitHub repository. Most of the code (89% according to GitHub) is written in C#.
The project was scanned with the PVS-Studio analyzer. Besides familiar errors (like V3001), a number of suspicious code fragments were found by new diagnostics added in the latest version.
Every diagnostic message contains documentation, where you can find a description and examples of the error and ways of fixing it. The latest version of the analyzer can be downloaded here.
To support my words, I'm discussing some interesting issues found in this project further in the article.
Errors often have more serious implications than it may seem at first. To get a clearer idea of what they are about and how to fix them, I recommend referring to the documentation on the diagnostic rules.
public bool CanHandleRequest(TexImage image, IRequest request)
{
....
return SupportFormat(compress.Format) &&
SupportFormat(image.Format);
....
return SupportFormat(converting.Format) &&
SupportFormat(converting.Format); // <=
....
}PVS-Studio warning: V3001 There are identical sub-expressions 'SupportFormat(converting.Format)' to the left and to the right of the '&&' operator. SiliconStudio.TextureConverter DxtTexLib.cs 141
A common reaction is, "Well, it's just that a condition is checked two times, there's nothing bad about it." Yes, errors like this aren't always that bad. But more often, they imply quite a different issue: checking a wrong condition leads to a logical error and, therefore, changes the program's logic. It's just the case with the code above. A subcondition is checked twice by calling to method 'SupportFormat(converting.Format)', but the second check is more likely to contain the following call: 'SupportFormat(image.Format)'. Then, the whole expression will look like this:
return SupportFormat(converting.Format) &&
SupportFormat(image.Format);A similar error (in the same method, by the way):
public enum Rescaling
{
Box = 0,
Bicubic = 1,
Bilinear = 2,
BSpline = 3,
CatmullRom = 4,
Lanczos3 = 5,
Nearest,
}
public bool CanHandleRequest(TexImage image, IRequest request)
{
....
return rescale.Filter == Filter.Rescaling.Box ||
rescale.Filter == Filter.Rescaling.Bicubic || // <=
rescale.Filter == Filter.Rescaling.Bicubic || // <=
rescale.Filter == Filter.Rescaling.Nearest;
....
}PVS-Studio warning: V3001 There are identical sub-expressions 'rescale.Filter == Filter.Rescaling.Bicubic' to the left and to the right of the '||' operator. SiliconStudio.TextureConverter DxtTexLib.cs 148
The way this code is presented here, the error can be easily spotted. But when looking through the original source file, it doesn't strike your eye, to say the least. Partly it's "thanks" to the formatting: this expression is written in one line, so duplicate subexpressions are difficult to notice without close reading. My guess is that the programmer really meant to use a different enumeration member - for example 'BSpline'.
Generally speaking, it's very easy to make a mistake like that in large expressions, as demonstrated by the following example. Try to find the error by yourself, without reading the analyzer warning and my comments after the example:
public static ContainmentType BoxContainsSphere(
ref BoundingBox box,
ref BoundingSphere sphere)
{
....
if ((((box.Minimum.X + sphere.Radius <= sphere.Center.X) &&
(sphere.Center.X <= box.Maximum.X - sphere.Radius)) &&
((box.Maximum.X - box.Minimum.X > sphere.Radius) &&
(box.Minimum.Y + sphere.Radius <= sphere.Center.Y))) &&
(((sphere.Center.Y <= box.Maximum.Y - sphere.Radius) &&
(box.Maximum.Y - box.Minimum.Y > sphere.Radius)) &&
(((box.Minimum.Z + sphere.Radius <= sphere.Center.Z) &&
(sphere.Center.Z <= box.Maximum.Z - sphere.Radius)) &&
(box.Maximum.X - box.Minimum.X > sphere.Radius))))
....
}PVS-Studio warning: V3001 There are identical sub-expressions 'box.Maximum.X - box.Minimum.X > sphere.Radius' to the left and to the right of the '&&' operator. SiliconStudio.Core.Mathematics Collision.cs 1322
It's not easy to figure this code out, is it? Let's try to simplify the expression by replacing the subexpressions with simple letters (and omitting the parentheses). We'll get the following code:
if (A && B && C && D && E && F && G && H && C)Though the number of subexpressions is still impressive, the error has become much more visible. The 'C' subexpression, which stands for 'box.Maximum.X - box.Minimum.X > sphere.Radius', is checked twice. If you look close at the original expression, you'll see that the following subexpression must be used instead:
box.Maximum.Z - box.Minimum.Z > sphere.RadiusMoving on:
....
/// <exception cref="System.ArgumentNullException">
/// key is null.</exception>
public bool Remove(KeyValuePair<TKey, Tvalue> item)
{
if (item.Key == null ||
item.Key == null)
throw new ArgumentException();
....
}PVS-Studio warning: V3001 There are identical sub-expressions 'item.Key == null' to the left and to the right of the '||' operator. SiliconStudio.Core MultiValueSortedDictionary.cs 318
This condition looks strange, to say the least. We could assume that there must be a different expression, too, but this assumption would contradict the comment. So, this error turns out to be a typo, though it's not quite clear how one could make it. Anyway, the code has to be fixed.
Programmers often make mistakes in assignments as well, assigning objects to themselves. In such cases, you can't say for sure how to fix the code if you aren't the author. Here are a few examples:
public ParameterComposedKey(ParameterKey key, string name,
int indexer)
{
Key = key;
Name = name;
Indexer = indexer;
unchecked
{
hashCode = hashCode = Key.GetHashCode();
hashCode = (hashCode * 397) ^ Name.GetHashCode();
hashCode = (hashCode * 397) ^ Indexer;
}
}PVS-Studio warning: V3005 The 'hashCode' variable is assigned to itself. SiliconStudio.Xenko ParameterKeys.cs 346
The 'hashCode' field is assigned to itself. It's an extra assignment, to say the least, but what looks more likely is that there is a mistake in the hashing method. There are a few ways to fix it:
Which option is the right one is up to the code's author to decide.
The code contains a few expressions which always evaluate either to true or false. Such issues are detected by the V3022 diagnostic, and what follows are code fragments found using this diagnostic.
private void SetTime(CompressedTimeSpan timeSpan)
{
....
while (....)
{
var moveNextFrame = currentKeyFrame.MoveNext();
if (!moveNextFrame)
{
....
break;
}
var keyFrame = moveNextFrame ? currentKeyFrame.Current :
data.ValueNext;
....
}
....
}PVS-Studio warning: V3022 Expression 'moveNextFrame' is always true. SiliconStudio.Xenko.Engine AnimationChannel.cs 314
In the ternary operator, the 'moveNextFrame' variable will always refer to 'true'. Otherwise, the loop will be exited before the operator is executed. So, if the flow of execution does reach it, the 'keyFrame' object will always refer to the same value, 'currentKeyFrame.Current'.
Other similar warnings:
The next issue:
public enum Diff3ChangeType
{
None,
Children,
MergeFromAsset1,
MergeFromAsset2,
MergeFromAsset1And2,
Conflict,
ConflictType,
ConflictArraySize,
InvalidNodeType,
}
private static bool CheckVisitChildren(Diff3Node diff3)
{
return diff3.ChangeType == Diff3ChangeType.Children ||
diff3.ChangeType != Diff3ChangeType.None;
}PVS-Studio warning: V3023 Consider inspecting this expression. The expression is excessive or contains a misprint. SiliconStudio.Assets Diff3Node.cs 70
This expression is either redundant or incorrect. If the first subexpression is true, the second will always be true as well (though it will never be evaluated). The expression can be reduced to 'diff3.ChangeType != Diff3ChangeType.None'. What's more likely is that we are dealing with just an extra check, although in certain cases it may indicate a different kind of error - checking a wrong variable. See the details in the documentation for this diagnostic.
There were also a couple of interesting fragments with format strings:
public string ToString(string format, IFormatProvider formatProvider)
{
if (format == null)
return ToString(formatProvider);
return string.Format(formatProvider,
"Red:{1} Green:{2} Blue:{3}",
R.ToString(format, formatProvider),
G.ToString(format, formatProvider),
B.ToString(format, formatProvider));
}PVS-Studio warning: V3025 Incorrect format. A different number of format items is expected while calling 'Format' function. Expected: 4. Present: 3. SiliconStudio.Core.Mathematics Color3.cs 765
Format-string parameters are normally indexed starting with {0}, but here indexing starts with {1}. In this code, the format string is expecting 4 arguments but gets only 3, which issue will result in a 'FormatException'. To fix this error, indices in the format string must be numbered correctly.
"Red:{0} Green:{1} Blue:{2}"Another example:
public static bool IsValidNamespace(string text, out string error)
{
....
error = items.Where(s => !IsIdentifier(s))
.Select(item => string.Format("[{0}]", item, text))
.FirstOrDefault();
....
}PVS-Studio warning: V3025 Incorrect format. A different number of format items is expected while calling 'Format' function. Expected: 1. Present: 2. SiliconStudio.Core.Design NamingHelper.cs 56
This one is an opposite issue: a format string requires 1 argument while the method has 2 arguments, 'item' and 'text'. In this case, the extra argument will be simply ignored, but code like that inevitably raises certain suspicions. At best, the second argument is just an extra one and can be safely deleted; at worst, the format string was formed with mistakes.
private bool requestedExit;
public void MainLoop(IGameDebuggerHost gameDebuggerHost)
{
....
while (!requestedExit)
{
Thread.Sleep(10);
}
}PVS-Studio warning: V3032 Waiting on this expression is unreliable, as compiler may optimize some of the variables. Use volatile variable(s) or synchronization primitives to avoid this. SiliconStudio.Xenko.Debugger GameDebuggerTarget.cs 225
This loop is expecting some event from outside and must keep running as long as the 'requestedExit' variable has value 'false'. However, this loop may become an infinite one as the compiler may optimize it by caching the value of the 'requestedExit' variable. Errors like this are pretty hard to catch as the program behavior may differ very much in 'Debug' and 'Release' modes because of that very optimization-driven caching. To fix it, we need to add the 'volatile' modifier to the field declaration or use special synching techniques. See the documentation on this diagnostic for details.
The next code fragment:
private void QuickSort(List<TexImage> list, int left, int right)
{
int i = left;
int j = right;
double pivotValue = ((left + right) / 2);
int x = list[(int)pivotValue].DataSize;
....
}PVS-Studio warning: V3041 The expression was implicitly cast from 'int' type to 'double' type. Consider utilizing an explicit type cast to avoid the loss of a fractional part. An example: double A = (double)(X) / Y;. SiliconStudio.TextureConverter AtlasTexLibrary.cs 422
It has to be said right off that the 'pivotValue' variable is not used anywhere except the fragment above. This variable is of type 'double'; however, an integer-division operation will be performed during its initialization since the types of all the variables participating in the initializing expression are integer. Furthermore, this variable is then cast back to type 'int'. So, the 'pivotValue' could as well be declared as of type 'int' right from the start, or the initializing expression could be used to evaluate the array's index. One way or another, the code looks strange and should be simplified.
The next warning deals with the WPF subsystem:
public static readonly DependencyProperty KeyProperty =
DependencyProperty.Register("Key",
typeof(object),
typeof(TextBoxKeyUpCommandBehavior),
new PropertyMetadata(Key.Enter));
public Key Key {
get { return (Key)GetValue(KeyProperty); }
set { SetValue(KeyProperty, value); }
}PVS-Studio warning: V3046 WPF: the type registered for DependencyProperty does not correspond with the type of the property used to access it. SiliconStudio.Presentation TextBoxKeyUpCommandBehavior.cs 18
When registering a dependency property, the programmer specified that the property should store a value of type 'object'. That is, this property can store a value of any type, but attempting to address it may cause an exception if the object written into the property can't be cast to type 'Key'. The requirement to set 'Key' as the type of the value stored in the property when registering it is supported by the fact that 'Key.Enter' is set as a default value for this property.
As I mentioned in the beginning, the analyzer found some fragments in this code using new diagnostic rules, which were added in the PVS-Studio's latest version. What follows is an overview of some of those fragments.
Some examples deal with overwriting a method parameter although its value had not been used before. That is, the value passed into the method simply gets lost:
internal delegate void InternalValueChangedDelegate(
InternalValue internalValue, object oldValue);
private static InternalValueChangedDelegate
CreateInternalValueChangedEvent(
ParameterKey key,
InternalValueChangedDelegate internalEvent,
ValueChangedDelegate originalEvent)
{
internalEvent = (internalValue, oldValue) =>
originalEvent(key, internalValue, oldValue);
return internalEvent;
}PVS-Studio warning: V3061 Parameter 'internalEvent' is always rewritten in method body before being used. SiliconStudio.Xenko ParameterCollection.cs 1158
This code looks strange because the 'internalEvent' object is not used anywhere, is overwritten right off, and is then returned from the method. That way, it would be better to remove this parameter from the method signature and simplify the method body to the following code:
return (internalValue, oldValue) =>
originalEvent(key, internalValue, oldValue);But this error may be more tricky and interesting if this method was really meant for creating a delegate chain. If it is the case, the issue can be solved by changing the '=' sign to '+='.
There were two more cases of parameter overwriting:
private void Load(TexImage image, DxtTextureLibraryData libraryData,
LoadingRequest loader)
{
....
libraryData = new DxtTextureLibraryData(); // <=
image.LibraryData[this] = libraryData;
libraryData.Image = new ScratchImage();
....
}PVS-Studio warning: V3061 Parameter 'libraryData' is always rewritten in method body before being used. SiliconStudio.TextureConverter DxtTexLib.cs 213
The 'libraryData' parameter is overwritten before its value is used anywhere. At the same time, it doesn't have modifier 'ref' or 'out'. It looks strange, as the value received by the method simply gets lost.
One more similar warning: V3061 Parameter 'libraryData' is always rewritten in method body before being used. SiliconStudio.TextureConverter FITexLib.cs 244
And here's an opposite situation: a method receives an argument whose value is not used:
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. SiliconStudio.Xenko Image.cs 473
As the warning says, the 'height' parameter is not used anywhere. Instead, parameter 'width' is passed twice to the 'CreateDescription' method, and it may be a sign of an error. A correct call to the 'CreateDescription' method should look something like this:
CreateDescription(TextureDimension.Texture3D,
width, height, depth, mipMapCount, format, 1)It was an interesting experience to analyze a game engine written in C#. Everyone makes mistakes, and there are various tools designed to minimize their number, static analyzer being one of these tools. Remember: the earlier an error is found, the cheaper it is to fix.
Of course, I haven't discussed all the errors found in the project. First, it would make the article too lengthy; second, some of the diagnostics are too specific, i.e. relevant only for certain types of projects and are not of interest to everyone. But no doubt every developer (and just inquisitive programmer) would like to see all the suspicious fragments the analyzer managed to find in this project. You can do it by downloading the trial version of the analyzer.