Digging into open-source Unity VR Games. Part 2: NorthStar

Additionally, the game includes dynamic mechanics like day/night cycles, weather changes, realistic swaying of sailing, and living NPCs. These features inspired not only to play NorthStar, but to examine its source code using PVS-Studio analyzer.

About the game

NorthStar is a compact yet exceptionally beautiful demo game where you can take the role of a shipwreck survivor, miraculously rescued by the crew of the Polaris ship, destined to become part of it.

Beyond its stunning graphics (the ocean visuals alone are remarkable), the game features unique interactive ship elements:

Ropes for wrapping, swinging, and pushing objects.

A spyglass for observing distant objects.

A manually operated harpoon gun that loads bolts, draws the bowstring, aims by rotating the harpoon, and fires with a button press.

Grabbed something interesting with the harpoon? Wind the cranks to hoist it aboard.

Additionally, the game includes dynamic mechanics like day/night cycles, weather changes, realistic swaying of sailing, and living NPCs.

These features inspired not only to play NorthStar, but to examine its source code using PVS-Studio analyzer.

About PVS-Studio

If you're unfamiliar or missed the first article, don't forget to check it out.

PVS-Studio is a tool that automatically detects potential issues in C#, C++, C, and Java projects.

It finds simple typos as well as complex errors and security vulnerabilities that require tracking data flow and analyzing code semantics.

For Unity projects specifically, PVS-Studio accounts for Unity script peculiarities, enhancing analysis usefulness. Key analyzer features are listed below.

• API annotations from UnityEngine namespaces provide extra context on the purpose, arguments, and return values. As a result, it can detect more errors through more accurate data flow tracking.
• Diagnostic rules detecting Unity-specific issues.
• Diagnostic rules identifying micro-optimization opportunities in frequently executed blocks like Update.
• Accounting for the specifics of null equality checks for UnityEngine objects when processing them. The analyzer recognizes that such checks may also indicate an object has been destroyed in the game, reducing false null-dereference warnings.

Learn more on this topic in these 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.

The basic analyzer workflow is straightforward and involves three steps:

1. Analyze individual files, projects, or entire solutions.

2. Wait for the first warnings in the PVS-Studio window or full analysis completion.

3. Review warnings. For large code bases, you can apply various filters and use mass warning suppression—don't worry, you can revisit them later. This way you can divide this process into several iterations.

Let's take a closer look at the Best warnings tab—warnings shown there are most likely to be valuable.

The screenshots above show Visual Studio integration, but PVS-Studio also works with Rider, Visual Studio Code, and others. A full list is available on our website.

Now, let's see PVS-Studio in action!

About the code

The same as in the previous article, we found few errors. Some popped up in third-party scripts, but some showed up in the game's code. Below, we'll examine the most eye-catching and interesting ones. Let's dive in!

Potential negative index access

public void Skip()
{
  ....
  var currTaskIndex = Task.Sequence
                          .TaskDefinitions
                          .FindIndex(def => def.ID == TaskID);

  if (currTaskIndex != 0)
  {
    var lastTask = Task.Sequence.TaskDefinitions[currTaskIndex - 1];
    ....
  }
  ....
}

The PVS-Studio warning: V3106 [CWE-125] Possible negative index value. The value of 'currTaskIndex - 1' index could reach -2. TaskHandler.cs 201

The analyzer warns about possible negative index access. Indeed, the code looks suspicious: for some reason, the currTaskIndex != 0check excludes the collection's first element. Yet there's no check for the -1 index inequality, which would indicate a missing element.

The unexpected exception

public void CollisionPass(....)
{
  ....
  if (!CachedSphereCollider.TryGet(out var sphereCollider)) // <=
  {
    return;
  }
  ....
}

The PVS-Studio warning: V3022 [CWE-570] Expression '!CachedSphereCollider.TryGet(out var sphereCollider)' is always false. NPC_DynamicRigs.cs 522

Here, PVS-Studio warns us that the 'CachedSphereCollider.TryGet' method always returns true. Is it so? Let's check the declaration:

public static bool TryGet(out SphereCollider collider)
{
  if (s_hasSphere)
  {
    collider = s_sphereCollider;
    return true;
  }

  try
  {
    ....
    s_sphereCollider = obj.GetComponent<SphereCollider>();
    collider = s_sphereCollider;
    collider.enabled = false;
    s_hasSphere = true;
    return true;
  }
  catch
  {
    ....
    throw;
  }
}

