# 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\. ![1083_GTA_SA/image1.png](https://import.viva64.com/docx/blog/1083_GTA_SA/image1.png) ## 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](https://github.com/GTA-ASM/SanAndreasUnity)\. > \*\*Note\\\.\*\* If you're interested in the fan remasters of classic games, you can learn more in the article: "\[Heroes of Code and Magic: VCMI game engine analysis\]\(https://pvs\-studio\.com/en/blog/posts/cpp/1058/\)"\\\. ## 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](https://pvs-studio.com/en/pvs-studio/try-free/) 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: ![1083_GTA_SA/image2.png](https://import.viva64.com/docx/blog/1083_GTA_SA/image2.png) 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: ![1083_GTA_SA/image3.png](https://import.viva64.com/docx/blog/1083_GTA_SA/image3.png) 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](https://pvs-studio.com/en/docs/manual/6607/)\. > \*\*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\]\(https://pvs\-studio\.com/en/docs/manual/0032/\) 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: ```cpp 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: ```cpp 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](https://pvs-studio.com/en/docs/warnings/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_\. ![1083_GTA_SA/image4.png](https://import.viva64.com/docx/blog/1083_GTA_SA/image4.png) 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](https://pvs-studio.com/en/blog/posts/1048/)\. **Issue 2** Look at the following suspicious fragment: ```cpp 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](https://pvs-studio.com/en/docs/warnings/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\]\(https://pvs\-studio\.com/en/blog/posts/csharp/1049/\)\\\. ### Optimization The new feature of our C\# analyzer is [diagnostic rules to optimize Unity projects](https://pvs-studio.com/en/blog/posts/csharp/1071/)\! 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** ```cpp 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\(\)_: ```cpp 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](https://pvs-studio.com/en/docs/warnings/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** ```cpp 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](https://pvs-studio.com/en/docs/warnings/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\]\(https://pvs\-studio\.com/en/blog/posts/csharp/1060/\)\\\. **Issue 5** ```cpp 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](https://docs.unity3d.com/ScriptReference/Camera-main.html)\. 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: ```cpp private void OnDrawGizmosSelected() { .... var vehicles = FindObjectsOfType() .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](https://pvs-studio.com/en/docs/warnings/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: ```cpp public override void ClientLateUpdate() { while (reliableClientToServer.Count > 0) { QueuedMessage message = reliableClientToServer[0]; if (message.time <= Time.unscaledTime) { wrap.ClientSend(new ArraySegment(message.bytes), Channels.Reliable); reliableClientToServer.RemoveAt(0); } break; } .... } ``` The PVS\-Studio warning: [V3020](https://pvs-studio.com/en/docs/warnings/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\. ![1083_GTA_SA/image5.png](https://import.viva64.com/docx/blog/1083_GTA_SA/image5.png) **Issue 8** Here is a classic error pattern "busted" by static analyzers: ```cpp 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](https://pvs-studio.com/en/docs/warnings/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](https://pvs-studio.com/en/blog/posts/?tag=gamedev)\. If you'd like to check a Unity project with PVS\-Studio, download and try the analyzer [here](https://pvs-studio.com/en/pvs-studio/try-free/)\.