Это первая статья из небольшого цикла, посвящённого знакомству с некоторыми любопытными VR-играми, а заодно и с примерами проблем в их исходном коде, которые нашлись с помощью PVS-Studio. Знакомьтесь, RocketMan!

О игре

RocketMan — это очень короткая, простая, но всё же увлекательная игра-головоломка. По лору вы собрались сбежать с погибающей планеты на космической ракете, но всё пошло не так: ракета оказалась в совершенно непригодном для использования состоянии, системы вышли из строя, все отсеки заперты, а вы застряли в одном из них. Теперь перед вами стоит задача выбраться из ловушки, привести в порядок один отсек за другим, в конечном итоге починить всю ракету и наконец улететь!

Имея в своём распоряжении VR-гарнитуру, я сам сыграл в игру и в целом получил положительные эмоции, несмотря на визуальную простоту, некорректность захвата некоторых предметов и одну внеплановую эвакуацию из ракеты через текстуры :)

По геймплею в игре, как мне показалось, не хватает какого-нибудь таймера до коллапса на планете, от которого мы и пытаемся сбежать. Ещё круче будет, если к таймеру добавить нагнетающие атмосферу эффекты вроде землетрясений, учащающихся ближе к концу. А что в игру добавили бы вы? Можете поделиться своими идеями в комментариях.

Теперь, когда мы немного познакомились с игрой, почему бы не познакомиться с ошибками в её исходном коде, которые были найдены с помощью PVS-Studio? Но погодите, а что такое PVS-Studio?

O PVS-Studio

Если вдруг вы слышите это название впервые, PVS-Studio — это инструмент, который автоматически проверяет качество и безопасность кода на C#, Java, C или С++ без его непосредственного выполнения. Когда я говорю "код", то имею в виду не только отдельные классы или файлы, но и целые проекты и решения. Опечатки, потенциальные исключения, логические ошибки, уязвимости безопасности — далеко неполный список того, что умеет находить этот инструмент.

Также хочется отметить, что в PVS-Studio уделено отдельное внимание специфике кода Unity-скриптов. В частности, в анализаторе есть:

аннотации API из пространства имён UnityEngine, предоставляющие дополнительную информацию о назначении, принимаемых аргументах и возвращаемых значениях API. Эта информация помогает детектировать больше ошибок, для выявления которых требуется хорошее понимание потоков данных в коде;
диагностики, детектирующие специфичные для Unity-скриптов проблемы;
диагностики, выявляющие возможности для микрооптимизаций в блоках таких часто выполняемых функций, как Update;
учёт перегрузки проверок на равенство/неравенство Unity-объектов с null. Анализатор понимает, что такие проверки могут также означать уничтоженность объекта в игре, что сокращает количество ложных предупреждений о потенциальном null-разыменовании.

Ещё больше узнать об этих особенностях можно в следующих статьях:

А здесь можно ознакомиться с полным списком диагностик анализатора.

Теперь давайте рассмотрим принцип базовой работы с анализатором. Он достаточно прост:

вместе с анализатором устанавливается специальное расширение для вашей IDE (у меня это Visual Studio);
в инспекторе IDE выбираются файлы, проекты или решения, которые нужно проанализировать, и с помощью специальной опции в контекстном меню запускается анализ;

Во время анализа на специальной панели будут постепенно появляться его результаты:

Просматриваем предупреждения анализатора, которых может оказаться немало. В этом случае можно воспользоваться многочисленными фильтрами и функцией массового обратимого подавления предупреждений. Она позволяет просматривать предупреждения частями, чтобы можно было распределить нагрузку.

При желании больше информации об анализаторе вы можете найти на сайте. А теперь давайте наконец приступим к обзору проблем, найденных в коде игры.

О исходном коде

Может быть, потому что игра небольшая, а может, потому что разработчики пишут хороший код, но явных ошибок непосредственно в коде игры удалось найти немного. Кроме ошибок, анализатор также дал несколько советов по микрооптимизации, с некоторыми из которых мы тоже ознакомимся.

Куда больше удивили скрипты Oculus, представляющие базовые компоненты для VR-взаимодействий. Большая часть ошибок была найдена именно в них, хоть их тоже немного. Также стоит отметить, что в игре используется устаревшее VR-API. Но это не повод нам с вами не прокачать свой скилл программиста и не поучиться на них :)

