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

Об игре

NorthStar — это небольшая, но весьма красивая демонстрационная игра, где вы примите на себя роль потерпевшего кораблекрушение, чудом спасенного командой корабля Polaris, частью которой вам суждено стать.

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

Верёвки, с помощью которых можно обматывать, раскачивать и толкать другие объекты.

Подзорная труба, в которую можно разглядеть объекты, находящиеся очень далеко.

Гарпунное ружьё с ручным управлением: зарядка болта, натягивание тетивы, прицеливание путем вращения гарпуна и выстрел при нажатии кнопки.

Попали гарпуном во что-то интересное? Покрутите рукоятки, чтобы поднять это на корабль.

Кроме того, в игре имеются такие механики, как смена дня и ночи, смена погоды, реалистичное переменное движение корабля, NPC.

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

О PVS-Studio

На случай, если вы ещё не знакомы и не читали первую статью из этой серии.

Анализатор PVS-Studio — это инструмент для автоматического поиска потенциальных проблем в коде C#, C++, C и Java проектов.

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

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

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

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

Использовать анализатор просто. Базовый процесс сводится к трём основным шагам:

1. Запуск анализа отдельных файлов, проектов или всего решения целиком;

2. Ожидание появления первых предупреждений в специальной панели или же полного завершения анализа;

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

Отдельного внимания заслуживает вкладка Best warnings. В ней отображаются срабатывания, которые с наибольшей вероятностью могут оказаться полезными.

Стоит отметить, что выше приведены иллюстрации для Visual Studio, но PVS-Studio можно интегрировать и в другие IDE и текстовые редакторы, такие как Rider и Visual Studio Code. Полный список можно найти на сайте.

Итак, почему бы теперь не посмотреть на PVS-Studio в деле?

O коде

Как и в предыдущей статье, ошибок удалось найти немного. Некоторые из них были в сторонних скриптах, но и предупреждения на собственный код игры также имеются. Ниже мы рассмотрим самые явные и интересные из них. Погнали!

Потенциальное обращение по негативному индексу

public void Skip()
{
  ....
  var currTaskIndex = Task.Sequence
                          .TaskDefinitions
                          .FindIndex(def => def.ID == TaskID);

  if (currTaskIndex != 0)
  {
    var lastTask = Task.Sequence.TaskDefinitions[currTaskIndex - 1];
    ....
  }
  ....
}

Предупреждение PVS-Studio: V3106 [CWE-125] Possible negative index value. The value of 'currTaskIndex - 1' index could reach -2. TaskHandler.cs 201

Анализатор предупреждает о возможном обращении по негативному индексу. Код действительно выглядит подозрительно; по какой-то причине проверкой currTaskIndex != 0 исключается самый первый элемент коллекции, и при этом отсутствует проверка на равенство индекса -1, что означало бы отсутствие в коллекции нужного элемента.

Исключение, которое не ждали

public void CollisionPass(....)
{
  ....
  if (!CachedSphereCollider.TryGet(out var sphereCollider)) // <=
  {
    return;
  }
  ....
}

Предупреждение PVS-Studio: V3022 [CWE-570] Expression '!CachedSphereCollider.TryGet(out var sphereCollider)' is always false. NPC_DynamicRigs.cs 522

Здесь анализатор предупреждает о том, что метод CachedSphereCollider.TryGet всегда возвращает одно значение — true. Так ли это? Убедимся, взглянув на декларацию:

public static bool TryGet(out SphereCollider collider)
{
  if (s_hasSphere)
  {
    collider = s_sphereCollider;
    return true;
  }

  try
  {
    ....
    s_sphereCollider = obj.GetComponent<SphereCollider>();
    collider = s_sphereCollider;
    collider.enabled = false;
    s_hasSphere = true;
    return true;
  }
  catch
  {
    ....
    throw;
  }
}

Как видим, это действительно так. Очень странная реализация. По идее, метод должен возвращать true в случае, если объект sphereCollider удалось получить тем или иным образом, и false — в обратном случае. На деле же в случае, если по каким-то причинам значение sphereCollider получить не удалось, метод выбрасывает исключение. Если пробежаться по стеку вызовов CollisionPass вплоть до точек входа, таких как LateUpdate, можно убедиться, что это исключение нигде не обрабатывается.

Потенциальное null-разыменование

private void DisplayResults()
{
  if (m_texturesWithoutMipmaps.Count > 0)
  {
    ....
  }
  else if (m_texturesWithoutMipmaps != null)
  {
    ....
  }
}

Предупреждение PVS-Studio: V3095 [CWE-476] The 'm_texturesWithoutMipmaps' object was used before it was verified against null. Check lines: 91, 128. MipMapChecker.cs 91

