We are looking for errors in the game engine Xenko

Open source engines written in C ++ are much more than similar engines written in C #. But there are exceptions. Xenko is one of the engines written in C # and having open source code. That interesting managed to be found in the code of this engine, it will be told in this article.

Analyzed project

Xenko (formerly known as Paradox) is a cross-platform game engine that allows you to develop games using the C # programming language. The engine allows you to develop both 2D and 3D games for different platforms: Android, iOS, Windows Desktop, Windows Phone, PlayStation 4. In the future, support for MacOSX and Linux is also planned. The source code of the engine is available in the repository on GitHub . Most of the code (89% of the information from GitHub) is written in C #.
')

Analysis tool

The project was checked using the PVS-Studio analyzer. In addition to the usual mistakes (like V3001 ), it detected suspicious code fragments diagnosed by the rules from the latest release of the analyzer.



All diagnostic messages contain documentation , which provides examples of errors, their description, as well as ways to correct them. Download the latest version of the analyzer at the link .

In order not to be unfounded, I suggest to see what was interesting.

Suspicious code snippets

Often mistakes lead to more serious consequences than it seems at first glance. In order to better understand their essence and methods of correction, I recommend reading the documentation for 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)' operator. SiliconStudio.TextureConverter DxtTexLib.cs 141

Often people think: “Well, they checked the condition twice, there’s nothing wrong with that.” At first glance - yes, and sometimes this is true. But more often the problem lies elsewhere - the condition is mixed up, therefore, a logical error is made and the program logic does not correspond to what was intended. So it is here. The sub-condition is checked twice by calling the 'SupportFormat (converting.Format)' method, but most likely in the second case the following call is implied: 'SupportFormat (image.Format)'. Then the whole expression takes the form:

 return SupportFormat(converting.Format) && SupportFormat(image.Format); 

Similar error (for what in the same method):

 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 | operator. SiliconStudio.TextureConverter DxtTexLib.cs 148

In the code given in the article, the error can be seen with the naked eye. But in the file with this code, she, to put it mildly, is not evident. Partly in this “merit” of formatting - in the file this expression is written in one line, so that duplication of subexpressions without a detailed view of the code can not be overlooked. I think it meant another element of the listing, for example, 'BSpline'.

In general, in large expressions it is quite easy to make such a mistake, as will be seen in the following example. Try to find the error yourself without reading the analyzer warnings and code explanations:

 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' operator. SiliconStudio.Core.Mathematics Collision.cs 1322

Understanding this code is clearly not easy ... Let's try to simplify the expression by replacing the subexpressions with simple letters (omitting the brackets). Then we get the following code:

 if (A && B && C && D && E && F && G && H && C) 

Although the number of subexpressions is still impressive, the error is easier to notice. In the code, the subexpression 'C' corresponding to 'box.Maximum.X - box.Minimum.X> sphere.Radius' is checked twice. If you look closely at the original expression, you can understand that in fact, instead of this subexpression should be the following:

 box.Maximum.Z - box.Minimum.Z > sphere.Radius 

Go ahead:

 .... /// <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 '||' operator. SiliconStudio.Core MultiValueSortedDictionary.cs 318

At a minimum, the condition looks strange. One would assume that another subexpression was also implied here, but judging by the commentary — no. It turns out that this is a typo, although it is not entirely clear how it could have been allowed. In any case, the code must be corrected.

Often, errors are made in assignments when the object is assigned to itself. In such cases, looking from the side, it is difficult to say how to correct the code. Look at a few places like this:

 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. At a minimum, this is an extra assignment, but most likely an error has been made in the hashing method. There are several fixes:

1. Remove excess assignment;
2. Replace the first assignment with a subexpression similar to the ones below (hashCode * 397);
3. Perhaps you should also call the 'GetHashCode ()' method on the 'Indexer' property.

How to act in this case is up to the author of the code.

In the code there were expressions, the meaning of which was always true or false. Such cases are revealed by the diagnostics of V3022 , and further some fragments of code that could be detected with its help will be given.

 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 variable 'moveNextFrame' will always have the value 'true'. Otherwise, it will exit the loop before executing the considered operator. Thus, if the execution still reaches the ternary operator, then the 'keyFrame' object will always have the same value - 'currentKeyFrame.Current'.

Warnings for code snippets that show a similar situation:

• V3022 Expression 'inputTexture.Dimension == TextureDimension.TextureCube' is always true. SiliconStudio.Xenko.Engine LambertianPrefilteringNoCompute.cs 66
• V3022 Expression 'inputTexture.Dimension == TextureDimension.TextureCube' is always true. SiliconStudio.Xenko.Engine LambertianPrefilteringSH.cs 72

Continue the review:

 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 misprint. SiliconStudio.Assets Diff3Node.cs 70

This expression is either redundant or contains an error. If the first subexpression is true, then the second subexpression will also always be true (however, its execution will not reach its calculation). This expression can be simplified to diff3.ChangeType! = Diff3ChangeType.None. Most likely, in this case, an unnecessary check is implemented, although in some cases this may indicate a different error - when the wrong variable is checked. Read more about this in the documentation for diagnostics.