Обзор ошибок

Неиспользованный итератор

public void InsertChar(string c) 
{
  ....
  waiter(); //avoid double button clicks
}
IEnumerator waiter()
{
  yield return new WaitForSecondsRealtime(2);
}

Предупреждение PVS-Studio:

V3206 Unity Engine. A direct call to the 'waiter' coroutine-like method will not start it. Use the 'StartCoroutine' method instead. Keyboard_Tuto.cs 26.

В методе InsertChar есть вызов метода 'waiter', который по задумке должен приостанавливать поток выполнения на две секунды. На это указывает возвращаемое значение waiter — new WaitForSecondsRealtime(2).

В действительности же это работать не будет, причём сразу по двум причинам:

При вызове waiter будет возвращён объект-итератор без выполнения логики в теле метода. Чтобы эта логика все же выполнилась, нужно вызвать waiter().MoveNext();
Сам по себе возврат new WaitForSecondsRealtime(2) не приостанавливает поток выполнения. По сути, это итератор-таймер, который при вызове у него 'MoveNext' возвращает логическое значение, указывающее, прошёл ли заданный отрезок времени или нет.

Вероятно, программист хотел предотвратить повторный вызов InsertChar из-за случайного повторного клика по некоторой кнопке. В таком случае корректный код может выглядеть так:

bool _disableInsertChar = false;
public void InsertChar(string c) 
{
  if (_disableInsertChar)
    return;
  ....
  StartCoroutine(waiter()); 
}
IEnumerator waiter()
{
  _disableInsertChar = true;
  yield return new WaitForSecondsRealtime(2);
  _disableInsertChar = false;
}

В этой реализации waiter будет запущен как корутина Unity, т.к. возвращённый им итератор будет передан в качестве аргумента методу StartCoroutine. Это значит, что логика в блоке waiter будет выполняться асинхронно, а возврат объекта new WaitForSecondsRealtime(2) действительно будет приостанавливать выполнение этой логики на две секунды (т.к. этот класс предназначен именно для использования в корутинах).

Новое поле _disableInsertChar будет установлено в true при запуске корутины, и в false — по истечению двух секунд и завершению работы корутины. В начало метода InsertChar был добавлен возврат из метода, если _disableInsertChar равен true.

Некорректная проверка на null ввиду неявной инициализации поля

public class OVRTrackedKeyboard : MonoBehaviour
{
  ....
  public Transform ActiveKeyboardTransform;
  ....
}

public class OVRTrackedKeyboardHands : MonoBehaviour
{
  ....
  private void LateUpdate()
  {
    ....
    var position = KeyboardTracker.ActiveKeyboardTransform?.position;
    var rotation = KeyboardTracker.ActiveKeyboardTransform?.rotation;
    ....
  }
  ....
}

Предупреждение PVS-Studio:

V3216 [CWE-754] Unity Engine. Checking the 'ActiveKeyboardTransform' field with a specific Unity Engine type for null with '?.' may not work correctly due to implicit field initialization by the engine. OVRTrackedKeyboardHands.cs 296.

Это предупреждение основано на одном неочевидном, специфическом для Unity-скриптов моменте: неявной инициализацией объектом-пустышкой сериализуемых полей с типом UnityEngine.Object или производным от него (исключения: MonoBehaviour, ScriptableObject).

Для наглядности, давайте отладимся в простом MonoBehaviour-скрипте, где есть лишь два публичных поля: одно c типом Transform, а другое — MonoBehaviour. Оставив значения полей в инспекторе пустыми, давайте посмотрим, действительно ли они оба имеют значение null:

Как видим поле с типом Transform инициализировано некоторым объектом, который лишь "притворяется" null-ом, но не является им. Это значит, что такие проверки на null, как ?., ??, ??=, is null попросту не сработают, ведь для них поле инициализировано некоторым значением.

В примере кода из проанализированного проекта мы можем наблюдать как раз такой случай: поле KeyboardTracker.ActiveKeyboardTransform проверяют с помощью ?., но из-за неявной инициализации проверка не сработает, а при последующем разыменовании будет выброшено исключение.

Хорошая новость в том, что неявная инициализация таких полей выполняется только при "проигрывании" проекта из редактора. В релизной сборке всё будет работать так, как и ожидается.