Анализатор обнаружил потенциальное null-разыменование в условии if. На возможность этого указывает проверка в условии последующего else if. Идея проверять сначала наличие элементов в коллекции, а потом уже равенство коллекции с null кажется очень странной. Но всё становится логичным, стоит добавить в первое условие ?.:

private void DisplayResults()
{
  if (m_texturesWithoutMipmaps?.Count > 0)
  {
    ....
  }
  else if (m_texturesWithoutMipmaps != null)
  {
    ....
  }
}

Ненадёжная проверка на null

public class FadeVolume : MonoBehaviour
{
  [SerializeField] private AudioSource m_audioSource;
  ....
  public void FadeOutAudio(float time)
  {
    if (m_audioSource is null)
    {
      ....
    }
    ....
  }
  ....
}

Предупреждение PVS-Studio: V3216 [CWE-754] Unity Engine. Checking the 'm_audioSource' field with a specific Unity Engine type for null with 'is' may not work correctly due to implicit field initialization by the engine. FadeVolume.cs 25

Проблема в этом коде не опасна в Release сборке игры, но может привести к неожиданным ошибкам во время проигрывания в редакторе Unity. Дело в том, что отображаемые в инспекторе поля с типом UnityEngine.Object или производным от него (кроме MonoBehaviour и ScriptableObject) неявно инициализируются объектом-пустышкой, если ни в инспекторе, ни в коде эти поля не были проинициализированы. Ниже приведён момент отладки, наглядно иллюстрирующий это:

В результате неявной инициализации проверки на null, не имеющие специальной перегрузки, в частности ??, ??=, ?., is null окажутся бесполезны, т.к. формально значение у поля есть. Чтобы избежать такого неочевидного поведения, для проверок на null следует использовать операторы ==, != или сокращённые проверки m_audioSource, !m_audioSource, имеющие перегрузку, учитывающую этот момент.

Некорректный цикл

string MemberLabel(string memberName)
{
  foreach (var memberInfo in currObject.GetType().GetMember(memberName))
  {
    if (....)
    {
      memberName = $"{memberName}()";
    }

    return $"{memberInfo.DeclaringType?.Name}.{memberName}";
  }

  return memberName;
}

Предупреждение PVS-Studio: V3020 [CWE-670] An unconditional 'return' within a loop. MemberLinkDrawer.cs 162

Странный цикл, который перебирает коллекцию элементов, но при этом у него всегда будет только одна итерация из-за безусловного return в теле. Возможно, этот return должен был находиться в блоке if внутри цикла. В этом случае код выглядел бы намного логичнее:

string MemberLabel(string memberName)
{
  foreach (var memberInfo in currObject.GetType().GetMember(memberName))
  {
    if (....)
    {
      memberName = $"{memberName}()";
      return $"{memberInfo.DeclaringType?.Name}.{memberName}";
    }
  }

  return memberName;
}

Опечатки

Issue 1

[Serializable]
public struct ComfortPreset
{
  public float BoatMovementStrength;
  public float BoatReactionStrength;
}
public void ComfortLevelChanged()
{
  OnComfortLevelChanged?.Invoke(PlayerConfigs.ComfortLevel);
  var comfortPreset = ComfortPresets[(int)ComfortLevel];
  BoatMovementStrength = comfortPreset.BoatMovementStrength;
  BoatReactionStrength = comfortPreset.BoatMovementStrength;
}

Предупреждение PVS-Studio: V3127 [CWE-682] Two similar code fragments were found. Perhaps, this is a typo and 'BoatReactionStrength' variable should be used instead of 'BoatMovementStrength' GlobalSettings.cs 70.

Анализатор обнаружил явную опечатку: полю BoatReactionStrength присваивается не то значение. Это легко понять, ведь в структуре ComfortPreset есть одноимённое поле, значение которого и должно быть присвоено. Исправленный код выглядит так:

public void ComfortLevelChanged()
{
  ....
  BoatMovementStrength = comfortPreset.BoatMovementStrength;
  BoatReactionStrength = comfortPreset.BoatReactionStrength;
}

Issue 2

public void OffsetVerlet(Vector3 offset)
{
  m_previousFrame = new Frame
  {
    Position = m_previousFrame.Position + offset,
    Time = m_previousFrame.Time,
  };
  m_currentFrame = new Frame
  {
    Position = m_currentFrame.Position + offset,
    Time = m_previousFrame.Time,
  };
}

