analysis of errors in projects of a closed indie studio
Indie studio StarlightLabsCo has closed, and developer and founder Harris Rothaermel has published the source code of his projects. Now anyone can improve these projects or develop new ones based on them. We decided not to stand aside and decided to look for errors in his project.
The creator and founder of the indie studio StarlightLabsCo, Harris Rothaermel, decided to close the studio and publish source code projects on GitHub. This is quite a rare occurrence. Typically, studios close and the source code of projects is not published. Although sometimes source codes can emerge decades later from developers. Harris decided to do the noble thing and kindly share his projects.
The studio was created to develop games using machine learning models for NPCs and scenario generation.
This is exactly the news I came across on the Internet. I immediately became interested in what kind of projects there were. And then I encountered professional deformation in all its glory. How? I'll tell you now.
I'm developing a static analyzer PVS-Studio. Specifically, I lead the C# direction. If someone is not familiar with the term static analysis, then I’ll tell you briefly:
static code analysis is a process aimed at detecting defects, errors and potential vulnerabilities in the source code of programs. Static analysis can be thought of as automated code review. And, of course, PVS-Studio can analyze game projects. Including those written using Unity and Unreal Engine.
Returning to the issue of professional deformation. I saw the news that the studio had published the source code for its game. And the first thing I did was run a code analyzer to find errors in the code. I decided to analyze the game Starlight, which is written in C# using Unity. This is a game in which each character has a unique biography and his own opinion. Players can communicate with the characters, and responses are generated using a language model.
Unfortunately, very few games are open source, and there are only a few of them in C#. I was really hoping for the game to come out. Gigaya by Unity open source. It was meant to be a model game for teaching game development. But, alas, it was cancelled. By the way, Harris noted that he did not tidy up the code before publication. This makes it even more interesting to see what is stored under the hood.
Errors and strange code
Let's start analyzing what the analyzer revealed. I would like to note right away that the project is small and belongs to the category of indie games, so its code base is not too extensive. In such projects, the error density is much lower, which is why the analyzer did not detect many problems. However, the goal is not to find as many bugs as possible, but to improve the project as the developer wanted.
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 warning:
V3022 Expression 'ch < '0' && ch > '9'' is always false. Probably the '||' operator should be used here. TMP_PhoneNumberValidator.cs 20
Let's start with a simple mistake. The following code fragment checks the validity of the parameter ch. Only this check is wrong. Let's put the condition in a more understandable form:
ch < 48 && ch > 57
Instead of the '&&' operator you should have used '||' because the two conditions contradict each other. Now the test is always false.
Issue 2
And now it will be small interprocedural case.
protected override IEnumerable<Progress> ScanInternal ()
{
....
TriangleMeshNode.SetNavmeshHolder(AstarPath.active.data.GetGraphIndex(this),
this);
....
}
PVS-Studio warning:
V3106 The 1st argument 'AstarPath.active.data.GetGraphIndex(this)' is potentially used inside method to point beyond collection's bounds. NavMeshGenerator.cs 225
The analyzer reports that inside the method TriangleMeshNode.SetNavmeshHolder An index access occurs outside the permissible range. In this case, the first argument acts as an index.
Let's take a look at the method 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;
}
As we can see, the method can return -1 as a result. Now let's look at the method 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; // <=
}
}
The analyzer reports that the problematic access occurs on the last line. Access by index occurs unconditionally.
Issue 3
void ON_TEXT_CHANGED(Object obj)
{
if (obj = m_TextComponent)
hasTextChanged = true;
}
PVS-Studio warning:
V3137 The 'obj' variable is assigned but is not used by the end of the function. VertexShakeB.cs 44
It's hard to say whether an assignment inside a condition is an error, but it seems like it is. But what can be said for sure is that obj no longer used. Accordingly, the assignment makes no sense.
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 warning:
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
This is suspicious code and a candidate for an error. Note the third assignment and first argument to the structure's constructor Vector3. It is used there currentCharInfo.topRight.x as in the fourth assignment. Potentially need to use 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 warning:
V3038 The 'TileWorldSizeX' argument was passed to 'Max' method several times. It is possible that other argument should be passed instead. NavmeshBase.cs 479
Method Math.Max transmitted TileWorldSizeX as the first and second argument. This method must return the greater of the two values passed in. It turns out that it will always return the same value.
Micro-optimizations
PVS-Studio also has diagnostic rulesaimed at optimizing code for Unity projects. The analyzer finds performance-sensitive context in the program and warns about heavy operations. There were 3 such warnings issued for this project.
protected void Update()
{
....
if ( WebSocketClient.Instance.websocket.State == WebSocketState.Open
&& !this.IsPlayerControlled)
{
observedEntities = Utilities.UpdateObservedEntities(this, // <=
observedEntities,
transform,
5f);
}
....
}
PVS-Studio warning:
V4003 Avoid capturing variables in performance-sensitive context inside the 'UpdateObservedEntities' method. This can lead to decreased performance. Character.cs 168
The analyzer warns that the method Utilities.UpdateObservedEntities Variables are captured, which is a difficult operation. In this case, the method Utilities.UpdateObservedEntities called in the method Updatewhich processes every frame. This method will be the performance-sensitive context.
public static HashSet<Entity> UpdateObservedEntities(Character character, ....)
{
....
HashSet<Entity> newEntitiesInRange = new(collidersInRange
.Select(gameObject => gameObject.GetComponent<Entity>())
.Where(entity => entity != null && entity != character)); // <=
....
}
And here is the capture of variables in a lambda expression. In this case, the parameter is captured character. Capturing variables can cause performance degradation due to additional memory allocation. Thus, the presented section of code creates additional load on the GC.
Let's look at another case.
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 warning:
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
Here the performance sensitive context is the method LateUpdate. It is called every frame. According to Unity documentationa number of methods and properties from the Unity API perform resource-intensive operations when accessed. In this case, there is multiple access to the property Camera.main. When accessing a property Camera.main The cache is searched, which creates a load on the processor. You can avoid multiple access to a property by storing its value in a variable.
The analyzer also found a similar case, but with a call GetComponent. I will not show it, since it is similar in meaning to the others.
Conclusion
This case is quite rare, so I was very interested to look at the game code, albeit from a small indie studio. Plus we also managed to find errors in the project. In the end, I will recommend you several interesting articles on gaming topics from PVS-Studio:
Good luck and bye everyone!
If you want to share this article with an English-speaking audience, please use the translation link: Artem Rovenskii. The last source code: digging into bugs in projects after indie game studio shuts down.
