Шукаємо помилки в ігровому движку 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

Йдемо далі:
....
/// <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: 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;

безконтрольно
{
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' присвоюється сама собі. Як мінімум — це зайве присвоювання, але швидше за все в методі хешування допущена помилка. Бачиться кілька варіантів виправлення:
  1. Прибрати зайве присвоювання;
  2. Замінити перше призначення на підвираз, аналогічне стоять нижче (hashCode * 397);
  3. Можливо, варто також викликати метод '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(format string, 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 format of 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 format of 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<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: 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 for registered 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 method in 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 method in body before being used. SiliconStudio.TextureConverter DxtTexLib.cs 213

Параметр 'libraryData' перезаписується до того, як його значення де використовується. При цьому він не відзначений модифікаторами 'ref' або 'out'. Це дуже дивно, так як значення, що приймається методом, просто втрачається.

Попередження, видане на схожий код: V3061 Parameter 'libraryData' is always rewritten method in 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.

Прочитали статтю і є питання?Часто до наших статей задають одні і ті ж питання. Відповіді на них ми зібрали тут: Відповіді на питання читачів статей про PVS-Studio, версія 2015. Будь ласка, ознайомтеся зі списком.


Джерело: Хабрахабр

0 коментарів

Тільки зареєстровані та авторизовані користувачі можуть залишати коментарі.