Несмотря на эту оговорку, для проверки на null всё же стоит использовать операторы ==, != или сокращённые проверки field, !field, чтобы избежать неожиданных ошибок при отладке. Оба эти способа имеют специальные перегрузки, учитывающие неявную инициализацию, а значит, будут всегда работать правильно.

Путаница между || и && операторами

public static bool SaveCubemapCapture(....)
{
  ....
  if (    dirName[dirName.Length - 1] != '/' 
       || dirName[dirName.Length - 1] != '\\')
  {
    dirName += "/";
  }
  ....
}

Предупреждение PVS-Studio:

V3022 [CWE-571] Expression is always true. Probably the '&&' operator should be used here. OVRCubemapCapture.cs 201.

Отвлечёмся от специфичных для Unity-скриптов ошибок и рассмотрим вполне типичный случай. Условие в приведённом коде всегда равно true, ведь в нем через оператор || выполняются проверки на неравенство dirName[dirName.Length - 1] двум разным значениям. Таким образом, если первое подусловие будет ложно, то второе — истинно, и наоборот.

Очевидно, этой проверкой программист хотел убедиться, что в конце строки имеется разделитель файлового пути. Чтобы она заработала так, как ожидалось, достаточно заменить оператор || на &&.

Неосвобождённая память после чтения потока

private SceneInfo GetSceneInfo()
{
  SceneInfo sceneInfo = new SceneInfo();
  try
  {
    var reader = new StreamReader(sceneLoadDataPath); // <=
    ....
    while (!reader.EndOfStream)
    {
      sceneList.Add(reader.ReadLine());
    }
    sceneInfo.scenes = sceneList;
  }
  ....

}

Предупреждение PVS-Studio:

V3114 [CWE-404] IDisposable object 'reader' is not disposed before method returns. OVRSceneLoader.cs 212

Здесь анализатор обнаружил небольшую утечку вычислительных ресурсов. После использования объекта типа StreamReader требуется явно освободить выделенную под сторонние (недоступные для GC) ресурсы память, вызвав метод Dispose().

Кроме того, можно воспользоваться конструкцией using, по завершению выполнения блока которой Dispose будет вызван неявно. Исправленный код может выглядеть так:

private SceneInfo GetSceneInfo()
{
  SceneInfo sceneInfo = new SceneInfo();
  try
  {
    using (var reader = new StreamReader(sceneLoadDataPath))
    {
      ....
      while (!reader.EndOfStream)
      {
        sceneList.Add(reader.ReadLine());
      }
      sceneInfo.scenes = sceneList;

    }
  }
  ....

}

Одинаковые подвыражения слева и справа от оператора

Issue 1

private static bool RenameSpatializerPluginToOld(string currentPluginPath)
{
  ....
  targetPluginPath = currentPluginPath + ".old" + index.ToString();
  targetPluginMetaPath = targetPluginPath + ".meta";

  if (!File.Exists(targetPluginPath) && !File.Exists(targetPluginPath))
    break;
  ....
}

Предупреждение PVS-Studio:

V3001 There are identical sub-expressions '!File.Exists(targetPluginPath)' to the left and to the right of the '&&' operator. ONSPAudioPluginUpdater.cs 138.

Типичная ошибка, допущенная по невнимательности. В условии дважды подряд проверяется существование файла по одному и тому же пути из targetPluginPath. Наличие рядом с условием ещё одной переменной, хранящей путь к файлу targetPluginMetaPath, наводит на мысль, что вторая проверка предназначалась для неё.

Рассмотрим похожий случай.

Issue 2

protected void UpdateActive(OvrAvatar avatar, ovrAvatarVisibilityFlags mask)
{
  ....
  bool showFirstPerson = (mask & ovrAvatarVisibilityFlags.FirstPerson) != 0;
  bool showThirdPerson = (mask & ovrAvatarVisibilityFlags.ThirdPerson) != 0;

  gameObject.SetActive(showThirdPerson || showThirdPerson);
  ....
}

Предупреждение PVS-Studio:

V3001 There are identical sub-expressions 'showThirdPerson' to the left and to the right of the '||' operator. OvrAvatarRenderComponent.cs 21.

