Как не стоит делать игры на 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().DOAnchorPos(new Vector2(-300f, 0f), 0.5f).SetEase(Ease.OutCubic);
yield return new WaitForSeconds(0.5f);
// завершающий код
} Решение: использовать метод OnComplete, который вызывается при завершении анимации:
private void ShowMotion()
{
GetComponent().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();
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 count;
List 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();
if (component.itemData.MoveToInventory())
{
OnDrawUpdate();
DrawUpdateHandler.CallDrawUpdate("ItemPanelGuild");
}
} Неочевидные назначения полей
В коде выше используется поле List
Решение: использовать два отдельных поля, или если необходима совместимость, то скрыть 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
В результате при покупке, продаже или перемещении предметов порядок скачет. При тестировании такое выявить сложно, потому что при небольшом количестве элементов используется стабильная сортировка.
Решение: использовать стабильную сортировка, сделать её из нестабильной дело пяти минут с незначительным оверхедом по производительности.
Отсутствие прокрутки списков
Отображение списков реализовано просто: фиксированное количество элементов без использования 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(assetPath, filePath);
if (textAsset != null)
{
TextAsset textAsset2 = UnityEngine.Object.Instantiate(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()[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 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 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 CloneObject(List source)
{
return new List(source);
}
public static List CloneObject(List source)
{
return new List(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(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();
panels[i].GetComponent(); Решение: использовать нужный тип:
protected unitBlockControl[] m_panels = new unitBlockControl[6];где unitBlockControl будет хранить ссылку на свой RectTransform.
Итог
Всё эти проблемы не помешают сделать и выпустить игру, но скажутся на удобстве для игроков и времени, потраченном на поиск и исправлении ошибок. Разработчики этой игры уже дважды переносили дату выпуска своей последней игры: сперва на два месяца, потом ещё на месяц.
