Как не стоит делать игры на Unity

Много статей написано про хороший и плохой код, но статей с разборами проблем реального кода очень мало (за исключением багов в open source проектах), поэтому решил показать проблемы в реальной игре на Unity.

Оригинальная версия игры вышла в 2016 году на японском, в 2023 на английском, сделана на Unity 5.1.4.
Для декомпиляции использовались dnSpy, ILSpy, dotPeek. Без ошибок не справился ни один, dotPeek оказался хуже всех, но в данном случае это говорит о качестве кода, а не о недостатках инструментов.

Некорректные элементы Enum

public enum EquipIndex { Main Weapon, Sidearm, Headgear, Torso, Arms, Legs, Accessory, Food }

У элемента «Main Weapon» посередине пробел, потому что EquipIndex связан с отображаемым игроку текстом.
Такой код не скомпилируется без специальных трюков, и локализация намертво прибита к коду, поэтому для каждого языка потребуется отдельная версия кода и исправление ошибок во всех версиях станет заметной проблемой.

Решение: в данных хранить значение Enum, а не имя, для локализации максимум использовать элементы как ключи.

Отключение EventSystem

Например так:

EventSystem.current.gameObject.SetActive(false); // анимация или сохранение настроек EventSystem.current.gameObject.SetActive(true);

С сохранением настроек и возникли проблемы: если зайти в настройки звука или боя и выйти ничего не поменяв, то при сохранение бросается NullReferenceException и игра «повисает» из-за отключенной EventSystem. И единственное решение это перезапуск игры, хотя критической ошибки нет.

Само сохранение настроек, когда в них ничего не поменялось, достаточно спорная вещь.
(Замечание: если хоть один параметр в настройках поменять, то всё работает без проблем.)

Решение: использовать CanvasGroup.interactable для переключения ограниченной группы элементов.

Ненужные coroutine в анимации интерфейса