Обратите внимание на бинарное выражение, передаваемое в метод gameObject.SetActive. Оно представляет собой оператор ||, слева и справа от которого проверяется одна и та же переменная showThirdPerson. Как и в предыдущем случае, в ближайшем к условию коде есть ещё одна переменная showFirstPerson, которая, вероятно, должна была использоваться вместо одного из дубликатов.

Цикл, который вовсе не цикл

protected override void DoSelectUpdate(....)
{
  ....
  while (!outsideDelta)
  {
    ....
    if (....)
    {
      outsideDelta = true;
      break;
    }

    if (....)
    {
      outsideDelta = true;
      break; // <=
    }

    break; // <=
  }
  ....
}

Предупреждение PVS-Studio:

V3020 [CWE-670] An unconditional 'break' within a loop. PokeInteractor.cs 300.

В процессе написания функционала часто приходит идея более оптимальной или правильной его реализации, и приходится всё переделывать. Случается так, что остаются артефакты, некоторые из которых могут быть безвредны, а другие могут стать серьёзной ошибкой.

Так, в коде выше представлен цикл без операторов continue и безусловным break в конце, который явно является таким артефактом. Следствием его наличия является серьёзная ошибка, из-за которой у цикла всегда будет только одна итерация.

Обзор микрооптимизаций

Помимо помощи в обнаружении ошибок, анализатор PVS-Studio может давать некоторое советы по микрооптимизации Unity-скриптов. На первый взгляд такие кейсы могут показаться не очень важными, но представьте, что какие-нибудь избыточные выделения памяти или вычисления происходят при каждом обновлении кадра десятки раз в секунду.

А если таких кейсов не один-два, а один-два десятка? В этом случае микрооптимизационные упущения могут "съедать" значительную долю вычислительных ресурсов и тем самым ограничить ваши возможности по дальнейшему масштабированию проекта.

В собственном коде RocketMan таких предупреждений оказалось не так уж много, всего 25. Давайте рассмотрим некоторые из них.

Выделение памяти при каждом обновлении кадра

void Update()
{
  ....
  FindHoverObject(controllerPos, controllerRot);
  ....
}

void FindHoverObject(Vector3 controllerPos, Quaternion controllerRot)
{
  var objectsHit = Physics.RaycastAll(controllerPos, 
                                      controllerRot * Vector3.forward);
  ....
}

Предупреждение PVS-Studio: V4008 Unity Engine. Avoid using the 'Physics.RaycastAll' memory allocation method in performance-sensitive context inside 'FindHoverObject'. Consider using the 'RaycastNonAlloc' non-allocation method instead. ObjectManipulator.cs 55.

В этом примере Physics.RaycastAll вызывается при обновлении каждого кадра. Но что же тут можно оптимизировать? Дело в том, что при каждом вызове Physics.RaycastAll (несколько десятков раз в секунду) выделяется память под новую коллекцию для возвращаемого результата. Это приводит к учащению вызова GC, что в свою очередь замедляет работу приложения.

К счастью, у Physics.RaycastAll есть более оптимальная альтернатива — Physics.RaycastNonAlloc. Разница между ними в том, что Physics.RaycastNonAlloc принимает в качестве аргумента ссылку на некоторый буфер, в который и записывает результат. Сам буфер можно вынести в поле, инициализировать всего раз, а после перезаписывать, не освобождая и не выделяя повторно память.

Вызов вычислительно дорогой функции при каждом обновлении

void Update()
{
  ....
  if (grabObject)
  {
    ....
    ManipulateObject(grabObject, 
   controllerPos, 
   controllerRot);
    ....
  }
  ....
}
void ManipulateObject(....)
{
  ....
  if (obj.GetComponent<GrabObject>()) // <=
  {
     ....
     switch (obj.GetComponent<GrabObject>() // <=
    .objectManipulationType)
     ....
  }
  ....
}

Предупреждение PVS-Studio:

V4005 Expensive operation is performed inside the 'GetComponent' method. Calling such method in performance-sensitive context located inside the 'ManipulateObject' method can lead to decreased performance. ObjectManipulator.cs 71.

В этот раз анализатор обращает внимание на избыточные вызовы obj.GetComponent<GrabObject>() внутри метода Update. GetComponent довольно дорогой с вычислительной точки зрения, а потому стоит избегать его использования внутри таких часто исполняемых методов, как Update.

