Digging into open-source Unity VR Games. Part 1: RocketMan

This is the first article in a short series exploring some intriguing VR games while examining code issues found with PVS-Studio. Meet RocketMan!

About the game

RocketMan is a very short, simple, yet engaging puzzle game. According to the lore, you attempt to escape a dying planet aboard a spaceship, but everything goes wrong: the rocket is completely unusable, systems have failed, all compartments are locked, and you're trapped in one. Your task is to escape the trap, repair each compartment one by one, ultimately fix the entire rocket, and finally blast off!

Using a VR headset, I played the game myself and overall enjoyed the experience despite its visual simplicity, occasional object-grabbing glitches, and one unplanned evacuation through the textures :)

As for the gameplay, I felt the game lacked a countdown timer to the planet's collapse—the very disaster we're fleeing. It would be even cooler to add tension-building effects, like earthquakes, to the timer that intensify as the end approaches. And what would you add to the game? Share your ideas in the comments!

Now that we're familiar with the game, why not explore the errors PVS-Studio uncovered in its source code? But wait—what is PVS-Studio?

About PVS-Studio

If you're hearing this name for the first time, PVS-Studio is a tool that automatically checks the quality and security of C#, Java, C, and C++ code without executing it. By "code," I mean not just individual classes or files, but entire projects and solutions. From typos and potential exceptions to logic errors and security vulnerabilities—this is just part of what the tool can detect.

PVS-Studio also pays special attention to the specifics of Unity script code. Notably, the analyzer offers:

• annotations for APIs in the UnityEngine namespace, providing extra context about purpose, parameters, and return values. This enables detecting more errors that require a deep understanding of data flow;
• diagnostic rules for issues specific to Unity scripts;
• diagnostic rules suggesting micro-optimizations in frequently executed functions like Update;
• recognition of overloaded null checks for Unity objects. The analyzer understands these checks may also indicate destroyed in-game objects, reducing false positives about potential null dereferences.

Learn more about these features in the articles:

• How the PVS-Studio analyzer began to find even more errors in Unity projects;
• PVS-Studio in development of Unity projects: new specialized diagnostic rules.

Check out the full list of analyzer diagnostic rules at the link.

Now let's look at the basic workflow with the analyzer. It is quite straightforward.

• Install the IDE extension (Visual Studio, in my case) alongside PVS-Studio.
• Select files/projects/solutions in your IDE explorer and start analysis via the context menu.

• Results appear incrementally in a PVS-Studio window.

• Review warnings, which may be quite a few. In this case, you can apply various filters and use mass warning suppression that can be reverted later. This way you can view warnings in parts to manage workload.

Find more information about the analyzer on our website. Now, let's review issues found in the game's code.

About the source code

Whether due to the game's small scale or the developers' skill, explicit errors in the core game code were rare. Beyond outright errors, the analyzer suggested several micro-optimizations, some of which we'll examine.

Surprisingly, the Oculus scripts—providing basic VR interaction components—contained most of the issues (though still few). Note: the game uses a legacy VR API, but that's no reason for us to miss a programming lesson :)

Error review

Unused iterator

public void InsertChar(string c) 
{
  ....
  waiter(); //avoid double button clicks
}
IEnumerator waiter()
{
  yield return new WaitForSecondsRealtime(2);
}

The PVS-Studio warning: V3206 Unity Engine. A direct call to the 'waiter' coroutine-like method will not start it. Use the 'StartCoroutine' method instead. Keyboard_Tuto.cs 26.

The InsertChar method calls waiter, intended to pause execution for two seconds. It is hinted by its return value: new WaitForSecondsRealtime(2).

This won't work for two reasons.

1. Calling waiter returns an iterator object without executing its body. To execute the logic, the author should call waiter().MoveNext().
2. Returning new WaitForSecondsRealtime(2) doesn't pause execution. It's a timer iterator where 'MoveNext' returns a boolean value indicating if the time elapsed.

Most likely, the developer wanted to prevent accidental button re-clicks from re-triggering InsertChar. A fixed version might look like this:

bool _disableInsertChar = false;
public void InsertChar(string c) 
{
  if (_disableInsertChar)
    return;
  ....
  StartCoroutine(waiter()); 
}
IEnumerator waiter()
{
  _disableInsertChar = true;
  yield return new WaitForSecondsRealtime(2);
  _disableInsertChar = false;
}

