Return to Grove Street. Checking the Grand Theft Auto: San Andreas engine in Unity

The Grand Theft Auto series has transformed into the iconic game franchise. San Andreas is one of the most memorable parts for many gamers. Time is running, but fans still love this game. Some still can't get rid of the old and dusty CD, but some go even further. Let's take a look at how the PVS-Studio static analyzer checks the GTA: San Andreas fan port in the Unity engine.

Few words about project

SanAndreasUnity is the open-source project that ports Grand Theft Auto: San Andreas in Unity. Using the new engine, fans have a chance to extend game content and modify it more effectively than the original game from Rockstar Games allows. Moreover, developers have added cross-platforming, and now you can run the game on a smartphone, console, and even in VR.

Fans have written the code in C# and still support the project. The project is available on the GitHub repository.

Using PVS-Studio for Unity

Before we start describing bugs, let's find out how to check a Unity project.

The first and main point

Install PVS-Studio. :)

If you haven't already done so, this page will help you.

Open the project in Unity

Then set your preferred script editor in the Unity settings.

Use the "External Script Editor" parameter in the "External Tools" tab in the "Preferences" window.

To open this window, use the "Edit" -> "Preferences" menu option in the Unity editor:

Then open your project in the selected IDE using the "Assets" -> "Open C# Project" option in the Unity editor.

Run the analysis in the IDE

I'm going to use Visual Studio 2022. To analyze the project in this IDE version, use the "Extensions" -> "PVS-Studio" -> "Check Solution" menu item:

That's all! After the code analysis is started, you can start working with the analyzer warnings.

You can learn more about the Unity project analysis in the documentation.

Note. If you use PVS-Studio for the first time, the analyzer may issue many warnings for the entire code.
You don't need to fix all errors at once. You can suppress all warnings and regularly analyze only new code, going back to old warnings from time to time.

Cleaning up the streets of Los Santos

NullReferenceException

It's one of the most frequent enemies which makes a fuss among developers. The main thing is to find it in time.

Issue 1

Sometimes the error sneaks up in the most unexpected places.

Let's take a look at the following code:

public static void RegisterPrefab(GameObject prefab, Guid newAssetId,
  SpawnHandlerDelegate spawnHandler, UnSpawnDelegate unspawnHandler)
{
  if (newAssetId == Guid.Empty) 
  {
     Debug.LogError($"Could not register handler for '{prefab.name}' 
       with new assetId because the new assetId was empty");
     return;
  }
  if (prefab == null) 
  {
    Debug.LogError("Could not register handler for prefab 
      because the prefab was null");
    return;
  }
  ....
  if (spawnHandler == null) 
  {
    Debug.LogError($"Can not Register null SpawnHandler for {assetId}");
    return;
  }
  if (unspawnHandler == null) 
  {
    Debug.LogError($"Can not Register null UnSpawnHandler for {assetId}");
    return;
  }
  ....
}

Here you can see the standard null checking of method parameters. However, such an obviously useful action can cause troubles (ironic, isn't it?).

Let's go ahead through the first two checks:

public static void RegisterPrefab(GameObject prefab, Guid newAssetId, ....)
{
  if (newAssetId == Guid.Empty)
  {
    Debug.LogError($"Could not register handler for '{prefab.name}'
      with new assetId because the new assetId was empty");
    return;
  }
  if (prefab == null)
  {
    Debug.LogError("Could not register handler for prefab 
      because the prefab was null");
    return;
  }
  ....
}

The PVS-Studio warning: V3095 The 'prefab' object was used before it was verified against null. Check lines: 669, 673. NetworkClient.cs 669

The name field of the prefab parameter is accessed in the check of the_ newAssetId_ parameter, while the prefab parameter is checked after. The incorrect order of the null checking can cause a NullReferenceException.

Errors in error handler can be more dangerous than getting a 5-star wanted level chase, because they can easily "escape" from developers' eyes. Even HESOYAM or BAGUVIX won't help in this case, but there is still hope — it's static code analysis.

Issue 2

Look at the following suspicious fragment:

static void RebuildObserversCustom(NetworkIdentity identity, bool initialize)
{
  newObservers.Clear();
  if (identity.visible != Visibility.ForceHidden)
  {
    aoi.OnRebuildObservers(identity, newObservers);
    }
  ....
  // this code is from UNET, it's a bit strange but it works:
  if (initialize)
  {
    if (!newObservers.Contains(localConnection))
    {
      if (aoi != null)
          aoi.SetHostVisibility(identity, false);
    }
  }
}

The PVS-Studio warning: V3095 The 'aoi' object was used before it was verified against null. Check lines: 1464, 1546. NetworkServer.cs 1464

The code fragment is partially similar to the previous example: aoi is accessed before the null checking. Indeed, the code may not cause a NullReferenceException, but static analysis should also mark suspicious places.

At least, the code is strange. The developers' comment from the snippet also confirms that:

"this code is from UNET, it's a bit strange but it works"

Note. If you'd like to learn more about how to avoid or fix NullReferenceException, I recommend you reading this article.

Optimization
The new feature of our C# analyzer is diagnostic rules to optimize Unity projects!

The diagnostic rules are developed to detect frequently called methods and places that negatively affect the application performance.

Let's take a look at a few code fragments that may cause the performance decrease:

Issue 3

private void Update()
{
  if (!Loader.HasLoaded)
    return;

  _updateTimeLimitStopwatch.Restart();

  this.FocusPointManager.Update(); // <=
  ....
}

The Update method is executed on each frame. If you put the code that additionally load the system in a frequently executed method, then... Try to guess for yourself :)

Let's take a look at this.FocusPointManager.Update():

public void Update()
{
  double timeNow = Time.timeAsDouble;

  UnityEngine.Profiling.Profiler.BeginSample("Update focus points");
  this._focusPoints.RemoveAll(f => {....});                  // <=
  UnityEngine.Profiling.Profiler.EndSample();

  bool hasElementToRemove = false;
  _focusPointsToRemoveAfterTimeout.ForEach(_ => {....});     // <=

  if (hasElementToRemove)
    _focusPointsToRemoveAfterTimeout.RemoveAll(_ => {....}); // <=
}

The PVS-Studio warning: V4003 Avoid capturing variables in performance-sensitive context inside the 'Update' method. This can lead to decreased performance. Cell.cs 427

Let's break it down why this code fragment can decrease performance.

An instance of the closure class is created on each closure, and the captured values are placed in its fields. Since the class instance is located in the heap, it additionally loads GC.

In this case, we can use a custom implementation similar to RemoveAll and ForEach. It can help avoid extra memory allocation and reduce the garbage collector load.

Issue 4

void OnGUI()
{
  if (!statisticsGUI) return;

  GUILayout.BeginArea(new Rect(5, 110, 300, 300));

  if (ServerActive())
  {
    ....
    GUILayout.Label($"connections: {server.connections.Count}");
    GUILayout.Label($"SendQueue: {GetTotalSendQueue()}");
    GUILayout.Label($"ReceiveQueue: {GetTotalReceiveQueue()}");
  }
  ....
}

The PVS-Studio warning: V4001 Boxing usage inside the frequently called 'OnGUI' method may decrease performance. KcpTransport.cs 281

Here when GUILayout.Label is called, boxing occurs in some places when values of type int or long are passed. On the one hand, the performance impact may not be strong, but on the other hand, the solution is simple — add ToString and avoid superfluous boxing.

Note. IDE finds the addition of ToString to be redundant. Why should we do it if everything works as it is? However, this will cause boxing and additional load. You can find more about other cases of boxing in this article.

Issue 5

private void LateUpdate()
{
  ....
  if (ped != null)
    this.FocusPos = ped.transform.position;

  else if (Camera.main != null)
    this.FocusPos = Camera.main.transform.position;
  ....
  float relAngle = Camera.main != null ?
    Camera.main.transform.eulerAngles.y : 0f;
....
}

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. MiniMap.cs 190

The analyzer has found the code snippet with the help of the new diagnostic rule. This rule is on the development stage but can find new optimization approaches.

The issue is in the multiple using of Camera.main that can lead to an increase of the load on the CPU.

The correct approach in this case is to create an additional variable to which the return value of the Camera.main property will be written. This variable can be accessed in the future.

Various issues
Issue 6-7

The ending was a bit of a letdown. Look at the following fragment:

private void OnDrawGizmosSelected()
{
  ....
  var vehicles = FindObjectsOfType<Vehicle>()
                .Where(....)
                .OrderBy(....)
                .ToArray();
  foreach (var vehicle in vehicles)
  {
    foreach (var seat in vehicle.Seats){....}
    var closestSeat = vehicle.GetSeatAlignmentOfClosestSeat(transform.position);
    if (closestSeat != Vehicle.SeatAlignment.None){....}
    break;
  }
}

The PVS-Studio warning: V3020 An unconditional 'break' within a loop. PlayerController.cs 258

A simple, yet serious error: break is executed under any circumstances on the first iteration of the loop. As a result, only the first element from the entire vehicles collection will be used in the loop.

Look at another analogous snippet:

public override void ClientLateUpdate()
{
  while (reliableClientToServer.Count > 0)
  {
    QueuedMessage message = reliableClientToServer[0];
    if (message.time <= Time.unscaledTime)
    {
      wrap.ClientSend(new ArraySegment<byte>(message.bytes), Channels.Reliable);
      reliableClientToServer.RemoveAt(0);
    }
    break;
  }
  ....
}

The PVS-Studio warning: V3020 An unconditional 'break' within a loop. LatencySimulation.cs 220

As in the previous fragment, break is unconditional and will be executed on the first iteration.

This case differs because developers have used while here, and almost identical fragments occur four more times. The developer may intend it that way, but there is no comment or context indicating this. Whether this place is odd or wrong, let the developers decide themselves. We just point out the suspicious moment.

Issue 8

Here is a classic error pattern "busted" by static analyzers:

public void SetString(string key, string value)
{
  if (m_syncDictionary.TryGetValue(key, out string existingValue))
  {
    if (value != existingValue)
    {
      m_syncDictionary[key] = value;
    }
  }
  m_syncDictionary[key] = value;
}

The PVS-Studio warning: V3008 The 'm_syncDictionary[key]' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 112, 108. SyncedBag.cs 112

When two conditions pass, m_syncDictionary[key] is assigned. If the condition doesn't pass... then there will be the same assignment! One of them may be different.

Conclusion

Despite all the flaws found in the code, the developers have done a good job, and the code is of high-quality. We should understand that fans have tried to copy the original game logic (with all the kludges and issues). The project community is dedicated to Grove Street. Home. Nice work!

If you're interested in articles about checking game projects, you can visit our blog.

If you'd like to check a Unity project with PVS-Studio, download and try the analyzer here.