As we can see, this is indeed the case. By the way, this implementation is peculiar. The method should return true if obtaining the sphereCollider object succeeds, and false—in the opposite case. In fact, the method throws an exception if obtaining sphereCollider fails. If we traverse the CollisionPass call stack up to entry points like LateUpdate, we can ensure that this exception isn't handled anywhere.

Potential null dereference

private void DisplayResults()
{
  if (m_texturesWithoutMipmaps.Count > 0)
  {
    ....
  }
  else if (m_texturesWithoutMipmaps != null)
  {
    ....
  }
}

The PVS-Studio warning: V3095 [CWE-476] The 'm_texturesWithoutMipmaps' object was used before it was verified against null. Check lines: 91, 128. MipMapChecker.cs 91

The analyzer detected a potential null dereference in the if statement. The following-up check in the else if statement indicates this possibility. The idea of first checking for elements in a collection before checking the collection to null seems strange. Adding ?. to the first condition would clarify the logic:

private void DisplayResults()
{
  if (m_texturesWithoutMipmaps?.Count > 0)
  {
    ....
  }
  else if (m_texturesWithoutMipmaps != null)
  {
    ....
  }
}

Unreliable null check

public class FadeVolume : MonoBehaviour
{
  [SerializeField] private AudioSource m_audioSource;
  ....
  public void FadeOutAudio(float time)
  {
    if (m_audioSource is null)
    {
      ....
    }
    ....
  }
  ....
}

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

The issue doesn't affect the game's Release build, but it may cause unexpected errors during the 'Play' mode in the Unity Editor because fields displayed in the Inspector window with the UnityEngine.Object type or its derivatives (except MonoBehaviour and ScriptableObject) are implicitly initialized with a fake null if they haven't been initialized in the Inspector or code. Below is a debugging screenshot that illustrates this:

Due to this implicit initialization, null checks without special overloads—specifically ??, ??=, ?., and is null—become useless since the field technically has a value. To avoid this non-obvious behavior, use == and != operators for null checks, or shorthand checks m_audioSource, !m_audioSource with the overload that takes implicit initialization into account.

Incorrect loop

string MemberLabel(string memberName)
{
  foreach (var memberInfo in currObject.GetType().GetMember(memberName))
  {
    if (....)
    {
      memberName = $"{memberName}()";
    }

    return $"{memberInfo.DeclaringType?.Name}.{memberName}";
  }

  return memberName;
}

The PVS-Studio warning: V3020 [CWE-670] An unconditional 'return' within a loop. MemberLinkDrawer.cs 162

Here we can see a strange loop that iterates over the collection but always has only one iteration due to an unconditional return inside its body. Perhaps, this return should be inside the if block in the loop. If this is the case, the fixed code makes more sense:

string MemberLabel(string memberName)
{
  foreach (var memberInfo in currObject.GetType().GetMember(memberName))
  {
    if (....)
    {
      memberName = $"{memberName}()";
      return $"{memberInfo.DeclaringType?.Name}.{memberName}";
    }
  }

  return memberName;
}

Typos

Issue 1

[Serializable]
public struct ComfortPreset
{
  public float BoatMovementStrength;
  public float BoatReactionStrength;
}
public void ComfortLevelChanged()
{
  OnComfortLevelChanged?.Invoke(PlayerConfigs.ComfortLevel);
  var comfortPreset = ComfortPresets[(int)ComfortLevel];
  BoatMovementStrength = comfortPreset.BoatMovementStrength;
  BoatReactionStrength = comfortPreset.BoatMovementStrength;
}

The PVS-Studio warning: V3127 [CWE-682] Two similar code fragments were found. Perhaps, this is a typo and 'BoatReactionStrength' variable should be used instead of 'BoatMovementStrength' GlobalSettings.cs 70.

The analyzer spotted an obvious typo: the wrong value is assigned to the BoatReactionStrength field. The ComfortPreset struct contains an identically named field whose value should be assigned instead. The fixed code snippet may look as follows:

public void ComfortLevelChanged()
{
  ....
  BoatMovementStrength = comfortPreset.BoatMovementStrength;
  BoatReactionStrength = comfortPreset.BoatReactionStrength;
}

