Инди-студия StarlightLabsCo закрылась, а разработчик и её основатель Харрис Ротаермель опубликовал исходный код своих проектов. Теперь желающие могут улучшать эти проекты или разрабатывать на их основе новые. Мы решили не оставаться в стороне и решили поискать ошибки в его проекте.

Создатель и основатель инди-студии StarlightLabsCo Харрис Ротаермель решил закрыть студию и опубликовать исходный код проектов на GitHub. Это достаточно редкое явление. Обычно студии закрываются, и исходный код проектов не опубликовывается. Хотя иногда исходники могут всплывать через десятилетия от разработчиков. Харрис же решил поступить благородно и любезно поделиться своими проектами.

Студия была создана для разработки игр с применением моделей машинного обучения для NPC и генерации сценариев.

Именно на такую новость я наткнулся в интернете. Мне сразу же стало интересно что за проекты там такие. И тут я столкнулся с профессиональной деформацией во всей своей красе. Как? Сейчас расскажу.

Я занимаюсь разработкой статического анализатора PVS-Studio. А конкретно руковожу C# направлением. Если кто-то не знаком с термином статический анализ, то расскажу кратко:

статический анализ кода — это процесс, направленный на обнаружение недочётов, ошибок и потенциальных уязвимостей в исходном коде программ. Статический анализ можно рассматривать как автоматизированный code review. И, конечно, PVS-Studio может анализировать игровые проекты. В том числе написанные с использованием Unity и Unreal Engine.

Возвращаясь к вопросу профессиональной деформации. Я увидел новость о том, что студия опубликовала исходники своей игры. И первым делом я запустил анализатор кода для поиска ошибок в коде. Я решил проанализировать игру Starlight, которая написана на C# с использованием Unity. Это игра, в которой каждый персонаж обладает уникальной биографией, своим мнением. Игроки могут общаться с героями, а ответы генерируются с помощью языковой модели.

К сожалению, очень мало игр имеют открытый исходный код, а на C# таких вообще единицы. Я очень надеялся на выход игры Gigaya от Unity с открытым исходным кодом. Она должна была стать игрой-образцом для обучения разработке игр. Но, увы, она была отменена. Кстати говоря, Харрис отметил, что он не приводил код в порядок перед публикацией. От того ещё интереснее посмотреть, что же хранится под капотом.

Ошибки и странный код

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

Issue 1

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 Expression 'ch < '0' && ch > '9'' is always false. Probably the '||' operator should be used here. TMP_PhoneNumberValidator.cs 20

Начнём с простой ошибки. В приведённом фрагменте кода выполняется проверка на валидность параметра ch. Только эта проверка неправильная. Приведём условие в более понятный вид:

ch < 48 && ch > 57

Вместо оператора '&&' следовало использовать '||' из-за того, что два условия противоречат друг другу. Сейчас же проверка является всегда ложной.

Issue 2

А сейчас будет небольшой межпроцедурный кейс.

protected override IEnumerable<Progress> ScanInternal () 
{
  ....
  TriangleMeshNode.SetNavmeshHolder(AstarPath.active.data.GetGraphIndex(this),
                                    this);
  ....
}

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

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:

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:

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

void ON_TEXT_CHANGED(Object obj)
{
    if (obj = m_TextComponent)
        hasTextChanged = true;
}

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

V3137 The 'obj' variable is assigned but is not used by the end of the function. VertexShakeB.cs 44

Сложно сказать является ли присваивание внутри условия ошибкой, кажется, что да. Но что точно можно сказать, так это то, что obj дальше никак не используется. Соответственно, присваивание не имеет смысла.

Issue 4

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 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

public override NNInfoInternal GetNearestForce (....) 
{
  ....
  for (int w = 0; w < wmax; w++) {
    if (bestDistance < (w-2)*Math.Max(TileWorldSizeX, TileWorldSizeX)) break;
  }
}

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

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 есть диагностические правила, направленные на оптимизацию кода для Unity проектов. Анализатор находит в программе чувствительный к производительности контекст и предупреждает о тяжёлых операциях. В этом проекте было выдано 3 таких предупреждения.

protected void Update()
{
  ....
  if (   WebSocketClient.Instance.websocket.State == WebSocketState.Open
      && !this.IsPlayerControlled)
  {
    observedEntities = Utilities.UpdateObservedEntities(this,       // <=
                                                        observedEntities,
                                                        transform,
                                                        5f);
  }
  ....
}

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

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, который отрабатывает каждый кадр. Этот метод и будет являться чувствительным к производительности контекстом.

public static HashSet<Entity> UpdateObservedEntities(Character character, ....)
{
  ....
  HashSet<Entity> newEntitiesInRange = new(collidersInRange
    .Select(gameObject => gameObject.GetComponent<Entity>())
    .Where(entity => entity != null && entity != character));    // <=
  ....
}

А вот и сам захват переменных в лямбда-выражении. В данном случае захватывается параметр character. Захват переменных может приводить к снижению производительности из-за дополнительного выделения памяти. Таким образом, представленный участок кода создаёт дополнительную нагрузку на GC.

Посмотрим ещё один кейс.

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 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, ряд методов и свойств из API Unity производят ресурсоёмкие операции при обращении. В данном случае происходит множественное обращение к свойству Camera.main. При обращении к свойству Camera.main осуществляется поиск в кэше, что создаёт нагрузку на процессор. Множественного обращения к свойству можно избежать, если записать его значение в переменную.

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

Заключение

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

• Повторная проверка Unity статическим анализатором PVS-Studio

• Зачем разработчикам игр на Unity использовать статический анализ?

• 30 лет DOOM: новый код — новые баги

• Как найти работу для фиксиков: анализируем Godot Engine

Удачи и всем пока!

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Artem Rovenskii. The last source code: digging into bugs in projects after indie game studio shuts down.