private IEnumerator ShowMotion() { GetComponent<RectTransform>().DOAnchorPos(new Vector2(-300f, 0f), 0.5f).SetEase(Ease.OutCubic); yield return new WaitForSeconds(0.5f); // завершающий код }

Решение: использовать метод OnComplete, который вызывается при завершении анимации:

private void ShowMotion() { GetComponent<RectTransform>().DOAnchorPos(new Vector2(-300f, 0f), 0.5f).SetEase(Ease.OutCubic).OnComplete(() => завершающий код); }

Дополнительные проблемы:

• полсекунды на закрытие текущей панели, потом полсекунды на открытие новой это очень долго

• длительность анимации это магическая константа, если понадобится ускорить анимацию, то менять придется везде

• в некоторых местах длительность DOTween анимации и длительность ожидания (WaitForSeconds) разные, уверен что это ошибка и они должны быть одинаковыми

Интерфейсы разделены по сценам

Панель базы на одной сцене, панель хранилища на другой, крафта на третьей, магазин на четвертой, и т.д. Загрузка этих сцен не аддитивная, получается 0,5с за анимацию закрытия текущего интерфейса, загрузка сцены, и еще 0,5с на анимацию открытия нового. Переключаться приходится часто и долгое ожидание игроков раздражает.

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

Множественные обработчики событий на кнопках, повешенные в редакторе

На одно событие добавлено по 3-4 обработчика, для тестирования и прототипирования такое использовать можно, но в рабочем проекте это доставит проблем как минимум при рефакторинге и поиске использований.

Cмешивание бизнес-логики и представления

Код перемещения предмета из хранилища в инвентарь:

public void OnEndDragOutbox(coreButtonEventData ev) { if (ev.selectTargetDrop == null || (!ev.selectTargetDrop.name.Equals("ItemBox") && ev.selectTargetDrop.name.IndexOf("inv") != 0)) { return; } itemCView component = ev.selectTarget.GetComponent<itemCView>(); if (inventoryControl.itemMaxCount(1) + 1 <= gameDatas.Instance.userData.GuildData.FGRMaxCount(1)) { if (component.itemData.count[2] < 0) { component.itemData.count[2] = 0; } if (component.itemData.category == 23) { List<int> count; List<int> list = (count = component.itemData.count); int index; int index2 = (index = 1); index = count[index]; list[index2] = index + component.itemData.count[2]; } else { component.itemData.count[1] = component.itemData.count[2]; } component.itemData.count[2] = 0; if (component.itemData.count[1] > 999) { component.itemData.count[1] = 999; } OnDrawUpdate(); DrawUpdateHandler.CallDrawUpdate("ItemPanelGuild"); } }

Решение: вынести код перемещения предмета в отдельный метод MoveToInventory() и упростить метод.

public void OnEndDragOutbox(coreButtonEventData ev) { if (ev.selectTargetDrop == null || (!ev.selectTargetDrop.name.Equals("ItemBox") && ev.selectTargetDrop.name.IndexOf("inv") != 0)) { return; } itemCView component = ev.selectTarget.GetComponent<itemCView>(); if (component.itemData.MoveToInventory()) { OnDrawUpdate(); DrawUpdateHandler.CallDrawUpdate("ItemPanelGuild"); } }

Неочевидные назначения полей

В коде выше используется поле List<int> count, в котором всегда три элемента. Индекс 2 это количество предметов в хранилище, 1 в инвентаре, а использование индекса 0 найти не удалось.

Решение: использовать два отдельных поля, или если необходима совместимость, то скрыть count и заменить свойствами, или хотя бы использовать именованные константы вместо 1 и 2.

const int STORAGE = 2; const int INVENTORY = 1;  private int[] count;  public int QuantityStorage { get => count[STORAGE];  set => count[STORAGE] = value; }  public int QuantityInventory { get => count[INVENTORY];  set => count[INVENTORY] = value; }

Два публичных поля с одним смыслом

У класса предмета для поля name и unique. Оба отвечают за название предмета, только name это базовое неизменяемое название, а unique это полное название для предметов с суффиксом и префиксом.

Из-за этого была ошибка, когда при продаже простых предметов появлялось сообщение » was sold» без названия предмета.

base.informationLogControl.QueueDialog(string.Format(" was sold.", this.SallItem.unique));

Решение: сделать поля скрытыми и вместо них оставить одно свойство.

public string Name { get => string.IsNullOrEmpty(unique) ? name : unique;  set => unique = value; }

Нестабильная сортировка предметов

Для сортировки используется List<T>.Sort(comparer), в документации к которому прямо говорится что сортировка нестабильная.
В результате при покупке, продаже или перемещении предметов порядок скачет. При тестировании такое выявить сложно, потому что при небольшом количестве элементов используется стабильная сортировка.

Решение: использовать стабильную сортировка, сделать её из нестабильной дело пяти минут с незначительным оверхедом по производительности.

Отсутствие прокрутки списков

Отображение списков реализовано просто: фиксированное количество элементов без использования ScrollRect отображает часть элементов из списка. Но вот переключение между страницами только по нажатию кнопок.

Решение: добавить реализацию интерфейс IScrollHandler с методом OnScroll() для смены страниц.

public void OnScroll(PointerEventData eventData) { PageObject2.pageMove(eventData.scrollDelta.y < 0 ? +1 : -1); }

Дублирование кода

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

Решение: оставить один класс, а события (перетаскивание, клики и т.д.) перенаправлять родительской панели.

Экономия на символах

В корутинах часто используется

yield return 0;

0 короче null на три символа, но создает боксинг на пустом месте (возвращаемое значение нигде не используется).

Решение: использовать null.

Излишнее использование drag&drop

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

Решение: метод OnPointerClick() имеет аргумент типа PointerEventData, достаточно проверить кнопку и количество нажатий.

var double_click = (eventData.button == PointerEventData.InputButton.Left) && ((eventData.clickCount % 2) == 0); if (double_click) { // ... }

Собственный Debug класс

public static class Debug { static bool IsEnable() { return UnityEngine.Debug.isDebugBuild; }  public static void Log(object message) { if (IsEnable()) { UnityEngine.Debug.Log(message); } }  // другие методы, аналогично Log }

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

Решение: для методов добавить атрибут Conditional с символом для включения лога вместо использования IsEnable(), тогда вызовы методов будут удалены из билда.

[Conditional("DEBUG_MODE")] public static void Log(object message) { UnityEngine.Debug.Log(message); }

Локализация в коде

ilc.QueueDialog("Obtained " + cnt + " " + itemName); informationLogControl.QueueDialog(string.Format(" stone(s) synthesized.", selectItems.name));

Строки локализации хранятся в коде без использования форматирования, или с форматированием, но с забытой подстановкой значений.

Решение: использовать систему локализации.

Неоптимизированная загрузка ресурсов

TextAsset textAsset = binaryLoad.LoadFromAssetBundleExecute<TextAsset>(assetPath, filePath); if (textAsset != null) { TextAsset textAsset2 = UnityEngine.Object.Instantiate<TextAsset>(textAsset); MemoryStream memoryStream = new MemoryStream(textAsset2.bytes); BinaryFormatter binaryFormatter = new BinaryFormatter(); obj = binaryFormatter.Deserialize(memoryStream); memoryStream.Close(); }

Вызов Instantiate() явно лишний, достаточно:

MemoryStream memoryStream = new MemoryStream(textAsset.bytes);

Множественная загрузка ресурсов

При загрузке сцены в цикле вызываются функции типа:

getItem = new ItemDataSet(gameDatas.loadDataSetDynamic<ItemDataSet>()[gets[0].getItem]);

вызов функции gameDatas.loadDataSetDynamic() в итоге сводится к:

FileStream fileStream = new FileStream(path, FileMode.Open, FileAccess.Read); BinaryFormatter binaryFormatter = new BinaryFormatter(); object obj = binaryFormatter.Deserialize(fileStream); fileStream.Close();

Загружаемый файл весит 541 килобайт, в итоге сцена загружается за пару секунд, после чего 5-20 секунд уходит на загрузку и десериализацию одного и тоже файла в цикле.
(вызов new ItemDataSet() создает копию полученного объекта, а не использует его как есть, поэтому проблем с множественными ссылками на один объект нет.)

Решение: кешировать загруженные данные и вынести создание объекта в отдельный метод.

public class ItemDataSet { static List<ItemDataSet> cache;  public static ItemDataSet Create(int index) { if (cache == null) { // загрузка и десериализация файла }  return new ItemDataSet(cache[index]); } }

getItem = ItemDataSet.Create(gets[0].getItem);

Клонирование объектов через сериализацию

public static T CloneObject<T>(T source) { using MemoryStream memoryStream = new MemoryStream(); BinaryFormatter binaryFormatter = new BinaryFormatter(); binaryFormatter.Serialize(memoryStream, source); memoryStream.Position = 0L; return (T)binaryFormatter.Deserialize(memoryStream); }

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

Решение: клонировать объект через MemberwiseClone() с последующим ручным клонированием для полей ссылочных типов.

Как минимум для списков структур без ссылок на изменяемые объекты можно создать отдельные версии без использования сериализации:

public static List<int> CloneObject(List<int> source) { return new List<int>(source); }  public static List<string> CloneObject(List<string> source) { return new List<string>(source); }

Использование классов вместо структур

public class LimitData { public int physical;  public int magical;  // конструктор }

Решение: в этом и многих других случаях структура уместнее, заодно снизит необходимость сериализации.

public struct LimitData { public int physical;  public int magical;  // конструктор }

Хранение ресурсов

По предыдущим пунктах видно, что активно используется сериализация через BinaryFormatter. В плюс у нее только очень простое использование, в минусах безопасность, плохая переносить между версиями (добавление и переименование полей, смена типа) и платформами.

Решение: хранить данные в ScriptableObject или JSON (если данные загружаются из внешнего источника или код должен работать вне Unity), по желания можно использовать AssetPostprocessor для конвертирования исходного файла в ScriptableObject.

Хранение текстур и спрайтов как TextAsset

public static byte[] LoadFromAssetBundleToBinary(string fn) { string fileName = Path.GetFileName(fn); string assetPath = dataRoot + "/" + Path.GetDirectoryName(fn).ToLower() + ".unity3d"; string filePath = dataRootAsA + "/" + Path.GetDirectoryName(fn) + "/" + fileName; TextAsset textAsset = LoadFromAssetBundleExecute<TextAsset>(assetPath, filePath); if (textAsset != null) { return textAsset.bytes; } return null; }

Sprite sprite = null; byte[] array = LoadFromAssetBundleToBinary(path + ".bytes"); if (array != null) { Texture2D texture2D = new Texture2D(64, 64, TextureFormat.ARGB32, mipmap: false); texture2D.LoadImage(array); texture2D.filterMode = FilterMode.Trilinear; texture2D.Apply(updateMipmaps: true, makeNoLongerReadable: true); sprite = Sprite.Create(texture2D, new Rect(0f, 0f, texture2D.width, texture2D.height), new Vector2(pw, ph)); string fileNameWithoutExtension = Path.GetFileNameWithoutExtension(path); sprite.name = fileNameWithoutExtension; }

Большинство текстур хранятся как TextAsset в бандлах или вообще отдельным файлом, поэтому читаются байты, создается текстура, а из нее спрайт.

Решение: хранить текстуры и спрайты в бандлах именно как текстуры и спрайты, это не только упростит загрузку, но ещё снизит выделение памяти и уменьшит размер игры при настроенном сжатии текстур.

Обработка ввода разбросана по разным классам

if (Input.GetKeyDown(KeyCode.Backspace) && this.back.interactable) { // ... }

Решение: использовать InputSystem, если это возможно, или вынести проверки в отдельный класс, будет более читаемо и упростит переназначение клавиш.

public static class Actions { public static bool Cancel() { return Input.GetKeyDown(KeyCode.Backspace); } }  if (Actions.Cancel() && this.back.interactable) { // ... }

Непостоянность горячих клавиш

Например, клавиши для быстрого сохранения/загрузки работают только в одном режиме игры, в остальное время недоступны, при этом кнопки быстрого сохранения/загрузки в интерфейсе всегда доступны и работают.

Два разных метода сохранения

В зависимости от того в каком режиме игра, используются разные методы для открытия интерфейса сохранения, работать будут оба, но в случае использования неверного метода часть данных будут утеряна.

Решение: сделать один публичный метод для сохранения, которому передавать информацию о текущем состоянии.

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

Использование неверной системы координат

В обучении маркеры (красный круг со стрелкой) часто указывают на неверную точку на скриншотах (на этой картинке это круг с мечом).

Координаты маркера заданы в 3D пространстве сцены, то есть для каждого маркера на новом скриншоте необходимо подбирать координаты в редакторе.

Решение: использовать координаты точки на скриншоте, и уже в коде конвертировать их в координаты маркера через Camera.ScreenToWorldPoint().

Использование объектов вместо компонентов

protected GameObject[] m_panels = new GameObject[6];

И дальше в коде используется в таком виде:

panels[i].GetComponent<unitBlockControl>(); panels[i].GetComponent<RectTransform>();

Решение: использовать нужный тип:

 protected unitBlockControl[] m_panels = new unitBlockControl[6];

где unitBlockControl будет хранить ссылку на свой RectTransform.

Итог

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