Here, waiter runs as a Unity coroutine, as the iterator it returns will be passed as an argument to the StartCoroutine method. Thus, the logic inside waiter will execute asynchronously while the return of new WaitForSecondsRealtime(2) truly pauses this logic execution for two seconds, as this class is meant to be used in coroutines.

A new _disableInsertChar field is set to true when running the coroutine, and then false—after two seconds, once the coroutine is executed. The InsertChar method starts with the return from this method, if _disableInsertChar is true.

Incorrect null check due to implicit initialization

public class OVRTrackedKeyboard : MonoBehaviour
{
  ....
  public Transform ActiveKeyboardTransform;
  ....
}

public class OVRTrackedKeyboardHands : MonoBehaviour
{
  ....
  private void LateUpdate()
  {
    ....
    var position = KeyboardTracker.ActiveKeyboardTransform?.position;
    var rotation = KeyboardTracker.ActiveKeyboardTransform?.rotation;
    ....
  }
  ....
}

The PVS-Studio warning: V3216 [CWE-754] Unity Engine. Checking the 'ActiveKeyboardTransform' field with a specific Unity Engine type for null with '?.' may not work correctly due to implicit field initialization by the engine. OVRTrackedKeyboardHands.cs 296.

This warning highlights a non-obvious Unity-specific behavior: serialized fields of type UnityEngine.Object (or its subclasses, except for MonoBehaviour and ScriptableObject) are implicitly initialized with a "fake null" object.

For clarity, let's debug a simple MonoBehaviour script with two public fields: one of the Transform type, another one—MonoBehaviour. Leaving the fields empty in the explorer reveals if they are both null:

As we can see, the field of type Transform is initialized with some object that only "pretends" to be null, but isn't actually null. This means such null checks as ?., ??, ??=, is null will simply not work, because for these checks the field is considered to have some value.

In the code example above, we find such a case: the field KeyboardTracker.ActiveKeyboardTransform is checked using ?.. But due to implicit initialization, the check won't work, and dereferencing it afterwards results in the exception.

The good news is that implicit initialization of such fields occurs only when running the project in the editor. Release builds behave as expected.

Despite this caveat, it's still best to perform null checks using the operators ==, != or the shorthand checks field, !field, to avoid unexpected errors during debugging. Both approaches have special overloads that handle implicit initialization, and hence will always work correctly.

Confusion between || and && operators

public static bool SaveCubemapCapture(....)
{
  ....
  if (    dirName[dirName.Length - 1] != '/' 
       || dirName[dirName.Length - 1] != '\\')
  {
    dirName += "/";
  }
  ....
}

The PVS-Studio warning: V3022 [CWE-571] Expression is always true. Probably the '&&' operator should be used here. OVRCubemapCapture.cs 201.

Let's set aside errors specific to Unity scripts and consider a quite typical case. The condition in the provided snippet is always true, since it uses the||operator to check that dirName[dirName.Length - 1] is not equal to two different values. Thus, if the first subcondition is false, then the second is true, and vice versa.

Clearly, the programmer wanted to ensure a path separator exists at the end of the string. To make it work as expected, the author should replace the||operator with&&.

Unreleased memory after reading a stream

