Artem Rovenskii

Oct 14 2024

Tags:

#CSharp #GameDev

The last source code: digging into bugs in projects after indie game studio shuts down

Oct 14 2024

Author: Artem Rovenskii

Errors and suspicious code
Micro-optimizations
Conclusion

StarlightLabsCo, an indie game studio, has shut down. Its founder and developer, Harris Rothaermel, has released the source code for his projects. Now anyone can refine these projects or come up with new ones based on them. We couldn't help but dive into the newly available open-source code, so we rushed to search for bugs.

Harris Rothaermel has put the source code of StarlightLabsCo projects on GitHub. That's quite a rare case. Most game studios don't release their source code after they shut down. Sometimes. you might see code pop up years later, but Harris is generous enough to share his projects right off the bat.

The game studio originated to create games that use machine learning models for NPCs and scenario generation.

That's what I found online, and it made me curious about the kinds of projects they were working on. That's when I really felt the impact of what they call "nerdview." I'll elaborate on that further.

I develop the PVS-Studio static code analyzer. To be more precise, I am the team lead of the C# development team. For those who aren't familiar with static analysis, I'll provide a brief explanation.

Static code analysis is a process aimed at identifying flaws, errors, and potential vulnerabilities in source code. You can think of it as an automated code review. PVS-Studio can analyze game projects, including those made with Unity and Unreal Engine.

So, back to the nerdview thing. The moment I saw that a game studio had released the source code of their projects, I rushed to check it with the static code analyzer to hunt down some interesting bugs. I chose to analyze Starlight, a game developed in C# using Unity. It's got characters with unique backstories and opinions. Players can interact with them, and characters respond using a language model.

Sadly, there are very few open-source games, and even fewer written in C#. I was looking forward to the release of Gigaya, an open-source game by Unity. It was intended to be a sample game for teaching game development. But it was canceled. By the way, Harris mentioned that he didn't clean up the code before releasing it, which makes the code even more interesting to investigate.

Errors and suspicious code

Now, let's see what the analyzer has revealed. First off, I should mention that this is a small indie project, so its codebase isn't large. In projects like this, the error density is much lower, so the analyzer didn't find a ton of issues. However, the goal isn't just to spot as many bugs as possible, but to enhance the project as the developer intended.

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

The 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 begin with a simple error. In the code snippet above, the validity of the ch parameter is being checked. But this check is incorrect. We'll take a closer look at the condition:

ch < 48 && ch > 57

Instead of the '&&' operator, the '||' operator should have been used since the two conditions simply contradict each other. As of now, the check will always return false.

Issue 2

Here's a case of interprocedural code analysis.

protected override IEnumerable<Progress> ScanInternal () 
{
  ....
  TriangleMeshNode.SetNavmeshHolder(AstarPath.active.data.GetGraphIndex(this),
                                    this);
  ....
}

The 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 an issue within the TriangleMeshNode.SetNavmeshHolder method: there is index access out of bounds. At the same time, the first argument serves as an index.

Take a look at the AstarPath.active.data.GetGraphIndex method:

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 look at the TriangleMeshNode.SetNavmeshHolder method:

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 the issue on the last line. Index access occurs unconditionally.

Issue 3

void ON_TEXT_CHANGED(Object obj)
{
    if (obj = m_TextComponent)
        hasTextChanged = true;
}

The 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 tell whether an assignment within a condition is an error, but it seems that it is the case. But clearly, obj is no longer used in any way. So, 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));
}

The 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

Here's the suspicious code fragment, a likely candidate for an error. Note the third assignment and the first argument of the Vector3 structure constructor. It uses currentCharInfo.topRight.x just like the fourth assignment, but it might be better to use currentCharInfo.bottomRight.x instead.

Issue 5

public override NNInfoInternal GetNearestForce (....) 
{
  ....
  for (int w = 0; w < wmax; w++) {
    if (bestDistance < (w-2)*Math.Max(TileWorldSizeX, TileWorldSizeX)) break;
  }
}

The 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

The Math.Max method is called with TileWorldSizeX as both the first and second argument. This method is supposed to return the larger of the two values. This means it always returns the same value.

Micro-optimizations

PVS-Studio provides a set of diagnostic rules aimed at code optimization in Unity projects. The analyzer detects a performance-sensitive context in the program and warns about resource-intensive operations. There are three such warnings for this project.

protected void Update()
{
  ....
  if (   WebSocketClient.Instance.websocket.State == WebSocketState.Open
      && !this.IsPlayerControlled)
  {
    observedEntities = Utilities.UpdateObservedEntities(this,       // <=
                                                        observedEntities,
                                                        transform,
                                                        5f);
  }
  ....
}

The 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 points out a variable being captured in the Utilities.UpdateObservedEntities method, which is quite a resource-intensive operation. The Utilities.UpdateObservedEntities method gets called within the Update method. Since Update is called every frame, it 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));    // <=
  ....
}

Here's a variable capture in the lambda expression. In this case, the character parameter is being captured. Variable capture can cause performance issues since it requires extra memory allocation. Thus, this code segment puts additional strain on the garbage collector.

Look at a similar 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);               // <=
  ....
  }
}

The 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

In this case, the LateUpdate method is the performance-sensitive context since it's called every frame. According to the Unity documentation, certain methods and properties in the Unity API can be resource-intensive when called. In this case, there are multiple accesses to the Camera.main property. When the Camera.main property is accessed, the cache is searched, which puts a load on the CPU. We can avoid multiple references to a property by storing the value in a variable.

The analyzer also pointed out a similar issue with the GetComponent<T> call. I won't go into that since it's pretty much the same as the others.

Conclusion

Since it's pretty uncommon for a game studio to release their source code, I was eager to check the code for this game, even if it comes from a small indie studio. Plus, I had the analyzer ready to hunt down any errors in the project. As a little bonus, here are a few interesting PVS-Studio articles about game projects:

Good luck, and see you soon!

#CSharp #GameDev