Предупреждение PVS-Studio: V3127 [CWE-682] Two similar code fragments were found. Perhaps, this is a typo and 'm_currentFrame' variable should be used instead of 'm_previousFrame' NPC_DynamicRigs.cs 868

Обратите внимание на общий паттерн инициализаций m_previousFrame и m_currentFrame. Но в одном он нарушается: в обоих случаях Time присваивается значение m_previousFrame.Time. Очень вероятно, что в случае инициализации m_currentFrame это присвоение содержит ошибку, и вместо m_previousFrame.Time должно быть присвоено m_currentFrame.Time.

Issue 3

public class DepthNormalOnlyPass : ScriptableRenderPass
{
  ....
  internal void Render(RenderGraph renderGraph, 
    out TextureHandle cameraNormalsTexture, 
    out TextureHandle cameraDepthTexture, 
    ref RenderingData renderingData)
  {
    ....
  }
  ....
}

private void OnMainRendering(....)
{
  ....
  m_DepthNormalPrepass.Render(renderGraph, 
    out frameResources.cameraDepthTexture, 
    out frameResources.cameraNormalsTexture, 
    ref renderingData);
  ....
}

Предупреждение PVS-Studio: V3066 [CWE-683] Possible incorrect order of arguments passed to 'Render' method: 'frameResources.cameraDepthTexture' and 'frameResources.cameraNormalsTexture'. UniversalRendererRenderGraph.cs 308

Ещё одна опечатка, на этот раз на собственном коде Unity. Сравните имена out-параметров и полей, которые передаются в них в вызове m_DepthNormalPrepass.Render. Карта глубины и карта нормалей были перепутаны местами. Поскольку эти текстуры используются для разных целей, они не являются взаимозаменяемыми. В конечном итоге эта ошибка может привести к заметным визуальным дефектам.

Ошибка из-за неправильного представления о приоритете операции '??'

public Quaternion? GetCorrectionQuaternion(HumanBodyBones humanBodyBone)
{
  var targetData = ....;

  if (targetData == null)
  {
    return null;
  }
  if (!targetData.CorrectionQuaternion.HasValue)
  {
    return null;
  }
  return targetData.CorrectionQuaternion.Value;
}

private void RetargetHand(....)
{
  ....
  var correctionQuaternion = retargetingLayer.GetCorrectionQuaternion(....);
  ....
  
  Transform t = ....;

  t.rotation = pose.rotation * correctionQuaternion ?? Quaternion.identity;
  ....
}

Предупреждение PVS-Studio: V3123 [CWE-783] Perhaps the '??' operator works in a different way than it was expected. Its priority is lower than priority of other operators in its left part. SyntheticHandRetargetingProcessor.cs 89

Анализатор обнаружил выражение которое может быть некорректным:

t.rotation = pose.rotation * correctionQuaternion ?? Quaternion.identity;

Чтобы лучше понять потенциальную проблему, давайте рассмотрим ход вычисления этого выражения в случае, если correctionQuaternion равен null:

Сначала будет выполнена перегруженная операция умножения. Если один из операндов null, в результате также будет null;
Далее будет выполнена проверка результата предыдущий операции с помощью ??. В результате t.rotation будет присвоено значение Quaternion.identity.

Quaternion.identity, по сути, означает отсутствие какого-либо вращения. Это значит, что в рассматриваемом выше случае поворот некоторого объекта будет резко обнулён. Вряд ли это является ожидаемым поведением, т.к. резкий сброс поворота объекта в большинстве случаев выглядит некорректно.

Куда вероятнее, что проверить с помощью ?? хотели именно correctionQuaternion, но разработчик не учёл, что приоритет у ?? ниже, чем у *. В этом случае исправленный вариант этого выражения выглядит так:

t.rotation = pose.rotation * (correctionQuaternion ?? Quaternion.identity);

Теперь в случае, если correctionQuaternion равен null, вращению объекта будет присвоен результат выражения pose\.rotation \* Quaternion\.identity. Т.к. операция умножения между двумя кватернионами, по сути, выполняет поворот первого кватерниона на вращение, заданное вторым, результатом этой операции будет просто pose\.rotation.``

Заключение

В этой статье мы познакомились с ещё одним интересным open source VR-проектом NorthStar, реализованным на Unity. Кроме того, немного познакомились и с его исходным кодом, рассмотрев найденные с помощью PVS-Studio ошибки.

Если у вас есть собственный проект, возможно, вам интересно, а смог бы анализатор найти какие-либо ошибки в нём? В этом случае вы можете воспользоваться бесплатным пробным периодом, запросив ключ тут. Туториал по запуску первого анализа Unity-проекта найти можно здесь.

А на этом у меня всё. До встречи в следующих статьях!

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