# Исходный код на прощание: разбор ошибок в проектах закрывшейся инди\-студии Инди\-студия StarlightLabsCo закрылась, а разработчик и её основатель Харрис Ротаермель опубликовал исходный код своих проектов\. Теперь желающие могут улучшать эти проекты или разрабатывать на их основе новые\. Мы решили не оставаться в стороне и решили поискать ошибки в его проекте\. ![1169_StarlightLabsCo_ru/image1.png](https://import.viva64.com/docx/blog/1169_StarlightLabsCo_ru/image1.png) Создатель и основатель инди\-студии StarlightLabsCo Харрис Ротаермель решил закрыть студию и опубликовать [исходный код](https://github.com/orgs/StarlightLabsCo/repositories) проектов на GitHub\. Это достаточно редкое явление\. Обычно студии закрываются, и исходный код проектов не опубликовывается\. Хотя иногда исходники могут всплывать через десятилетия от разработчиков\. Харрис же решил поступить благородно и любезно поделиться своими проектами\. Студия была создана для разработки игр с применением моделей машинного обучения для NPC и генерации сценариев\. Именно на такую новость я наткнулся в интернете\. Мне сразу же стало интересно что за проекты там такие\. И тут я столкнулся с профессиональной деформацией во всей своей красе\. Как? Сейчас расскажу\. Я занимаюсь разработкой статического анализатора [PVS\-Studio](https://pvs-studio.ru/ru/)\. А конкретно руковожу C\# направлением\. Если кто\-то не знаком с термином статический анализ, то расскажу кратко: статический анализ кода — это процесс, направленный на обнаружение недочётов, ошибок и потенциальных уязвимостей в исходном коде программ\. Статический анализ можно рассматривать как автоматизированный code review\. И, конечно, PVS\-Studio может анализировать игровые проекты\. В том числе написанные с использованием Unity и Unreal Engine\. Возвращаясь к вопросу профессиональной деформации\. Я увидел новость о том, что студия опубликовала исходники своей игры\. И первым делом я запустил анализатор кода для поиска ошибок в коде\. Я решил проанализировать игру Starlight, которая написана на C\# с использованием Unity\. Это игра, в которой каждый персонаж обладает уникальной биографией, своим мнением\. Игроки могут общаться с героями, а ответы генерируются с помощью языковой модели\. ![1169_StarlightLabsCo_ru/image2.png](https://import.viva64.com/docx/blog/1169_StarlightLabsCo_ru/image2.png) К сожалению, очень мало игр имеют открытый исходный код, а на C\# таких вообще единицы\. Я очень надеялся на выход игры [Gigaya от Unity](https://unity.com/ru/blog/games/introducing-unitys-latest-sample-game-gigaya) с открытым исходным кодом\. Она должна была стать игрой\-образцом для обучения разработке игр\. Но, увы, она была отменена\. Кстати говоря, Харрис отметил, что он не приводил код в порядок перед публикацией\. От того ещё интереснее посмотреть, что же хранится под капотом\. ## Ошибки и странный код Давайте приступим к разбору того, что выявил анализатор\. Сразу хочу отметить, что проект небольшой и относится к категории инди\-игр, поэтому его кодовая база не слишком обширная\. В таких проектах плотность ошибок сильно ниже, поэтому и анализатор обнаружил не так много проблем\. Однако цель не в том, чтобы найти как можно больше ошибок, а в том, чтобы улучшить проект, как и хотел разработчик\. **Issue 1** ```cpp public override char Validate(ref string text, ref int pos, char ch) { Debug.Log("Trying to validate..."); // Return unless the character is a valid digit if (ch < '0' && ch > '9') return (char)0; .... } ``` Предупреждение PVS\-Studio: [V3022](https://pvs-studio.ru/ru/docs/warnings/v3022/) Expression 'ch < '0' && ch \> '9'' is always false\. Probably the '\|\|' operator should be used here\. TMP\_PhoneNumberValidator\.cs 20 Начнём с простой ошибки\. В приведённом фрагменте кода выполняется проверка на валидность параметра _ch_\. Только эта проверка неправильная\. Приведём условие в более понятный вид: ```cpp ch < 48 && ch > 57 ``` Вместо оператора '&&' следовало использовать '\|\|' из\-за того, что два условия противоречат друг другу\. Сейчас же проверка является всегда ложной\. **Issue 2** А сейчас будет небольшой [межпроцедурный](https://pvs-studio.ru/ru/blog/terms/6785/) кейс\. ```cpp protected override IEnumerable ScanInternal () { .... TriangleMeshNode.SetNavmeshHolder(AstarPath.active.data.GetGraphIndex(this), this); .... } ``` Предупреждение PVS\-Studio: [V3106](https://pvs-studio.ru/ru/docs/warnings/v3106/) The 1st argument 'AstarPath\.active\.data\.GetGraphIndex\(this\)' is potentially used inside method to point beyond collection's bounds\. NavMeshGenerator\.cs 225 Анализатор сообщает о том, что внутри метода _TriangleMeshNode\.SetNavmeshHolder_ происходит доступ по индексу за пределами допустимого диапазона\. При этом в качестве индекса выступает первый аргумент\. Взглянем на метод _AstarPath\.active\.data\.GetGraphIndex_: ```cpp public int GetGraphIndex (NavGraph graph) { if (graph == null) throw new System.ArgumentNullException("graph"); var index = -1; if (graphs != null) { index = System.Array.IndexOf(graphs, graph); if (index == -1) Debug.LogError("Graph doesn't exist"); } return index; } ``` Как мы видим, метод может вернуть в качестве результата \-1\. Теперь взглянем на метод _TriangleMeshNode\.SetNavmeshHolder_: ```cpp public static void SetNavmeshHolder (int graphIndex, INavmeshHolder graph) { // We need to lock to make sure that // the resize operation is thread safe lock (lockObject) { if (graphIndex >= _navmeshHolders.Length) { var gg = new INavmeshHolder[graphIndex+1]; _navmeshHolders.CopyTo(gg, 0); _navmeshHolders = gg; } _navmeshHolders[graphIndex] = graph; // <= } } ``` Анализатор сообщает, что проблемный доступ происходит на последней строке\. Доступ по индексу происходит безусловно\. **Issue 3** ```cpp void ON_TEXT_CHANGED(Object obj) { if (obj = m_TextComponent) hasTextChanged = true; } ``` Предупреждение PVS\-Studio: [V3137](https://pvs-studio.ru/ru/docs/warnings/v3137/) The 'obj' variable is assigned but is not used by the end of the function\. VertexShakeB\.cs 44 Сложно сказать является ли присваивание внутри условия ошибкой, кажется, что да\. Но что точно можно сказать, так это то, что _obj_ дальше никак не используется\. Соответственно, присваивание не имеет смысла\. **Issue 4** ```cpp void DrawWordBounds() { topLeft = m_Transform.TransformPoint(new (topLeft.x, maxAscender, 0)); bottomLeft = m_Transform.TransformPoint(new (bottomLeft.x, minDescender, 0)); bottomRight = m_Transform.TransformPoint(new (currentCharInfo.topRight.x, minDescender, 0)); topRight = m_Transform.TransformPoint(new (currentCharInfo.topRight.x, maxAscender, 0)); } ``` Предупреждение PVS\-Studio: [V3127](https://pvs-studio.ru/ru/docs/warnings/v3127/) Two similar code fragments were found\. Perhaps, this is a typo and 'bottomRight' variable should be used instead of 'topRight' TMP\_TextInfoDebugTool\.cs 313 А это подозрительный код и кандидат на ошибку\. Обратите внимание на третье присваивание и первый аргумент конструктора структуры _Vector3_\. Там используется _currentCharInfo\.topRight\.x_ как и в четвёртом присваивании\. Потенциально нужно использовать_ currentCharInfo\.bottomRight\.x\._ **Issue 5** ```cpp public override NNInfoInternal GetNearestForce (....) { .... for (int w = 0; w < wmax; w++) { if (bestDistance < (w-2)*Math.Max(TileWorldSizeX, TileWorldSizeX)) break; } } ``` Предупреждение PVS\-Studio: [V3038](https://pvs-studio.ru/ru/docs/warnings/v3038/) The 'TileWorldSizeX' argument was passed to 'Max' method several times\. It is possible that other argument should be passed instead\. NavmeshBase\.cs 479 Методу _Math\.Max_ передаётся _TileWorldSizeX_ в качестве первого и второго аргумента\. Этот метод должен возвращать большее из двух переданных значений\. Получается, что он будет возвращать всегда одно и то же значение\. ## Микрооптимизации Также в PVS\-Studio есть [диагностические правила](https://pvs-studio.ru/ru/docs/warnings/#MicroOptimizationsCS), направленные на оптимизацию кода для Unity проектов\. Анализатор находит в программе чувствительный к производительности контекст и предупреждает о тяжёлых операциях\. В этом проекте было выдано 3 таких предупреждения\. ```cpp protected void Update() { .... if ( WebSocketClient.Instance.websocket.State == WebSocketState.Open && !this.IsPlayerControlled) { observedEntities = Utilities.UpdateObservedEntities(this, // <= observedEntities, transform, 5f); } .... } ``` Предупреждение PVS\-Studio: [V4003](https://pvs-studio.ru/ru/docs/warnings/v4003/) Avoid capturing variables in performance\-sensitive context inside the 'UpdateObservedEntities' method\. This can lead to decreased performance\. Character\.cs 168 Анализатор предупреждает о том, что в методе _Utilities\.UpdateObservedEntities_ происходит захват переменных, что является тяжёлой операцией\. При этом метод_ Utilities\.UpdateObservedEntities_ вызывается в методе _Update_, который отрабатывает каждый кадр\. Этот метод и будет являться чувствительным к производительности контекстом\. ```cpp public static HashSet UpdateObservedEntities(Character character, ....) { .... HashSet newEntitiesInRange = new(collidersInRange .Select(gameObject => gameObject.GetComponent()) .Where(entity => entity != null && entity != character)); // <= .... } ``` А вот и сам захват переменных в лямбда\-выражении\. В данном случае захватывается параметр _character_\. Захват переменных может приводить к снижению производительности из\-за дополнительного выделения памяти\. Таким образом, представленный участок кода создаёт дополнительную нагрузку на GC\. Посмотрим ещё один кейс\. ```cpp void LateUpdate() { m_isHoveringObject = false; if (TMP_TextUtilities .IsIntersectingRectTransform(m_TextMeshPro.rectTransform, Input.mousePosition, Camera.main)) // <= { m_isHoveringObject = true; } if (m_isHoveringObject) { #region Example of Character Selection int charIndex = TMP_TextUtilities .FindIntersectingCharacter(m_TextMeshPro, Input.mousePosition, Camera.main, // <= true); .... int wordIndex = TMP_TextUtilities .FindIntersectingWord(m_TextMeshPro, Input.mousePosition, Camera.main); // <= .... } } ``` Предупреждение PVS\-Studio: [V4005](https://pvs-studio.ru/ru/docs/warnings/v4005/) Expensive operation is performed inside the 'Camera\.main' property\. Using such property in performance\-sensitive context can lead to decreased performance\. TMP\_TextSelector\_A\.cs 34 Здесь чувствительным к производительности контекстом является метод _LateUpdate_\. Он вызывается каждый кадр\. Согласно [документации Unity](https://docs.unity3d.com/ScriptReference/Camera-main.html), ряд методов и свойств из API Unity производят ресурсоёмкие операции при обращении\. В данном случае происходит множественное обращение к свойству _Camera\.main_\. При обращении к свойству _Camera\.main_ осуществляется поиск в кэше, что создаёт нагрузку на процессор\. Множественного обращения к свойству можно избежать, если записать его значение в переменную\. Также анализатор нашёл похожий случай, но с вызовом _GetComponent_\. Его я показывать не буду, так как он похож по смыслу на остальные\. ## Заключение Данный случай достаточно редок, поэтому мне было очень интересно взглянуть на код игры, хоть и от небольшой инди\-студии\. Плюс ещё получилось найти ошибки в проекте\. Под конец порекомендую вам несколько интересных статей по игровой тематике от PVS\-Studio: * [Повторная проверка Unity статическим анализатором PVS\-Studio](https://pvs-studio.ru/ru/blog/posts/csharp/0932/) * [Зачем разработчикам игр на Unity использовать статический анализ?](https://pvs-studio.ru/ru/blog/posts/csharp/0945/) * [30 лет DOOM: новый код — новые баги](https://pvs-studio.ru/ru/blog/posts/cpp/1087/) * [Как найти работу для фиксиков: анализируем Godot Engine](https://pvs-studio.ru/ru/blog/posts/cpp/1150/) Удачи и всем пока\!