Found a couple of interesting places with formatting 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 formatted items is expected while calling 'Format' function. Expected: 4. Present: 3. SiliconStudio.Core.Mathematics Color3.cs 765

The format string parameters are indexed with {0}, here indexing starts with {1}. In this case, the format string expects 4 arguments, but only 3 are presented, which is why an exception of the type 'FormatException' will be generated. To correct this error, it is necessary to number the indexes in the format string 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 formatted items is expected while calling 'Format' function. Expected: 1. Present: 2. SiliconStudio.Core.Design NamingHelper.cs 56

The situation is exactly the opposite - the format string requires 1 argument, but the method contains 2 - 'item' and 'text'. In this case, the unnecessary argument will simply be ignored, but such code should lead to suspicions. At best, the second argument is simply redundant and can be deleted; at worst, the formatting string is erroneous.

 private bool requestedExit; public void MainLoop(IGameDebuggerHost gameDebuggerHost) { .... while (!requestedExit) { Thread.Sleep(10); } } 

PVS-Studio warning : V3032 Waiting on this expression is unreliable, as the 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 waits for some event from the outside and must be executed as long as the 'requestedExit' variable is set to 'false'. However, after optimization, this cycle may turn into infinite due to the fact that the compiler can cache the value of the 'requestedExit' variable. These errors are rather difficult to catch, since it is because of the caching performed during optimization that the behavior of the program 'Debug' and 'Release' versions may differ. To correct the error, you can add the modifier 'volatile' when declaring the field or use specialized synchronization tools. Read more about this in the documentation for this diagnostic .

The following strange code snippet:

 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 using a fractional part. An example: double A = (double) (X) / Y ;. SiliconStudio.TextureConverter AtlasTexLibrary.cs 422

Just want to say that the variable 'pivotValue', except in the above fragment, is not used anywhere. This variable is of type 'double', however, when it is initialized, integer division will be performed, since the types of all variables involved in the initialization expression are integer. Moreover, the next step is to reverse this variable back to the 'int' type. Thus, one could immediately use the 'int' type when declaring the variable 'pivotValue', or use an initialization expression to calculate the index of the array. One way or another, the code looks weird and should be simplified.

The following warning is related to 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 of property registered for DependencyProperty does not use it. SiliconStudio.Presentation TextBoxKeyUpCommandBehavior.cs 18

When registering a dependency property, it was indicated that the property stores a value of type 'object'. Thus, a value of any type can be written to this property; however, when trying to access it, an exception may be thrown if the type of the object written to the property cannot be cast to the type 'Key'. The fact that when registering a property as a stored type it is necessary to set the 'Key' indicates that the property's default value is set to 'Key.Enter'.

New diagnostic rules

As I mentioned at the beginning of the article, the project managed to find the locations revealed by the new diagnostic messages added in the latest release of PVS-Studio. An overview of some of these places is given below.

There were several code fragments in which one of the method parameters was overwritten, but before that its value was not used at all. Thus, the value that came into the method is simply 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. SiliconStudio.Xenko ParameterCollection.cs 1158

This code looks strange, since the 'internalEvent' object is not used anywhere, is immediately overwritten, and then returned from the method. Then it would be possible to remove this parameter from the method signature, and simplify the method body to the following form:

 return (internalValue, oldValue) => originalEvent(key, internalValue, oldValue); 

But it is possible that the error is more interesting and in fact this method was intended to create a chain of delegates. In this case, fixing the '=' sign on '+ =' will be the solution.

2 more cases with parameter overwriting were met:

 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. SiliconStudio.TextureConverter DxtTexLib.cs 213

The 'libraryData' parameter is overwritten before its value is used somewhere. However, it is not marked with modifiers 'ref' or 'out'. This is very strange, since the value taken by the method is simply lost.

Warning issued for a similar code: V3061 Parameter 'libraryData' is always rewritten. SiliconStudio.TextureConverter FITexLib.cs 244

The opposite situation - the method takes 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 can be seen from the analyzer warning, the 'height' parameter is not used anywhere. Instead, the 'width' parameter is passed to the 'CreateDescription' method twice, which may indicate an error. The correct call to the 'CreateDescription' method might look like this:

 CreateDescription(TextureDimension.Texture3D, width, height, depth, mipMapCount, format, 1) 

Conclusion

It was interesting to check out the game engine written in C #. Everyone makes mistakes, and various tools minimize their number, and the static analyzer is one of these tools. And the sooner the error is found, the cheaper it will cost to fix it.

Of course, not all the errors found in the project were written out in the article. First, it would greatly increase the size of the article; second, some of the diagnostic messages are specific, i.e. relevant for certain types of projects, so it may not be of interest to everyone. But, of course, developers (and perhaps just curious programmers) will be interested to see all the suspicious places found by the analyzer. This can be done by downloading a trial version of the analyzer .



If you want to share this article with an English-speaking audience, then please use the link to the translation: Sergey Vasiliev. Catching Errors in the Xenko Game Engine .