# Копаемся в открытом исходном коде Unity VR игр\. Часть 2: NorthStar Это вторая статья из небольшого цикла, посвящённого знакомству с некоторыми любопытными VR\-играми, а заодно и с примерами проблем в их исходном коде, найденных с помощью PVS\-Studio\. Знакомьтесь, NorthStar\! ![1279_NorthStarAnalysis_ru/image1.png](https://import.viva64.com/docx/blog/1279_NorthStarAnalysis_ru/image1.png) ## Об игре [NorthStar](https://github.com/oculus-samples/Unity-NorthStar/) — это небольшая, но весьма красивая демонстрационная игра, где вы примите на себя роль потерпевшего кораблекрушение, чудом спасенного командой корабля Polaris, частью которой вам суждено стать\. ![1279_NorthStarAnalysis_ru/image2.png](https://import.viva64.com/docx/blog/1279_NorthStarAnalysis_ru/image2.png) Помимо красивой графики \(один океан чего стоит\), игра может похвастаться уникальным интерактивом с элементами корабля\. Верёвки, с помощью которых можно обматывать, раскачивать и толкать другие объекты\. ![1279_NorthStarAnalysis_ru/image3.png](https://import.viva64.com/docx/blog/1279_NorthStarAnalysis_ru/image3.png) Подзорная труба, в которую можно разглядеть объекты, находящиеся очень далеко\. ![1279_NorthStarAnalysis_ru/image4.png](https://import.viva64.com/docx/blog/1279_NorthStarAnalysis_ru/image4.png) Гарпунное ружьё с ручным управлением: зарядка болта, натягивание тетивы, прицеливание путем вращения гарпуна и выстрел при нажатии кнопки\. ![1279_NorthStarAnalysis_ru/image5.png](https://import.viva64.com/docx/blog/1279_NorthStarAnalysis_ru/image5.png) Попали гарпуном во что\-то интересное? Покрутите рукоятки, чтобы поднять это на корабль\. ![1279_NorthStarAnalysis_ru/image6.png](https://import.viva64.com/docx/blog/1279_NorthStarAnalysis_ru/image6.png) Кроме того, в игре имеются такие механики, как смена дня и ночи, смена погоды, реалистичное переменное движение корабля, NPC\. Все это побудило меня познакомиться с игрой не только на уровне геймплея, но и на уровне исходного кода, в очередной раз испытав анализатор PVS\-Studio\. ## О PVS\-Studio На случай, если вы ещё не знакомы и не читали [первую статью](https://pvs-studio.ru/ru/blog/posts/csharp/1278/) из этой серии\. Анализатор PVS\-Studio — это инструмент для автоматического поиска потенциальных проблем в коде C\#, C\+\+, C и Java проектов\. PVS\-Studio может находить как простые опечатки, так и сложные ошибки и уязвимости безопасности, обнаружение которых требует понимания потока передачи данных и семантики кода\. Что касается проверки именно Unity\-проектов, PVS\-Studio учитывает специфику Unity\-скриптов, что позволяет анализатору эффективней проверять их код\. В частности, в анализаторе есть: * аннотации API из пространства имён UnityEngine, предоставляющие дополнительную информацию о назначении, принимаемых аргументах и возвращаемых значениях API\. Эта информация помогает находить больше ошибок, для выявления которых требуется хорошее понимание потоков данных в коде; * диагностики, обнаруживающие специфичные для Unity\-скриптов проблемы; * диагностики, выявляющие возможности для микрооптимизаций в блоках таких часто выполняемых функций, как `Update`; * учёт перегрузки проверок на равенство/неравенство Unity\-объектов с `null`\. Анализатор понимает, что такие проверки могут также означать уничтоженность объекта в игре, что сокращает количество ложных предупреждений о потенциальном null\-разыменовании\. Ещё больше узнать об этих особенностях можно в следующих статьях: * [Как анализатор PVS\-Studio стал находить ещё больше ошибок в проектах на Unity](https://pvs-studio.ru/ru/blog/posts/csharp/0744/); * [PVS\-Studio в разработке на Unity: новые специализированные диагностики](https://pvs-studio.ru/ru/blog/posts/csharp/1236/)\. Использовать анализатор просто\. Базовый процесс сводится к трём основным шагам: 1\. Запуск анализа отдельных файлов, проектов или всего решения целиком; ![1279_NorthStarAnalysis_ru/image7.png](https://import.viva64.com/docx/blog/1279_NorthStarAnalysis_ru/image7.png) 2\. Ожидание появления первых предупреждений в специальной панели или же полного завершения анализа; ![1279_NorthStarAnalysis_ru/image8.png](https://import.viva64.com/docx/blog/1279_NorthStarAnalysis_ru/image8.png) 3\. Просмотр предупреждений\. При анализе больших кодовых баз это может оказаться трудоёмкой задачей, но наличие разнообразных фильтров, а также возможность массового обратимого [подавления](https://pvs-studio.ru/ru/docs/manual/0032/) предупреждений помогут разделить этот процесс на несколько итераций\. ![1279_NorthStarAnalysis_ru/image9.png](https://import.viva64.com/docx/blog/1279_NorthStarAnalysis_ru/image9.png) Отдельного внимания заслуживает вкладка **Best warnings**\. В ней отображаются срабатывания, которые с наибольшей вероятностью могут оказаться полезными\. ![1279_NorthStarAnalysis_ru/image10.png](https://import.viva64.com/docx/blog/1279_NorthStarAnalysis_ru/image10.png) Стоит отметить, что выше приведены иллюстрации для Visual Studio, но PVS\-Studio можно интегрировать и в другие IDE и текстовые редакторы, такие как Rider и Visual Studio Code\. Полный список можно найти на [сайте](https://pvs-studio.ru/ru/docs/manual/0018/)\. Итак, почему бы теперь не посмотреть на PVS\-Studio в деле? ## O коде Как и в [предыдущей статье](https://pvs-studio.ru/ru/blog/posts/csharp/1278/), ошибок удалось найти немного\. Некоторые из них были в сторонних скриптах, но и предупреждения на собственный код игры также имеются\. Ниже мы рассмотрим самые явные и интересные из них\. Погнали\! ### Потенциальное обращение по негативному индексу ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v3106/) \[CWE\-125\] Possible negative index value\. The value of 'currTaskIndex \- 1' index could reach \-2\. [TaskHandler\.cs 201](https://github.com/oculus-samples/Unity-NorthStar/blob/d7c4f7c61f0db96d006df3ed52a0c3268881bf7e/Packages/com.meta.utilities.narrative/Runtime/NarrativeSequence/TaskHandler.cs#L201) Анализатор предупреждает о возможном обращении по негативному индексу\. Код действительно выглядит подозрительно; по какой\-то причине проверкой `currTaskIndex != 0` исключается самый первый элемент коллекции, и при этом отсутствует проверка на равенство индекса `-1`, что означало бы отсутствие в коллекции нужного элемента\. ### Исключение, которое не ждали ```cpp public void CollisionPass(....) { .... if (!CachedSphereCollider.TryGet(out var sphereCollider)) // <= { return; } .... } ``` Предупреждение PVS\-Studio: [V3022](https://pvs-studio.ru/ru/docs/warnings/v3022/) \[CWE\-570\] Expression '\!CachedSphereCollider\.TryGet\(out var sphereCollider\)' is always false\. [NPC\_DynamicRigs\.cs 522](https://github.com/oculus-samples/Unity-NorthStar/blob/d7c4f7c61f0db96d006df3ed52a0c3268881bf7e/Assets/NorthStar/Animation/AnimationScripts/NPC_DynamicRigs.cs#L522) Здесь анализатор предупреждает о том, что метод `CachedSphereCollider.TryGet` всегда возвращает одно значение — `true`\. Так ли это? Убедимся, взглянув на декларацию: ```cpp public static bool TryGet(out SphereCollider collider) { if (s_hasSphere) { collider = s_sphereCollider; return true; } try { .... s_sphereCollider = obj.GetComponent(); collider = s_sphereCollider; collider.enabled = false; s_hasSphere = true; return true; } catch { .... throw; } } ``` Как видим, это действительно так\. Очень странная реализация\. По идее, метод должен возвращать `true` в случае, если объект `sphereCollider` удалось получить тем или иным образом, и `false` — в обратном случае\. На деле же в случае, если по каким\-то причинам значение `sphereCollider` получить не удалось, метод выбрасывает исключение\. Если пробежаться по стеку вызовов `CollisionPass` вплоть до точек входа, таких как `LateUpdate`, можно убедиться, что это исключение нигде не обрабатывается\. ### Потенциальное null\-разыменование ```cpp private void DisplayResults() { if (m_texturesWithoutMipmaps.Count > 0) { .... } else if (m_texturesWithoutMipmaps != null) { .... } } ``` Предупреждение PVS\-Studio: [V3095](https://pvs-studio.ru/ru/docs/warnings/v3095/) \[CWE\-476\] The 'm\_texturesWithoutMipmaps' object was used before it was verified against null\. Check lines: 91, 128\. [MipMapChecker\.cs 91](https://github.com/oculus-samples/Unity-NorthStar/blob/d7c4f7c61f0db96d006df3ed52a0c3268881bf7e/Assets/NorthStar/Editor/MipMapChecker.cs#L91) Анализатор обнаружил потенциальное null\-разыменование в условии `if`\. На возможность этого указывает проверка в условии последующего `else if`\. Идея проверять сначала наличие элементов в коллекции, а потом уже равенство коллекции с `null` кажется очень странной\. Но всё становится логичным, стоит добавить в первое условие `?.`: ```cpp private void DisplayResults() { if (m_texturesWithoutMipmaps?.Count > 0) { .... } else if (m_texturesWithoutMipmaps != null) { .... } } ``` ### Ненадёжная проверка на null ```cpp public class FadeVolume : MonoBehaviour { [SerializeField] private AudioSource m_audioSource; .... public void FadeOutAudio(float time) { if (m_audioSource is null) { .... } .... } .... } ``` Предупреждение PVS\-Studio: [V3216](https://pvs-studio.ru/ru/docs/warnings/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](https://github.com/oculus-samples/Unity-NorthStar/blob/d7c4f7c61f0db96d006df3ed52a0c3268881bf7e/Assets/NorthStar/Scripts/Vfx-Sfx/FadeVolume.cs#L25) Проблема в этом коде не опасна в Release сборке игры, но может привести к неожиданным ошибкам во время проигрывания в редакторе Unity\. Дело в том, что отображаемые в инспекторе поля с типом `UnityEngine.Object` или производным от него \(кроме `MonoBehaviour` и `ScriptableObject`\) неявно инициализируются объектом\-пустышкой, если ни в инспекторе, ни в коде эти поля не были проинициализированы\. Ниже приведён момент отладки, наглядно иллюстрирующий это: ![1279_NorthStarAnalysis_ru/image11.png](https://import.viva64.com/docx/blog/1279_NorthStarAnalysis_ru/image11.png) В результате неявной инициализации проверки на null, не имеющие специальной перегрузки, в частности `??`, `??=`, `?.`, `is null` окажутся бесполезны, т\.к\. формально значение у поля есть\. Чтобы избежать такого неочевидного поведения, для проверок на `null` следует использовать операторы `==`, `!=` или сокращённые проверки `m_audioSource`, `!m_audioSource`, имеющие перегрузку, учитывающую этот момент\. ### Некорректный цикл ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v3020/) \[CWE\-670\] An unconditional 'return' within a loop\. [MemberLinkDrawer\.cs 162](https://github.com/oculus-samples/Unity-NorthStar/blob/d7c4f7c61f0db96d006df3ed52a0c3268881bf7e/Packages/com.meta.utilities.narrative/Editor/MemberLinkDrawer.cs#L162) Странный цикл, который перебирает коллекцию элементов, но при этом у него всегда будет только одна итерация из\-за безусловного `return` в теле\. Возможно, этот `return` должен был находиться в блоке `if` внутри цикла\. В этом случае код выглядел бы намного логичнее: ```cpp string MemberLabel(string memberName) { foreach (var memberInfo in currObject.GetType().GetMember(memberName)) { if (....) { memberName = $"{memberName}()"; return $"{memberInfo.DeclaringType?.Name}.{memberName}"; } } return memberName; } ``` ### Опечатки **Issue 1** ```cpp [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](https://pvs-studio.ru/ru/docs/warnings/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](https://github.com/oculus-samples/Unity-NorthStar/blob/d7c4f7c61f0db96d006df3ed52a0c3268881bf7e/Assets/NorthStar/Scripts/System/GlobalSettings.cs#L70)\. Анализатор обнаружил явную опечатку: полю `BoatReactionStrength` присваивается не то значение\. Это легко понять, ведь в структуре `ComfortPreset` есть одноимённое поле, значение которого и должно быть присвоено\. Исправленный код выглядит так: ```cpp public void ComfortLevelChanged() { .... BoatMovementStrength = comfortPreset.BoatMovementStrength; BoatReactionStrength = comfortPreset.BoatReactionStrength; } ``` **Issue 2** ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/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](https://github.com/oculus-samples/Unity-NorthStar/blob/d7c4f7c61f0db96d006df3ed52a0c3268881bf7e/Assets/NorthStar/Animation/AnimationScripts/NPC_DynamicRigs.cs#L868) Обратите внимание на общий паттерн инициализаций `m_previousFrame` и `m_currentFrame`\. Но в одном он нарушается: в обоих случаях `Time` присваивается значение `m_previousFrame.Time`\. Очень вероятно, что в случае инициализации `m_currentFrame` это присвоение содержит ошибку, и вместо `m_previousFrame.Time` должно быть присвоено `m_currentFrame.Time`\. **Issue 3** ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v3066/) \[CWE\-683\] Possible incorrect order of arguments passed to 'Render' method: 'frameResources\.cameraDepthTexture' and 'frameResources\.cameraNormalsTexture'\. [UniversalRendererRenderGraph\.cs 308](https://github.com/oculus-samples/Unity-NorthStar/blob/d7c4f7c61f0db96d006df3ed52a0c3268881bf7e/Packages/com.unity.render-pipelines.universal/Runtime/UniversalRendererRenderGraph.cs#L308) Ещё одна опечатка, на этот раз на собственном коде Unity\. Сравните имена `out`\-параметров и полей, которые передаются в них в вызове `m_DepthNormalPrepass.Render`\. Карта глубины и карта нормалей были перепутаны местами\. Поскольку эти текстуры используются для разных целей, они не являются взаимозаменяемыми\. В конечном итоге эта ошибка может привести к заметным визуальным дефектам\. ### Ошибка из\-за неправильного представления о приоритете операции '??' ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/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](https://github.com/oculus-samples/Unity-NorthStar/blob/d7c4f7c61f0db96d006df3ed52a0c3268881bf7e/Assets/NorthStar/Scripts/Player/SyntheticHandRetargetingProcessor.cs#L89) Анализатор обнаружил выражение которое может быть некорректным: ```cpp t.rotation = pose.rotation * correctionQuaternion ?? Quaternion.identity; ``` Чтобы лучше понять потенциальную проблему, давайте рассмотрим ход вычисления этого выражения в случае, если `correctionQuaternion` равен `null`: 1. Сначала будет выполнена перегруженная операция умножения\. Если один из операндов `null`, в результате также будет `null`; 1. Далее будет выполнена проверка результата предыдущий операции с помощью `??`\. В результате `t.rotation` будет присвоено значение `Quaternion.identity`\. `Quaternion.identity`, по сути, означает отсутствие какого\-либо вращения\. Это значит, что в рассматриваемом выше случае поворот некоторого объекта будет резко обнулён\. Вряд ли это является ожидаемым поведением, т\.к\. резкий сброс поворота объекта в большинстве случаев выглядит некорректно\. Куда вероятнее, что проверить с помощью `??` хотели именно `correctionQuaternion`, но разработчик не учёл, что приоритет у `??` ниже, чем у `*`\. В этом случае исправленный вариант этого выражения выглядит так: ```cpp t.rotation = pose.rotation * (correctionQuaternion ?? Quaternion.identity); ``` Теперь в случае, если `correctionQuaternion` равен `null`, вращению объекта будет присвоен результат выражения `pose.rotation * Quaternion.identity`\. Т\.к\. операция умножения между двумя кватернионами, по сути, выполняет поворот первого кватерниона на вращение, заданное вторым, результатом этой операции будет просто `pose.rotation`\. ## Заключение В этой статье мы познакомились с ещё одним интересным open source VR\-проектом NorthStar, реализованным на Unity\. Кроме того, немного познакомились и с его исходным кодом, рассмотрев найденные с помощью PVS\-Studio ошибки\. Если у вас есть собственный проект, возможно, вам интересно, а смог бы анализатор найти какие\-либо ошибки в нём? В этом случае вы можете воспользоваться бесплатным пробным периодом, запросив ключ [тут](https://pvs-studio.ru/ru/pvs-studio/try-free/)\. Туториал по запуску первого анализа Unity\-проекта найти можно [здесь](https://pvs-studio.ru/ru/docs/manual/6607/)\. А на этом у меня всё\. До встречи в следующих статьях\!