Возвращаемся на Гроув-Стрит. Анализ движка Grand Theft Auto: San Andreas на Unity

• О проекте
• Использование PVS-Studio в Unity
• Наводим порядок на улицах Лос-Сантоса
  • NullReferenceException
  • Оптимизация
  • Различные проблемы
• Заключение

Серия Grand Theft Auto стала культовой в игровой индустрии, а San Andreas — одной из самых запоминающихся частей для многих игроков. Время проходит, но фанаты всё так же преданы игре. Кто-то достаёт старый и пыльный диск с ностальгией, а некоторые заходят ещё дальше. Сегодня проверим фанатский перенос GTA: San Andreas на движок Unity с помощью статического анализатора PVS-Studio!

О проекте

SanAndreasUnity — это Open Source проект, который переносит Grand Theft Auto: San Andreas в Unity! Новый движок открывает возможности для энтузиастов расширять и модифицировать игру намного сильнее, чем позволяла оригинальная игра от Rockstar Games. Помимо этого, добавлена кроссплатформенность — теперь игру можно запустить на смартфоне, консоли и даже в VR.

Проект написан на C#, создавался фанатами и поддерживается до сих пор. Проект доступен в репозитории на GitHub.

Использование PVS-Studio в Unity

Но перед разбором ошибок давайте узнаем — как проверить проект на Unity?

Во-первых, самое важное

У вас должен быть установлен PVS-Studio :)

Если же вы ещё этого не сделали, то в этом вам поможет данная страница.

Откройте проект в Unity

Затем необходимо установить в настройках Unity предпочитаемый редактор скриптов.

Это можно сделать с помощью параметра "External Script Editor" на вкладке "External Tools" в окне "Preferences".

Чтобы открыть это окно, используйте опцию меню "Edit" -> "Preferences" в редакторе Unity:

После этого можно открывать свой проект в выбранной IDE, используя опцию "Assets" -> "Open C# Project" в редакторе Unity.

Запустите анализ в IDE

Я буду использовать Visual Studio 2022. Чтобы проанализировать проект в данной версии IDE, можно использовать пункт меню "Extensions" -> "PVS-Studio" -> "Check Solution":

Всё! Анализ кода будет запущен, и после этого вы сможете начать работу с предупреждениями анализатора.

Более подробно анализ проектов на Unity описан в документации.

Наводим порядок на улицах Лос-Сантоса

NullReferenceException

Один из самых частых врагов разработчиков, главное вовремя его найти.

Issue 1

Иногда ошибка подкрадывается из самых неожиданных мест.

Рассмотрим следующий код:

public static void RegisterPrefab(GameObject prefab, Guid newAssetId,
  SpawnHandlerDelegate spawnHandler, UnSpawnDelegate unspawnHandler)
{
  if (newAssetId == Guid.Empty) 
  {
     Debug.LogError($"Could not register handler for '{prefab.name}' 
       with new assetId because the new assetId was empty");
     return;
  }
  if (prefab == null) 
  {
    Debug.LogError("Could not register handler for prefab 
      because the prefab was null");
    return;
  }
  ....
  if (spawnHandler == null) 
  {
    Debug.LogError($"Can not Register null SpawnHandler for {assetId}");
    return;
  }
  if (unspawnHandler == null) 
  {
    Debug.LogError($"Can not Register null UnSpawnHandler for {assetId}");
    return;
  }
  ....
}

В фрагменте представлена стандартная практика проверки параметров метода на null. Но даже такое очевидно полезное действие может создать проблемы (иронично, да?).

Рассмотрим первые две проверки:

public static void RegisterPrefab(GameObject prefab, Guid newAssetId, ....)
{
  if (newAssetId == Guid.Empty)
  {
    Debug.LogError($"Could not register handler for '{prefab.name}'
      with new assetId because the new assetId was empty");
    return;
  }
  if (prefab == null)
  {
    Debug.LogError("Could not register handler for prefab 
      because the prefab was null");
    return;
  }
  ....
}

Предупреждение PVS-Studio: V3095 The 'prefab' object was used before it was verified against null. Check lines: 669, 673. NetworkClient.cs 669

В проверке параметра newAssetId происходит обращение к полю name параметра prefab, при этом проверка параметра prefab идёт уже после. Неправильный порядок проверки на null может привести к выбросу исключения NullReferenceException.

Ошибки в обработчиках ошибок могут быть опаснее 5-звездочной погони, ведь их легко пропустить. В данном случае не поможет даже HESOYAM или BAGUVIX, но надежда ещё есть — это статический анализ кода.

Issue 2

Рассмотрим следующий подозрительный фрагмент:

static void RebuildObserversCustom(NetworkIdentity identity, bool initialize)
{
  newObservers.Clear();
  if (identity.visible != Visibility.ForceHidden)
  {
    aoi.OnRebuildObservers(identity, newObservers);
    }
  ....
  // this code is from UNET, it's a bit strange but it works:
  if (initialize)
  {
    if (!newObservers.Contains(localConnection))
    {
      if (aoi != null)
          aoi.SetHostVisibility(identity, false);
    }
  }
}

Предупреждение PVS-Studio: V3095 The 'aoi' object was used before it was verified against null. Check lines: 1464, 1546. NetworkServer.cs 1464

Фрагмент частично напоминает прошлый пример: к aoi обращаются до проверки на null. Конечно, есть шанс, что код не приводит к возникновению NullReferenceException, но статический анализ должен подмечать и подозрительные места.

Как минимум, код странный. Это также подтверждает комментарий разработчиков из фрагмента:

"this code is from UNET, it's a bit strange but it works"

Оптимизация

Одна из новых особенностей нашего C# анализатора – это диагностики оптимизации для Unity проектов!

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

Рассмотрим несколько фрагментов кода, которые могут привести к снижению производительности:

Issue 3

private void Update()
{
  if (!Loader.HasLoaded)
    return;

  _updateTimeLimitStopwatch.Restart();

  this.FocusPointManager.Update(); // <=
  ....
}

Особенность метода Update в том, что он выполняется каждый кадр. А если поместить в часто выполняемый метод код, который будет дополнительно нагружать систему, то... Попробуйте догадаться сами :)

Взглянем на this.FocusPointManager.Update():

public void Update()
{
  double timeNow = Time.timeAsDouble;

  UnityEngine.Profiling.Profiler.BeginSample("Update focus points");
  this._focusPoints.RemoveAll(f => {....});                  // <=
  UnityEngine.Profiling.Profiler.EndSample();

  bool hasElementToRemove = false;
  _focusPointsToRemoveAfterTimeout.ForEach(_ => {....});     // <=

  if (hasElementToRemove)
    _focusPointsToRemoveAfterTimeout.RemoveAll(_ => {....}); // <=
}

Предупреждение PVS-Studio: V4003 Avoid capturing variables in performance-sensitive context inside the 'Update' method. This can lead to decreased performance. Cell.cs 427

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

При каждом замыкании создаётся экземпляр класса замыкания, а в его поля помещаются захваченные значения. Так как экземпляр класса располагается в куче, это создаёт дополнительную нагрузку на GC.

В данном случае одним из возможных решений будет воспользоваться собственной реализацией, аналогичной RemoveAll и ForEach. Это позволит избежать дополнительного выделения памяти и снизить нагрузку на сборщик мусора.

Issue 4

void OnGUI()
{
  if (!statisticsGUI) return;

  GUILayout.BeginArea(new Rect(5, 110, 300, 300));

  if (ServerActive())
  {
    ....
    GUILayout.Label($"connections: {server.connections.Count}");
    GUILayout.Label($"SendQueue: {GetTotalSendQueue()}");
    GUILayout.Label($"ReceiveQueue: {GetTotalReceiveQueue()}");
  }
  ....
}

Предупреждение PVS-Studio: V4001 Boxing usage inside the frequently called 'OnGUI' method may decrease performance. KcpTransport.cs 281

В данном случае при вызове GUILayout.Label в некоторых местах происходит упаковка при передаче значений типа int или long. С одной стороны, влияние на производительность может быть не сильным, но с другой, решение очень простое — добавить ToString и избежать лишней упаковки.

Issue 5

private void LateUpdate()
{
  ....
  if (ped != null)
    this.FocusPos = ped.transform.position;

  else if (Camera.main != null)
    this.FocusPos = Camera.main.transform.position;
  ....
  float relAngle = Camera.main != null ?
    Camera.main.transform.eulerAngles.y : 0f;
....
}

Предупреждение PVS-Studio: V4005 Expensive operation is performed inside the 'Camera.main' property. Using such property in performance-sensitive context can lead to decreased performance. MiniMap.cs 190

Анализатор нашёл данный участок кода с помощью новой диагностики. Она ещё на этапе разработки, но уже способна находить возможности для оптимизации!

Проблема заключается в многократном использовании Camera.main, что приводит к повышению нагрузки на процессор.

Правильным подходом в данном случае будет создание дополнительной переменной, в которую запишется возвращаемое значение свойства Camera.main. В дальнейшем можно будет обращаться к этой переменной.

Различные проблемы

Issue 6-7

Концовка подкачала. Рассмотрим следующий фрагмент:

private void OnDrawGizmosSelected()
{
  ....
  var vehicles = FindObjectsOfType<Vehicle>()
                .Where(....)
                .OrderBy(....)
                .ToArray();
  foreach (var vehicle in vehicles)
  {
    foreach (var seat in vehicle.Seats){....}
    var closestSeat = vehicle.GetSeatAlignmentOfClosestSeat(transform.position);
    if (closestSeat != Vehicle.SeatAlignment.None){....}
    break;
  }
}

Предупреждение PVS-Studio: V3020 An unconditional 'break' within a loop. PlayerController.cs 258

Простая и одновременно серьёзная ошибка — break при любых обстоятельствах на первой итерации цикла. В итоге из всей коллекции vehicles в цикле будет задействован лишь первый элемент.

Ещё один схожий фрагмент:

public override void ClientLateUpdate()
{
  while (reliableClientToServer.Count > 0)
  {
    QueuedMessage message = reliableClientToServer[0];
    if (message.time <= Time.unscaledTime)
    {
      wrap.ClientSend(new ArraySegment<byte>(message.bytes), Channels.Reliable);
      reliableClientToServer.RemoveAt(0);
    }
    break;
  }
  ....
}

Предупреждение PVS-Studio: V3020 An unconditional 'break' within a loop. LatencySimulation.cs 220

Как и в прошлом фрагменте — break является безусловным и произойдёт на первой итерации.

Данный случай отличается тем, что тут используется while, и практически идентичные фрагменты встречаются ещё четыре раза. Возможно, разработчик так и задумал, но комментариев или указывающего на это контекста нет. Странное это место или ошибочное, пусть решают разработчики, а нам остаётся лишь указать на подозрительный момент.

Issue 8

Классический паттерн ошибок, отлавливаемый статическими анализаторами:

public void SetString(string key, string value)
{
  if (m_syncDictionary.TryGetValue(key, out string existingValue))
  {
    if (value != existingValue)
    {
      m_syncDictionary[key] = value;
    }
  }
  m_syncDictionary[key] = value;
}

Предупреждение PVS-Studio: V3008 The 'm_syncDictionary[key]' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 112, 108. SyncedBag.cs 112

При прохождении двух условий происходит присваивание m_syncDictionary[key]. А если условие не проходит... то будет то же самое присваивание! Возможно, что одно из них должно отличаться.

Заключение

Несмотря на все найденные недочёты в коде, разработчики молодцы, и код оказался качественным. Стоит учитывать, что при разработке они старались повторить логику оригинальной игры (со всеми костылями и проблемами). Комьюнити проекта преданы своему второму дому — Гроув-стриту. Nice work!

Если вам интересны статьи про проверку игровых проектов, предлагаю ознакомиться с нашим блогом.

А если вас заинтересовала возможность проверить Unity-проект с помощью PVS-Studio, скачать и попробовать анализатор можно здесь.