How not to make games in Unity

Many articles have been written about good and bad code, but there are very few articles analyzing problems in real code (with the exception of bugs in open source projects), so I decided to show problems in a real game on Unity.

The original version of the game was released in 2016 in Japanese, in 2023 in English, made on Unity 5.1.4.
For decompilation dnSpy, ILSpy, dotPeek were used. Not a single one did it without errors; dotPeek turned out to be the worst, but in this case this speaks about the quality of the code, and not about the shortcomings of the tools.

Invalid Enum elements

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

At the element “Main Weapon“there's a space in the middle because EquipIndex associated with the text displayed to the player.
Such code will not compile without special tricks, and localization is tightly nailed to the code, so each language will require a separate version of the code and fixing errors in all versions will be a noticeable problem.

Solution: store value in data Enumnot a name; for localization, use elements as keys at most.

Disabling EventSystem

For example like this:

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

There were problems with saving the settings: if you go into the sound or combat settings and exit without changing anything, then when saving, it quits NullReferenceException and the game hangs due to disabled EventSystem. And the only solution is to restart the game, although there is no critical error.

The very saving of settings when nothing has changed in them is a rather controversial thing.
(Note: if you change at least one parameter in the settings, then everything works without problems.)

Solution: use CanvasGroup.interactable to switch a limited group of elements.

Unnecessary coroutine in interface animation

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

Solution: use method OnCompletewhich is called when the animation completes:

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

Additional problems:

  • half a second to close the current panel, then half a second to open a new one – this is a very long time

  • The duration of the animation is a magic constant; if you need to speed up the animation, you will have to change it everywhere

  • in some places duration DOTween animations and waiting duration (WaitForSeconds) are different, I’m sure this is a mistake and they should be the same

Interfaces are divided into scenes

A base panel on one scene, a storage panel on another, a crafting panel on a third, a store on a fourth, etc. Loading of these scenes is not additive; it takes 0.5s for the animation of closing the current interface, loading a scene, and another 0.5s for the animation of opening a new one. You have to switch frequently and the long wait for players is annoying.

Solution: make the speed of interface animations customizable, do not separate interfaces by scenes, or use additive loading of scenes.

Multiple event handlers on buttons hanging in the editor

3-4 handlers have been added per event; this can be used for testing and prototyping, but in a working project it will cause problems, at least when refactoring and finding uses.

Mixing business logic and presentation

Code for moving an item from storage to inventory:

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");
	}
}

Solution: move the code for moving an item into a separate method MoveToInventory() and simplify the method.

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");
	}
}

Unobvious field assignments

The code above uses the field List count, which always has three elements. Index 2 is the number of items in storage, 1 in inventory, and the use of index 0 could not be found.

Solution: use two separate fields, or if compatibility is needed, hide count and replace with properties, or at least use named constants instead of 1 and 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;
}

Two public fields with the same meaning

At the item class for the field name And unique. Both are responsible for the name of the item, only name this is the basic immutable name, and unique this is the full name for items with a suffix and prefix.

Because of this, there was a bug when, when selling simple items, the message “was sold” appeared without the name of the item.

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

Solution: make the fields hidden and leave one property instead.

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

Unstable item sorting

Used for sorting List.Sort(comparer)the documentation for which directly states that sorting is unstable.
As a result, the order jumps when buying, selling or moving items. It is difficult to detect this during testing, because stable sorting is used for a small number of elements.

Solution: use stable sorting, convert it from unstable to a matter of five minutes with a slight overhead in terms of performance.

No list scrolling

Displaying lists is implemented simply: a fixed number of elements without using ScrollRect displays some of the elements from the list. But switching between pages is only possible by pressing buttons.

Solution: add an interface implementation IScrollHandler with method OnScroll() to change pages.

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

Code duplication

Several different classes are used to display an item in different panels, but the visual representation is always the same.

Solution: leave one class, and redirect events (drags, clicks, etc.) to the parent panel.

Saving on symbols

Often used in coroutines

yield return 0;

0 shorter null by three characters, but creates a boxing from scratch (the return value is not used anywhere).

Solution: use null.

Excessive use of drag&drop

Buying and selling items, equipment, crafting, and much more is done through drag and drop. These functions are divided into separate screens, so there is only one possible action and you can do it with a double click.

Solution: method OnPointerClick() has an argument like PointerEventDatajust check the button and the number of presses.

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