private SceneInfo GetSceneInfo()
{
  SceneInfo sceneInfo = new SceneInfo();
  try
  {
    var reader = new StreamReader(sceneLoadDataPath); // <=
    ....
    while (!reader.EndOfStream)
    {
      sceneList.Add(reader.ReadLine());
    }
    sceneInfo.scenes = sceneList;
  }
  ....

The PVS-Studio warning: V3114 [CWE-404] IDisposable object 'reader' is not disposed before method returns. OVRSceneLoader.cs 212

The analyzer detected a minor resource leak. After using an object of the StreamReader type, we need to explicitly release memory for unmanaged (inaccessible to GC) resources by calling the Dispose() method.

Alternatively, we can use the using construct. Once it is executed, Dispose is called implicitly. The fixed code may look as follows:

private SceneInfo GetSceneInfo()
{
  SceneInfo sceneInfo = new SceneInfo();
  try
  {
    using (var reader = new StreamReader(sceneLoadDataPath))
    {
      ....
      while (!reader.EndOfStream)
      {
        sceneList.Add(reader.ReadLine());
      }
      sceneInfo.scenes = sceneList;

    }
  }
  ....
}

Identical subexpressions on both sides of the operator

Issue 1

private static bool RenameSpatializerPluginToOld(string currentPluginPath)
{
  ....
  targetPluginPath = currentPluginPath + ".old" + index.ToString();
  targetPluginMetaPath = targetPluginPath + ".meta";

  if (!File.Exists(targetPluginPath) && !File.Exists(targetPluginPath))
    break;
  ....
}

The PVS-Studio warning: V3001 There are identical sub-expressions '!File.Exists(targetPluginPath)' to the left and to the right of the '&&' operator. ONSPAudioPluginUpdater.cs 138.

A typical oversight. The condition checks for the existence of a file twice using the same path stored in targetPluginPath. The presence of another variable next to the condition storing the path to targetPluginMetaPath suggests that the second check was intended for that variable.

Let's look at the similar case.

Issue 2

protected void UpdateActive(OvrAvatar avatar, ovrAvatarVisibilityFlags mask)
{
  ....
  bool showFirstPerson = (mask & ovrAvatarVisibilityFlags.FirstPerson) != 0;
  bool showThirdPerson = (mask & ovrAvatarVisibilityFlags.ThirdPerson) != 0;

  gameObject.SetActive(showThirdPerson || showThirdPerson);
  ....
}

The PVS-Studio warning: V3001 There are identical sub-expressions 'showThirdPerson' to the left and to the right of the '||' operator. OvrAvatarRenderComponent.cs 21.

Notice the binary expression passed to the gameObject.SetActive method. It uses the||operator, where the same showThirdPerson variable is checked both on the left and on the right. As in the previous case, another variable, showFirstPerson, appears in the code near the condition and was likely intended to replace one of the duplicates.

A loop that is not a loop

protected override void DoSelectUpdate(....)
{
  ....
  while (!outsideDelta)
  {
    ....
    if (....)
    {
      outsideDelta = true;
      break;
    }

    if (....)
    {
      outsideDelta = true;
      break; // <=
    }

    break; // <=
  }
  ....
}

The PVS-Studio warning: V3020 [CWE-670] An unconditional 'break' within a loop. PokeInteractor.cs 300.

During feature development, ideas for a more optimal or correct implementation often arise, requiring revision. It happens that artifacts remain, some of which may be harmless while others can become serious errors.

Thus, the snippet shows a loop without continue operators and with an unconditional break at the end, which is clearly such an artifact. As a result, the loop has only one iteration.

Micro-optimizations overview

Beyond helping detect errors, PVS-Studio can provide some advice on micro-optimizations for Unity scripts. At first glance, such cases may seem unimportant, but imagine redundant memory allocations or evaluations occurring dozens of times per second, every frame.

What if there are not one or two such cases, but ten or twenty? In this scenario, oversights in micro-optimization can consume a significant portion of computational resources, thereby limiting the potential to scale the project further.

In the RocketMan code, there are relatively few such warnings—only 25. Let's look at some of them.

Memory allocation every frame update

void Update()
{
  ....
  FindHoverObject(controllerPos, controllerRot);
  ....
}

void FindHoverObject(Vector3 controllerPos, Quaternion controllerRot)
{
  var objectsHit = Physics.RaycastAll(controllerPos, 
                                      controllerRot * Vector3.forward);
  ....
}

The PVS-Studio warning: V4008 Unity Engine. Avoid using the 'Physics.RaycastAll' memory allocation method in performance-sensitive context inside 'FindHoverObject'. Consider using the 'RaycastNonAlloc' non-allocation method instead. ObjectManipulator.cs 55.

In this example, Physics.RaycastAll is called every frame. What can be optimized here? The problem is that each call to Physics.RaycastAll (dozens of times per second) allocates memory for a new collection to store the returned result. This leads to more frequent GC invocations, which slows down the app.

Fortunately, Physics.RaycastAll has a more efficient alternative—Physics.RaycastNonAlloc. The difference is that Physics.RaycastNonAlloc takes a reference to a pre-allocated buffer as an argument and writes the result there. The buffer can be moved to a field, initialized just once, and then overwritten without freeing and reallocating memory.

Calling consuming functions every update

void Update()
{
  ....
  if (grabObject)
  {
    ....
    ManipulateObject(grabObject, 
   controllerPos, 
   controllerRot);
    ....
  }
  ....
}
void ManipulateObject(....)
{
  ....
  if (obj.GetComponent<GrabObject>()) // <=
  {
     ....
     switch (obj.GetComponent<GrabObject>() // <=
    .objectManipulationType)
     ....
  }
  ....
}

The PVS-Studio warning: V4005 Expensive operation is performed inside the 'GetComponent' method. Calling such method in performance-sensitive context located inside the 'ManipulateObject' method can lead to decreased performance. ObjectManipulator.cs 71.

This time, the analyzer points out redundant calls to obj.GetComponent<GrabObject>() inside the Update method. The GetComponent is computationally expensive, so it's better to avoid it in frequently executed methods like Update.

In most cases, this can be avoided, since the component obtained via GetComponent can be cached once in fields within methods like Start and Awake. And instead of regularly adding/removing components, they can be enabled/disabled using the Behaviour.enabled property.

Non-optimal arithmetic expression

private void Update()
{
  ....
  if (referenceHand)
  {
    ....            
    CreateHandMesh(); // <=
  }
  ....
}

void CreateHandMesh()
{
  ....
  for (int i = 0; i < 5; i++)
  {
    ....
    AddKnuckleMesh(....); // <=
    AddKnuckleMesh(....); // <=
    ....
    AddKnuckleMesh(....); // <=
  }
  ....
}

void AddKnuckleMesh(....)
{
  ....
  Vector3 windingVec = rightVec;

  for (int i = 0; i < fanVerts * 2; i++)
  {
    ....
    Vector3 vertPos = handVertices[basePoint] + 
                      windingVec *       // <=
                      borderSize * 
                      handScale * 
                      (....);
    ....
  }
  ....
}

The PVS-Studio warning: V4006 Unity Engine. Multiple operations between the 'windingVec' complex value and numeric values. Prioritizing operations between numeric values can optimize execution time. HandMeshMask.cs 70.

Look at the vertPos variable initialization expression in the AddKnuckleMesh method. It performs an addition operation, where one operand is a chain of multiplication operations. This chain is the optimization point flagged by the analyzer. What's wrong with it?

Note that the very first multiplier—the windingVec variable is of type Vector3. When multiplying this type by a number, each of its three components is multiplied by that number. This means nine operations will be performed in the entire chain computation. This number can be reduced by first multiplying simple numbers together, then multiplying the result by the vector. In this case, only six operations are performed, and the multiplication chain looks like this:

windingVec * (borderSize * handScale * (....))

At first glance, this optimization seems insignificant, but let's evaluate the number of operations without optimization, considering the context.

• Multiplications occur inside a loop with, say, five iterations on average (9 * 5 = 45).
• The AddKnuckleMesh method is called three times inside a five-iteration loop in CreateHandMesh (45 * 3 * 5 = 675).
• The CreateHandMesh method is called in the Update method, whose execution frequency depends on the frame rate. Assuming it runs 100 times per second on average (675 * 100 = 67,500).
• Also, note that the CreateHandMesh call is under the condition. We can't say exactly how much this affects the execution frequency, so let's assume it halves the frequency (67,500 / 2 = 33,750).

Ultimately, we get an imprecise but illustrative representation of multiplication operations (33,750) performed per second just in this expression.

Now adjust this number with the optimization (33,750 / 9 * 6 = 22,500). A difference of 11,250 computational operations per second. Quite an optimization, right?

Conclusion

Analyzing the RocketMan VR game partially demonstrates PVS-Studio's value for Unity development. Admittedly, RocketMan is a small project, whereas static analysis truly shines on medium to large codebases.

In large-scale projects, it saves significant time that would otherwise be spent on code reviews and debugging while minimizing "escaped" defects found too late by users.

Try PVS-Studio on your projects: request a free trial key here. You might also want to check our first run guide.

See you in the next articles!