Сложно придумать случай, когда без этого нельзя было бы обойтись, ведь компонент, получаемый с помощью GetComponent, можно один раз закэшировать в поле в таких методах, как Start и Awake. А вместо того, чтобы регулярно добавлять/удалять компонент, можно включать и выключать его с помощью свойства Behaviour.enabled.

Неоптимальное арифметическое выражение

private void Update()
{
  ....
  if (referenceHand)
  {
    ....            
    CreateHandMesh(); // <=
  }
  ....
}

void CreateHandMesh()
{
  ....
  for (int i = 0; i < 5; i++)
  {
    ....
    AddKnuckleMesh(....); // <=
    AddKnuckleMesh(....); // <=
    ....
    AddKnuckleMesh(....); // <=
  }
  ....
}

void AddKnuckleMesh(....)
{
  ....
  Vector3 windingVec = rightVec;

  for (int i = 0; i < fanVerts * 2; i++)
  {
    ....
    Vector3 vertPos = handVertices[basePoint] + 
                      windingVec *       // <=
                      borderSize * 
                      handScale * 
                      (....);
    ....
  }
  ....
}

Предупреждение PVS-Studio:

V4006 Unity Engine. Multiple operations between the 'windingVec' complex value and numeric values. Prioritizing operations between numeric values can optimize execution time. HandMeshMask.cs 70.

Посмотрите на выражение инициализации переменной vertPos в методе AddKnuckleMesh. Оно представляет собой операцию сложения, один из операндов которой — цепочка операций умножения. Именно эта цепочка и является местом в оптимизации, на которое указывает анализатор. Что же в нём не так?

Обратите внимание, что самый первый множитель — переменная windingVec — имеет тип Vector3. При умножении этого типа на число выполняется умножение каждого из его трёх компонентов на это число. Это значит, что в ходе вычисления всей цепочки будет выполнено девять операций. Это число можно уменьшить, если сначала перемножить между собой обычные числа, а уже результат умножить на вектор. В этом случае будет выполнено уже шесть операций, а цепочка умножений будет выглядеть так:

windingVec * (borderSize * handScale * (....))

На первый взгляд эта оптимизация кажется незначительной, но давайте посчитаем количество выполняемых операций без оптимизации с учётом контекста:

умножения выполняются внутри цикла, у которого в среднем, допустим, пять итераций (9 * 5 = 45);
метод AddKnuckleMesh трижды вызывается внутри цикла с пятью итерациями в методе CreateHandMesh (45 * 3 * 5 = 675);
метод CreateHandMesh вызывается в методе Update, частота исполнения которого зависит от частоты кадров. Допустим, в среднем 100 раз в секунду (675 * 100 = 67500);
стоит также учесть, что вызов CreateHandMesh находится под условием. Нельзя наверняка сказать, насколько сильно оно влияет на частоту исполнения кода внутри своего блока, поэтому просто допустим, что частота уменьшается в два раза (67500 / 2 = 33750).

В итоге мы получили пусть и неточное, но достаточно наглядное представление о количестве операций умножения (33750), выполняемых за секунду только в рассматриваемом выражении.

А теперь скорректируем это число с учётом оптимизации (33750 / 9 * 6 = 22500). Разница в 11250 вычислительных операций в секунду. Неплохая оптимизация, правда?

Заключение

Надеюсь, на примере анализа VR-игры RocketMan мне удалось хотя бы отчасти продемонстрировать вам пользу статического анализатора кода в разработке на Unity. Честно говоря, RocketMan — довольно небольшой проект, по-настоящему же статический анализатор раскрывает себя на средних и больших кодовых базах.

В этих случаях анализатор может сэкономить много времени, которое бы было потрачено на избыточное ревью кода и отладку, а также заметно минимизировать количество "упущенных" ошибок, которые могли быть выявлены уже слишком поздно самими пользователями.

К слову, вы можете опробовать PVS-Studio на своих проектах: у анализатора есть бесплатный период, запросить ключ для которого можно тут. Кроме того, вы, вероятно, захотите ознакомиться с гайдом по первому запуску анализа.

До встречи в следующих статьях!

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Moskalev. Digging into open-source Unity VR Games. Part 1: RocketMan.

Копаемся в открытом исходном коде Unity VR игр. Часть 1: RocketMan