Issue 2

public void OffsetVerlet(Vector3 offset)
{
  m_previousFrame = new Frame
  {
    Position = m_previousFrame.Position + offset,
    Time = m_previousFrame.Time,
  };
  m_currentFrame = new Frame
  {
    Position = m_currentFrame.Position + offset,
    Time = m_previousFrame.Time,
  };
}

The PVS-Studio warning: V3127 [CWE-682] Two similar code fragments were found. Perhaps, this is a typo and 'm_currentFrame' variable should be used instead of 'm_previousFrame' NPC_DynamicRigs.cs 868

Let's take a look at the common pattern in m_previousFrame and m_currentFrame initializations. But here the pattern breaks: in both cases, Time is assigned the m_previousFrame.Time value. In the m_currentFrame initialization, the assignment may contain an error—m_currentFrame.Time should be assigned instead of m_previousFrame.Time.

Issue 3

public class DepthNormalOnlyPass : ScriptableRenderPass
{
  ....
  internal void Render(RenderGraph renderGraph, 
    out TextureHandle cameraNormalsTexture, 
    out TextureHandle cameraDepthTexture, 
    ref RenderingData renderingData)
  {
    ....
  }
  ....
}

private void OnMainRendering(....)
{
  ....
  m_DepthNormalPrepass.Render(renderGraph, 
    out frameResources.cameraDepthTexture, 
    out frameResources.cameraNormalsTexture, 
    ref renderingData);
  ....
}

The PVS-Studio warning: V3066 [CWE-683] Possible incorrect order of arguments passed to 'Render' method: 'frameResources.cameraDepthTexture' and 'frameResources.cameraNormalsTexture'. UniversalRendererRenderGraph.cs 308

This time, it's a typo in the native Unity code. Compare the out parameter names with the fields passed into m_DepthNormalPrepass.Render. The depth and normal maps were swapped. Since these textures serve distinct goals, they aren't interchangeable. This error may cause noticeable glitches.

Error due to misunderstanding the '??' operator precedence

public Quaternion? GetCorrectionQuaternion(HumanBodyBones humanBodyBone)
{
  var targetData = ....;

  if (targetData == null)
  {
    return null;
  }
  if (!targetData.CorrectionQuaternion.HasValue)
  {
    return null;
  }
  return targetData.CorrectionQuaternion.Value;
}

private void RetargetHand(....)
{
  ....
  var correctionQuaternion = retargetingLayer.GetCorrectionQuaternion(....);
  ....

  Transform t = ....;

  t.rotation = pose.rotation * correctionQuaternion ?? Quaternion.identity;
  ....
}

The PVS-Studio warning: V3123 [CWE-783] Perhaps the '??' operator works in a different way than it was expected. Its priority is lower than priority of other operators in its left part. SyntheticHandRetargetingProcessor.cs 89

The analyzer flagged a potentially flawed expression:

t.rotation = pose.rotation * correctionQuaternion ?? Quaternion.identity;

To grasp the potential problem, let's look at this expression evaluation if correctionQuaternion is null.

1. The overloaded multiplication operation executes first. If one of the operands is null, the result will also be null.
2. Next, the result of the previous operation will be checked using??. Thus, t.rotation will be assigned the Quaternion.identity value.

The Quaternion.identity value essentially means no rotation, so, in the scenario above, the object rotation will be abruptly reset to zero. Most likely, the author expects different behavior, as a sudden rotation reset usually looks incorrect.

Perhaps, the developer would like to check correctionQuaternionwith ?? but overlooked that ?? always has lower precedence than *. The fixed expression may be as follows:

t.rotation = pose.rotation * (correctionQuaternion ?? Quaternion.identity);

Now, if correctionQuaternion is null, the object rotation will be assigned the pose.rotation * Quaternion.identity result. Since multiplying two quaternions performs the rotation of the first quaternion according to the rotation set by the second quaternion, this operation simply results in pose.rotation.

Conclusion

In this article, we delved into another fascinating open-source Unity VR project—NorthStar—and took a closer look at the bugs PVS-Studio uncovered in its code.

If you have your own project, perhaps you wonder: could the analyzer find bugs in it too? If so, don't hesitate to request a free trial key here. Check out a tutorial to run your first Unity project analysis here.

That's all for today. See you in future articles!