Own Debug class

public static class Debug
{
	static bool IsEnable()
	{
		return UnityEngine.Debug.isDebugBuild;
	}

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

A good idea, but poorly implemented, because there will be no logs in the release build, but the method calls themselves and the formation of strings will remain.

Solution: Add an attribute for methods Conditional with a symbol to enable logging instead of using IsEnable()then the method calls will be removed from the build.

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

Localization in code

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

Localization strings are stored in code without using formatting, or with formatting, but with forgotten value substitution.

Solution: use a localization system.

Unoptimized resource loading

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();
}

Call instantiate() obviously superfluous, enough:

MemoryStream memoryStream = new MemoryStream(textAsset.bytes);

Multiple resource loading

When loading a scene, functions like:

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

function call gameDatas.loadDataSetDynamic() it ultimately comes down to:

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

The downloaded file weighs 541 kilobytes, as a result, the scene loads in a couple of seconds, after which 5-20 seconds are spent loading and deserializing the same file in a loop.
(call new ItemDataSet() creates a copy of the resulting object rather than using it as is, so there are no problems with multiple references to the same object.)

Solution: cache the loaded data and move object creation into a separate method.

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

Cloning objects via serialization

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);
}

Apparently, at what point the developers got tired of writing code for copying fields for each class separately.

Solution: Clone the object via MemberwiseClone() followed by manual cloning for fields of reference types.

At a minimum, for lists of structures without references to mutable objects, you can create separate versions without using serialization:

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);
}

Using classes instead of structures

public class LimitData
{
	public int physical;

	public int magical;
	
	// конструктор
}

Solution: in this and many other cases, the structure is more appropriate and will also reduce the need for serialization.

public struct LimitData
{
	public int physical;

	public int magical;
	
	// конструктор
}

Resource storage

From the previous paragraphs it is clear that serialization is actively used through BinaryFormatter. On the plus side, it is only very simple to use; on the minus side, it is secure and difficult to transfer between versions (adding and renaming fields, changing types) and platforms.

Solution: store data in ScriptableObject or JSON (if the data is loaded from an external source or the code needs to run outside of Unity), you can optionally use AssetPostprocessor to convert the source file to ScriptableObject.

Storing textures and sprites as 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;
}

Most textures are stored as TextAsset in bundles or even as a separate file, so the bytes are read, a texture is created, and a sprite is created from it.

Solution: store textures and sprites in bundles exactly as textures and sprites, this will not only simplify loading, but will also reduce memory allocation and reduce the size of the game with texture compression configured.

Input processing is scattered across different classes

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

Solution: use InputSystemif possible, or move the checks into a separate class, it will be more readable and simplify key reassignment.

public static class Actions
{
	public static bool Cancel()
	{
		return Input.GetKeyDown(KeyCode.Backspace);
	}
}

if (Actions.Cancel() && this.back.interactable)
{
	// ...
}

Inconsistency of hotkeys

For example, the quick save/load keys only work in one game mode and are unavailable the rest of the time, while the quick save/load buttons in the interface are always available and working.

Two different saving methods

Depending on what mode the game is in, different methods are used to open the save interface, both will work, but if the wrong method is used, some of the data will be lost.

Solution: make one public method for saving, to which information about the current state is passed.

(In another game, I saw that a full-screen screenshot in png format was saved to disk every time the menu was opened in case the player decided to save. This created a noticeable delay every time the menu was opened, and the higher the screen resolution, the longer it was necessary to wait.)

Using the wrong coordinate system

In training, markers (a red circle with an arrow) often point to the wrong point in screenshots (in this picture it is a circle with a sword).

The coordinates of the marker are specified in the 3D space of the scene, that is, for each marker in the new screenshot, you need to select the coordinates in the editor.

Solution: use the coordinates of the point in the screenshot, and in the code convert them to marker coordinates via Camera.ScreenToWorldPoint().

Using Objects Instead of Components

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

And further in the code it is used in this form:

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

Solution: use the desired type:

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

Where unitBlockControl will store a link to your RectTransform.

Bottom line

All these problems will not prevent the game from being made and released, but will affect the convenience for players and the time spent searching for and correcting errors. The developers of this game have already postponed the release date of their latest game twice: first by two months, then by another month.

Similar Posts

Leave a Reply

Your email address will not be published. Required fields are marked *