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

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

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

Об игре

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

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

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

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

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

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

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

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

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

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

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

Кроме того, в игре имеются такие механики, как смена дня и ночи, смена погоды, реалистичное переменное движение корабля, 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. Запуск анализа отдельных файлов, проектов или всего решения целиком;

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

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

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

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

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

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

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

Стоит отметить, что выше приведены иллюстрации для 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`) неявно инициализируются объектом-пустышкой, если ни в инспекторе, ни в коде эти поля не были проинициализированы. Ниже приведён момент отладки, наглядно иллюстрирующий это:

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

В результате неявной инициализации проверки на 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`:

1. Сначала будет выполнена перегруженная операция умножения. Если один из операндов `null`, в результате также будет `null`;

2. Далее будет выполнена проверка результата предыдущий операции с помощью `??`. В результате `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-проекта найти можно здесь.

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

1
Начать дискуссию