Ищем ошибки в игровом движке Xenko
Движков с открытым исходным кодом, написанных на C++, куда больше, чем аналогичных движков, написанных на C#. Но есть исключения. Xenko — один из движков, написанных на C# и имеющих открытый исходный код. О том, что же интересного удалось найти в коде этого движка, будет рассказано в этой статье.
Анализируемый проект
Xenko (ранее известный как Paradox) — кроссплатформенный игровой движок, позволяющий разрабатывать игры, используя для этого язык программирования C#. Движок позволяет разрабатывать как 2D, так и 3D-игры под разные платформы: Android, iOS, Windows Desktop, Windows Phone, PlayStation 4. В будущем также планируется поддержка MacOSX и Linux. Исходный код движка доступен в репозитории на GitHub. Большая часть кода (89% по информации с GitHub), написана на C#.
Инструмент анализа
Проект проверялся с помощью анализатора PVS-Studio. Помимо уже привычных ошибок (вроде V3001), в нём обнаружились подозрительные фрагменты кода, диагностируемые правилами из последнего релиза анализатора.
Все диагностические сообщения содержат документацию, в которой приводятся примеры ошибок, их описание, а также способы их исправления. Загрузить последнюю версию анализатора можно по ссылке.
Дабы не быть голословным, предлагаю посмотреть, что же нашлось интересного.
Подозрительные фрагменты кода
Часто ошибки ведут к более серьёзным последствиям, чем кажется на первый взгляд. Для того, чтобы лучше понять их суть и способы исправления, рекомендую ознакомиться с документацией к диагностическим правилам.
public bool CanHandleRequest(TexImage image, IRequest request)
{
....
return SupportFormat(compress.Format) &&
SupportFormat(image.Format);
....
return SupportFormat(converting.Format) &&
SupportFormat(converting.Format); //<=
....
}
Предупреждение PVS-Studio: V3001 There are identical sub-expressions 'SupportFormat (converting.Format)' to the left and to the right of the '&&' operator. SiliconStudio.TextureConverter DxtTexLib.cs 141
Часто люди думают: «Ну, проверили два раза условие, ничего страшного в этом нет». На первый взгляд — да, и порой это действительно так. Но чаще проблема заключается в ином — условие перепутали, следовательно, допущена логическая ошибка и логика программы не соответствует задуманной. Так же и здесь. Два раза выполняется проверка подусловия за счёт вызова метода 'SupportFormat (converting.Format)', но скорее всего во втором случае подразумевался вызов следующего вида: 'SupportFormat (image.Format)'. Тогда всё выражение примет вид:
return SupportFormat(converting.Format) &&
SupportFormat(image.Format);
Схожая ошибка (при чём в этом же методе):
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: 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
В коде, приведённом в статье, ошибку видно невооружённым глазом. А вот в файле с этим кодом она, мягко говоря, не бросается в глаза. Отчасти в этом «заслуга» форматирования — в файле это выражение записано в одну строку, так что дублирование подвыражений без детального просмотра кода можно и не заметить. Думаю, подразумевался другой элемент перечисления, например, 'BSpline'.
Вообще в больших выражениях достаточно легко допустить подобную ошибку, как это будет видно на следующем примере. Попробуйте найти ошибку самостоятельно, не читая предупреждения анализатора и пояснений к коду:
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: 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
Разобраться в таком коде явно непросто… Давайте попробуем упростить выражение, заменив подвыражение простыми буквами (опустив при этом скобки). Тогда получится следующий код:
if (A && B && C && D && E && F && G && H && C)
Несмотря на то, что количество подвыражений по-прежнему впечатляет, ошибку заметить проще. В коде два раза проверяется подвыражение 'C', соответствующее 'box.Maximum.X — box.Minimum.X > sphere.Radius'. Если внимательно посмотреть на исходное выражение, можно понять, что на самом деле вместо данного подвыражения должно находиться следующее:
box.Maximum.Z - box.Minimum.Z > sphere.Radius
Идём дальше:
....
///
/// key is null.
public bool Remove(KeyValuePair item)
{
if (item.Key == null ||
item.Key == null)
throw new ArgumentException();
....
}
Предупреждение PVS-Studio: V3001 There are identical sub-expressions 'item.Key == null' to the left and to the right of the '||' operator. SiliconStudio.Core MultiValueSortedDictionary.cs 318
Как минимум — условие выглядит странно. Можно было бы предположить, что здесь тоже подразумевалось другое подвыражение, но судя по комментарию — нет. Выходит, это опечатка, хотя не совсем понятно, как её можно было допустить. В любом случае, код необходимо поправить.
Нередко ошибаются и в присваиваниях, когда выполняется присваивание объекта самому себе. В подобных случаях, глядя со стороны, бывает сложно сказать, как правильно исправить код. Посмотрим на несколько таких мест:
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: V3005 The 'hashCode' variable is assigned to itself. SiliconStudio.Xenko ParameterKeys.cs 346
Поле 'hashCode' присваивается само себе. Как минимум — это лишнее присваивание, но скорее всего в методе хеширования допущена ошибка. Видится несколько вариантов исправления:
- Убрать лишнее присваивание;
- Заменить первое присваивание на подвыражение, аналогичное стоящим ниже (hashCode * 397);
- Возможно, стоит также вызвать метод 'GetHashCode ()' у свойства 'Indexer'.
Как правильно поступить в данном случае — решать автору кода.
В коде встречались выражения, значение которых всегда было истинным или ложным. Подобные случаи выявляет диагностика V3022, и дальше будут приведены некоторые фрагменты кода, которые удалось обнаружить с её помощью.
private void SetTime(CompressedTimeSpan timeSpan)
{
....
while (....)
{
var moveNextFrame = currentKeyFrame.MoveNext();
if (!moveNextFrame)
{
....
break;
}
var keyFrame = moveNextFrame ? currentKeyFrame.Current :
data.ValueNext;
....
}
....
}
Предупреждение PVS-Studio: V3022 Expression 'moveNextFrame' is always true. SiliconStudio.Xenko.Engine AnimationChannel.cs 314
В тернарном операторе переменная 'moveNextFrame' всегда будет иметь значение 'true'. В ином случае будет осуществлён выход из цикла до выполнения рассматриваемого оператора. Таким образом, если выполнение всё же дойдёт до тернарного оператора, то объект 'keyFrame' всегда будет иметь одно и то же значение — 'currentKeyFrame.Current'.
Предупреждения для фрагментов кода, в которых прослеживается схожая ситуация:
- 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
Продолжим обзор:
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: V3023 Consider inspecting this expression. The expression is excessive or contains a misprint. SiliconStudio.Assets Diff3Node.cs 70
Данное выражение или избыточно, или содержит ошибку. Если истинно первое подвыражение, то второе подвыражение также всегда будет истинным (впрочем, до его вычисления выполнение не дойдёт). Данное выражение можно упростить до 'diff3.ChangeType!= Diff3ChangeType.None'. Скорее всего, в рассматриваемом случае реализована просто излишняя проверка, хотя в некоторых случаях это может свидетельствовать об иной ошибке — когда проверяется не та переменная. Подробнее об этом написано в документации к диагностике.
Нашлась пара интересных мест со строками форматирования:
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: 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
Параметры строки форматирования индексируются с {0}, здесь же индексация начинается с {1}. В данном случае строка форматирования ожидает 4 аргумента, но представлены только 3, из-за чего будет сгенерировано исключение типа 'FormatException'. Для исправления данной ошибки необходимо правильно пронумеровать индексы в строке форматирования.
"Red:{0} Green:{1} Blue:{2}"
Другой пример:
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: 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
С точностью противоположная ситуация — строка форматирования требует наличия 1 аргумента, однако метод содержит 2 — 'item' и 'text'. В данном случае ненужный аргумент попросту будет проигнорирован, но такой код должен навести на подозрения. В лучшем случае второй аргумент просто является лишним и его можно удалить, в худшем — строка форматирования составлена ошибочно.
private bool requestedExit;
public void MainLoop(IGameDebuggerHost gameDebuggerHost)
{
....
while (!requestedExit)
{
Thread.Sleep(10);
}
}
Предупреждение PVS-Studio: 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
Этот цикл ожидает некого события извне и должен выполняться до тех пор, пока переменная 'requestedExit' имеет значение 'false'. Однако после оптимизации данный цикл может превратиться в бесконечный из-за того, что компилятор может закешировать значение переменной 'requestedExit'. Данные ошибки достаточно трудно отлавливаемы, так как именно из-за кеширования, проводимого при оптимизации, поведение программы 'Debug' и 'Release' версиях может отличаться. Для исправления ошибки можно добавить модификатор 'volatile' при объявлении поля или же использовать специализированные средства синхронизации. Подробнее об этом можно прочитать в документации к данной диагностике.
Следующий странный фрагмент кода:
private void QuickSort(List list, int left, int right)
{
int i = left;
int j = right;
double pivotValue = ((left + right) / 2);
int x = list[(int)pivotValue].DataSize;
....
}
Предупреждение PVS-Studio: 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
Сразу хочу сказать, что переменная 'pivotValue', кроме как в приведённом фрагменте нигде не используется. Данная переменная имеет тип 'double', однако при её инициализации будет производится целочисленное деление, так как типы всех переменных, участвующих в инициализирующем выражении — целочисленные. Более того, далее выполняется обратное приведение данной переменной обратно к типу 'int'. Таким образом, можно было бы сразу использовать тип 'int' при объявлении переменной 'pivotValue', или же использовать инициализирующее выражение для вычисления индекса массива. Так или иначе, код выглядит странно и его стоит упростить.
Следующее предупреждение связано с подсистемой WPF:
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: 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
При регистрации свойства зависимости было указано, что свойство хранит значение типа 'object'. Таким образом, в данное свойство может быть записано значение любого типа, однако при попытке обращения к нему возможно возникновение исключения в случае, если тип объекта, записанного в свойство, невозможно привести к типу 'Key'. О том, что при регистрации свойства в качестве хранимого типа необходимо задать 'Key' свидетельствует и то, что в качестве значения по умолчанию для свойства указано значение 'Key.Enter'.
Новые диагностические правила
Как я упоминал в начале статьи, в проекте удалось обнаружить места, выявленные новыми диагностическими сообщениями, добавленными в последнем релизе PVS-Studio. Обзор некоторых таких мест приведён ниже.
Встретилось несколько фрагментов кода, в которых один из параметров метода перезаписывался, но перед этим его значение никак не использовалось. Таким образом, значение, пришедшее в метод, попросту теряется:
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: V3061 Parameter 'internalEvent' is always rewritten in method body before being used. SiliconStudio.Xenko ParameterCollection.cs 1158
Данный код выглядит странно, так как объект 'internalEvent' нигде не используется, сразу же перезаписывается, после чего возвращается из метода. Тогда из сигнатуры метода вообще можно было бы убрать этот параметр, а тело метода упростить до следующего вида:
return (internalValue, oldValue) =>
originalEvent(key, internalValue, oldValue);
Но возможно, что ошибка более интересна и на самом деле данный метод предназначался для создания цепочки делегатов. В таком случае решением проблемы будет исправление знака '=' на '+='.
Встретились ещё 2 случая с перезаписью параметра:
private void Load(TexImage image, DxtTextureLibraryData libraryData,
LoadingRequest loader)
{
....
libraryData = new DxtTextureLibraryData(); //<=
image.LibraryData[this] = libraryData;
libraryData.Image = new ScratchImage();
....
}
Предупреждение PVS-Studio: V3061 Parameter 'libraryData' is always rewritten in method body before being used. SiliconStudio.TextureConverter DxtTexLib.cs 213
Параметр 'libraryData' перезаписывается до того, как его значение где-то используется. При этом он не отмечен модификаторами 'ref' или 'out'. Это очень странно, так как значение, принимаемое методом, попросту теряется.
Предупреждение, выданное на схожий код: V3061 Parameter 'libraryData' is always rewritten in method body before being used. SiliconStudio.TextureConverter FITexLib.cs 244
Противоположная ситуация — метод принимает аргумент, значение которого не используется:
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: V3065 Parameter 'height' is not utilized inside method’s body. SiliconStudio.Xenko Image.cs 473
Как видно из предупреждения анализатора, параметр 'height' нигде не используется. Вместо него в метод 'CreateDescription' дважды передаётся параметр 'width', что может свидетельствовать о наличии ошибки. Правильный вызов метода 'CreateDescription' может выглядеть так:
CreateDescription(TextureDimension.Texture3D,
width, height, depth, mipMapCount, format, 1)
Заключение
Было интересно проверить игровой движок, написанный на C#. Все допускают ошибки, и различные инструменты позволяют минимизировать их количество, и статический анализатор — один из таких инструментов. И чем раньше будет найдена ошибка, тем дешевле обойдётся её исправление.
Конечно, не все ошибки, найденные в проекте, были выписаны в статье. Во-первых, это сильно бы увеличило размер статьи, во-вторых — некоторые из диагностических сообщений являются специфичными, т.е. актуальными для определённых типов проектов, поэтому могут быть интересны не всем. Но, безусловно, разработчикам (а возможно и просто любопытным программистам) будет интересно посмотреть все подозрительные места, найденные анализатором. Это можно сделать, загрузив пробную версию анализатора.
Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Sergey Vasiliev. Catching Errors in the Xenko Game Engine.
