Прежде чем перейти к статье, хочу вам представить, экономическую онлайн игру Brave Knights, в которой вы можете играть и зарабатывать. Регистируйтесь, играйте и зарабатывайте!
В мире много хороших игр, но совсем немногие открывают исходный код. В этой статье мы познакомимся, на мой взгляд, с тремя лучшими Open Source играми на C# и разберём наиболее интересные ошибки, найденные в их исходном коде.
Введение
Все игры, приведённые в топе, можно скачать в Steam и поиграть. Топ составлялся на основе следующих критериев:
оригинальность идеи;
увлекательность геймплея;
популярность игры.
Раз уж игры, приведённые в топе, являются Open Source проектами, почему бы не уделить внимания и их исходному коду? Воспользовавшись анализатором PVS-Studio, я нашёл в этих проектах несколько ошибок, обзор которых также приведу в статье. Разбирать чужие ошибки может быть полезно, ведь так вы уменьшаете риск допустить похожие в собственном коде :)
Готовы? Тогда приступим!
3 место. Thrive
Обзор игры
Thrive — игра-симулятор эволюции. По задумке разработчиков, игровой процесс должен состоять из 7 этапов на протяжении всего развития вида от прокариотической клетки до освоения космоса. Однако игра всё ещё находится в разработке, и на данный момент из всех этапов доступен только первый — этап микроорганизма.
Идея игры напоминает Spore, однако в Thrive делается упор на реализм.
Здесь для успешной эволюции и выживания вам предстоит:
соблюдать баланс различных веществ в вашем организме;
с умом подходить к развитию организма, учитывая его энергетические возможности и особенности среды обитания;
защищаться от хищных организмов или самому стать хищником.
В противном случае вы рискуете обречь свой вид... на вымирание.
Разбор ошибок
Исследование проводилось на релизе Thrive 0.6.2.
Thrive — небольшой проект с хорошо отлаженным кодом. С помощью анализатора я нашёл только одну небольшую ошибку, которую мы рассмотрим далее.
Выражение всегда ложно
private void SpawnMicrobe(Vector3 position)
{
....
cloudSystem!.AddCloud( random.Next(0, 1) == 1 // <=
? phosphates
: ammonia,
....);
}Сообщение анализатора:
V3022. Expression 'random.Next(0, 1) == 1' is always false. MicrobeBenchmark.cs 520.
Обратите внимание на метод random.Next(0, 1). Он возвращает случайное целое число в заданном диапазоне. Диапазон задаётся с помощью аргументов: первый аргумент — это левая граница диапазона, а второй аргумент — правая. Однако у этого метода есть нюанс, который упустил разработчик: если левая граница входит в заданный диапазон, то правая граница — нет. Таким образом, метод random.Next(0, 1) может вернуть только одно значение — 0.
Чтобы этот код работал, как ожидалось, нужно просто увеличить правую границу до 2:
cloudSystem!.AddCloud(random.Next(0, 2) == 1 ? ....);2 место. Barotrauma
Barotrauma — многопользовательский космический 2D-симулятор подлодки в жанрах survival horror и RPG. Действие игры происходит в океане одного из спутников Юпитера.
Цель игры — добраться до ближайшего населённого пункта, совместно разрешая различные кризисные ситуации во время плавания.
В игре вам предстоит выбрать одну из ролей: капитан, инженер, механик, сотрудник службы безопасности, врач или помощник.
Роли помогают распределить обязанности между игроками в начале игры, а также определяют свойственные вашему персонажу таланты. Однако они не ограничивают задачи, которые вы можете выполнять. Так, медику никто не запрещает учиться управлению кораблём или ассистенту отстреливаться из турелей от ужасов, обитающих в глубинах океана.
Залог победы в Barotrauma — это командная работа и взаимовыручка, без которых выжить в мрачном мире этой игры просто невозможно.
Разбор ошибок
Исследование проводилось на релизе Barotrauma 0.17.12.0.
Этот проект крупнее предыдущего по количеству кода, а потому и в плане ошибок он гораздо интереснее.
Кстати, мы уже не первый раз разбираем ошибки этого проекта. Если вам интересно, вы можете ознакомиться с предыдущей статьёй на эту тему.
Бесполезная проверка на null
public class Contact
{
....
public Fixture FixtureA { get; internal set; }
public Fixture FixtureB { get; internal set; }
....
internal void Update(ContactManager contactManager)
{
Body bodyA = FixtureA.Body;
Body bodyB = FixtureB.Body;
if (FixtureA == null || FixtureB == null) // <=
return;
....
if (sensor)
{
Shape shapeA = FixtureA.Shape;
Shape shapeB = FixtureB.Shape;
touching = Collision.Collision.TestOverlap(....,
ref bodyA._xf,
ref bodyB._xf);
....
}
}
}Сообщения анализатора:
V3095. The 'FixtureA' object was used before it was verified against null. Check lines: 252, 255. Contact.cs 252.
V3095. The 'FixtureB' object was used before it was verified against null. Check lines: 253, 255. Contact.cs 253.
В приведённом коде есть проверка FixtureA == null || FixtureB == null, которая указывает, что FixtureA и FixtureB потенциально могут иметь значение null. Но в этом случае она ничем не поможет, ведь исключение будет выброшено ещё раньше:
Body bodyA = FixtureA.Body; // possible NullReferenceException
Body bodyB = FixtureB.Body; // possible NullReferenceException
if (FixtureA == null || FixtureB == null)
return;Можно было бы предположить, что данная проверка является избыточной и никакой угрозы NullReferenceException тут нет. Однако количество аналогичных проверок в других частях кода указывает на обратное:
Таким образом, у нас есть основания считать, что проблема реальна. Надёжнее всего в этом случае проверить на null как свойства FixtureA и FixtureB, так и свойства FixtureA.Body и FixtureB.Body следующим образом:
Body bodyA = FixtureA?.Body;
Body bodyB = FixtureB?.Body;
if (bodyA == null || bodyB == null)
return;Порядок вызовов имеет значение
public void RecreateSprites()
{
....
if (_deformSprite != null)
{
_deformSprite.Remove();
var source = _deformSprite.Sprite.SourceElement;
_deformSprite = new DeformableSprite(source, ....);
}
....
for (int i = 0; i < DecorativeSprites.Count; i++)
{
var decorativeSprite = DecorativeSprites[i];
decorativeSprite.Remove();
var source = decorativeSprite.Sprite.SourceElement; // <=
DecorativeSprites[i] = new DecorativeSprite(source, ....);
}
}Сообщение анализатора:
V3080. Possible null dereference. Consider inspecting 'decorativeSprite.Sprite'. Limb.cs 462.
В данном случае ошибка заключается в неправильном порядке выполнения операций, из-за чего в приведённом коде неизбежно будет выброшено исключение NullReferenceException. Чтобы это понять, достаточно посмотреть на реализацию методов _deformSprite.Remove и decorativeSprite.Remove:
class DecorativeSprite : ISerializableEntity
{
....
public Sprite Sprite { get; private set; }
....
public void Remove()
{
Sprite?.Remove();
Sprite = null;
}
}
partial class DeformableSprite
{
....
public Sprite Sprite { get; private set; }
....
public void Remove()
{
Sprite?.Remove();
Sprite = null;
....
}
}Оба метода присваивают свойствам Sprite значение null. Последующее разыменование этих свойств приведёт к ошибке.
Чтобы избежать исключения, нужно просто поменять вызовы методов Remove и инициализации переменных source местами:
if (_deformSprite != null)
{
var source = _deformSprite.Sprite.SourceElement; //<=
_deformSprite.Remove(); //<=
_deformSprite = new DeformableSprite(source, ....);
}
....
for (int i = 0; i < DecorativeSprites.Count; i++)
{
var decorativeSprite = DecorativeSprites[i];
var source = decorativeSprite.Sprite.SourceElement; //<=
decorativeSprite.Remove(); //<=
DecorativeSprites[i] = new DecorativeSprite(source, ....);
}Выражение всегда ложно
private void UpdateAttack(float deltaTime)
{
....
if (pathSteering != null) // <= (line 1551)
{ .... }
else
{
....
if (pathSteering != null) // <= (line 1954)
{
pathSteering.SteeringSeek(....);
}
else
{
SteeringManager.SteeringSeek(....);
}
....
}
....
}Сообщение анализатора:
V3022. Expression 'pathSteering != null' is always false. EnemyAIController.cs 1954.
За всеми '....' в реализации этого метода скрыты сотни строк кода. Такой метод просто не может быть без странностей, и статический анализатор это наглядно показал. Обратите внимание на его сообщение.
Анализатор говорит, что условие pathSteering != null всегда ложно. В этом можно убедиться, если промотать код на 403 строки вверх и увидеть, что данная проверка вложена в блок else точно такой же проверки. В результате этого код внутри блока if вложенной проверки будет недостижим.
Важно отметить, что между этими проверками значение pathSteering не изменяется. При желании вы сами можете в этом убедиться здесь.
1 место. Space Station 14
Space Station 14 — это многопользовательская ролевая онлайн-игра. Несмотря на простую 2D графику, она включает в себя множество механик и геймплей, полностью заточенный на взаимодействие между игроками и исполнение ими определённых ролей.
Действие игры разворачивается на космической станции, на которой вот-вот что-то пойдёт наперекосяк (возможно, из-за вас). Все рабочие на станции (игроки) делятся на 12 групп: командование, отдел службы безопасности, медицинский отдел, научный отдел, инженерный отдел, антагонисты (плохие ребята) и т. д.
Каждой группе соответствует от 2 до 14 ролей, всего ролей в игре — 66. Вам предстоит выбрать для себя одну из них и вместе с другими игроками трудиться во благо или во вред станции.
В игре нет ни прокачки, ни НПС, ни внутриигровых покупок. Каждый раунд игры — это уникальная отдельная история, которую вы творите вместе с другими игроками.
Разбор ошибок
Исследование проводилось на последней на момент написания статьи версии Space Station 14.
Одинаковые проверки
private void OnSmokeSpread(....)
{
if (.... || args.NeighborFreeTiles.Count == 0)
{
....
return;
}
....
if (args.NeighborFreeTiles.Count == 0 && ....) // <=
{
....
}
}Сообщение анализатора:
V3022 Expression 'args.NeighborFreeTiles.Count == 0 && args.Neighbors.Count > 0 && component.SpreadAmount > 0' is always false. SmokeSystem.cs 128
Здесь реализованы две одинаковых проверки args.NeighborFreeTiles.Count == 0. В случае, если первая проверка истинна, происходит возврат из метода. Таким образом, вторая проверка на момент её выполнения будет всегда ложна, а код в блоке соответствующего if — недостижим.
Использование неправильного логического оператора
private void OnStateChanged(...., MobStateChangedEvent args)
{
if ( args.NewMobState != MobState.Dead
|| args.NewMobState != MobState.Critical) // <=
{
return;
}
....
}Сообщение анализатора:
V3022. Expression is always true. Probably the '&&' operator should be used here. ....\Content.Shared\DoAfter\SharedDoAfterSystem.cs 49
Согласно сообщению анализатора, приведённое условное выражение всегда истинно, в результате чего последующий код будет недостижим. Причина этой ошибки заключается в неправильном использовании оператора || вместо оператора &&.
Вот еще один пример:
public GasTankWindow(GasTankBoundUserInterface owner)
{
....
_spbPressure = new FloatSpinBox
{
IsValid = f => f >= 0 || f <= 3000, // <=
....
};
....
}Сообщение анализатора:
V3022. Expression 'f >= 0 || f <= 3000' is always true. Probably the '&&' operator should be used here. GasTankWindow.cs 168.
Анализатор сообщает, что выражение f >= 0 || f <= 3000 всегда истинно. Это действительно так, ведь если значение f будет больше или равно 0, то выполнится первое условие — f >= 0, а если f меньше 0, то выполнится второе условие — f <= 3000. Очевидно, что разработчик здесь хотел ограничить переменную диапазоном от 0 до 3000. Однако для этого в условном выражении должен использоваться оператор && вместо ||:
_spbPressure = new FloatSpinBox
{
IsValid = f => f >= 0 && f <= 3000,
....
};Анонимная функция используется для отписки от события
protected override void Dispose(bool disposing)
{
MenuBody.OnChildRemoved -= ctrl =>
_uiController.OnRemoveElement(this, ctrl);
....
}Сообщение анализатора:
V3084. Anonymous function is used to unsubscribe from 'OnChildRemoved' event. No handlers will be unsubscribed, as a separate delegate instance is created for each anonymous function declaration. ContextMenuPopup.xaml.cs 74.
Здесь только что объявленная анонимная функция используется для отписки от события. Но отписки не произойдёт, т. к. эта функция является отдельным экземпляром делегата и никак не связана ни с одним из обработчиков события. Даже если реализации функции для отписки и функции-обработчика одинаковы, они всё равно будут представлять разные объекты.
Вот еще несколько примеров аналогичных ошибок, найденных в этом проекте:
public void OnRemoveElement(ContextMenuPopup menu, Control control)
{
....
element.OnMouseEntered -= _ => OnMouseEntered(element);
element.OnMouseExited -= _ => OnMouseExited(element);
element.OnKeyBindDown -= args => OnKeyBindDown(element, args);
....
}Сообщения анализатора:
V3084. Anonymous function is used to unsubscribe from 'OnMouseEntered' event. No handlers will be unsubscribed, as a separate delegate instance is created for each anonymous function declaration. ContextMenuUIController.cs 205.
V3084. Anonymous function is used to unsubscribe from 'OnMouseExited' event. No handlers will be unsubscribed, as a separate delegate instance is created for each anonymous function declaration. ContextMenuUIController.cs 206.
V3084. Anonymous function is used to unsubscribe from 'OnKeyBindDown' event. No handlers will be unsubscribed, as a separate delegate instance is created for each anonymous function declaration. ContextMenuUIController.cs 207.
Заключение
Мы познакомились с лучшими, на мой взгляд, Open Source играми на C# и рассмотрели наиболее интересные (но не все) ошибки, найденные в их исходном коде с помощью статического анализатора.
К слову, анализатор PVS-Studio, используемый в этой статье, вы можете попробовать бесплатно.
На этом у меня всё. Надеюсь, статья была для вас интересной :)
Успешных проектов вам и хорошего кода! Пока!
Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Moskalev. Top 3 open-source games written in C#: searching for bugs.
