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

This is the second article in a short series that explores interesting VR games and showcasing code issues found within their source code using PVS\-Studio\. Meet NorthStar\!

![1279_NorthStarAnalysis/image1.png](https://import.viva64.com/docx/blog/1279_NorthStarAnalysis/image1.png)

## About the game

[NorthStar](https://github.com/oculus-samples/Unity-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\. 

![1279_NorthStarAnalysis/image2.png](https://import.viva64.com/docx/blog/1279_NorthStarAnalysis/image2.png)

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

Ropes for wrapping, swinging, and pushing objects\.

![1279_NorthStarAnalysis/image3.png](https://import.viva64.com/docx/blog/1279_NorthStarAnalysis/image3.png)

A spyglass for observing distant objects\.

![1279_NorthStarAnalysis/image4.png](https://import.viva64.com/docx/blog/1279_NorthStarAnalysis/image4.png)

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

![1279_NorthStarAnalysis/image5.png](https://import.viva64.com/docx/blog/1279_NorthStarAnalysis/image5.png)

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

![1279_NorthStarAnalysis/image6.png](https://import.viva64.com/docx/blog/1279_NorthStarAnalysis/image6.png)

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](https://pvs-studio.com/en/blog/posts/csharp/1278/), 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](https://pvs-studio.com/en/blog/posts/csharp/0744/);
* [PVS\-Studio in development of Unity projects: new specialized diagnostic rules](https://pvs-studio.com/en/blog/posts/csharp/1236/)\. 

The basic analyzer workflow is straightforward and involves three steps:

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

![1279_NorthStarAnalysis/image7.png](https://import.viva64.com/docx/blog/1279_NorthStarAnalysis/image7.png)

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

![1279_NorthStarAnalysis/image8.png](https://import.viva64.com/docx/blog/1279_NorthStarAnalysis/image8.png)

3\. Review warnings\. For large code bases, you can apply various filters and use [mass warning suppression](https://pvs-studio.com/en/docs/manual/0032/)—don't worry, you can revisit them later\. This way you can divide this process into several iterations\.

![1279_NorthStarAnalysis/image9.png](https://import.viva64.com/docx/blog/1279_NorthStarAnalysis/image9.png)

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

![1279_NorthStarAnalysis/image10.png](https://import.viva64.com/docx/blog/1279_NorthStarAnalysis/image10.png)

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](https://pvs-studio.com/en/docs/manual/0018/)\.

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

## About the code

The same as in the [previous article](https://pvs-studio.com/en/blog/posts/csharp/1278/), 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

```cpp
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](https://pvs-studio.com/en/docs/warnings/v3106/) \[CWE\-125\] Possible negative index value\. The value of 'currTaskIndex \- 1' index could reach \-2\. [TaskHandler\.cs 201](https://github.com/oculus-samples/Unity-NorthStar/blob/d7c4f7c61f0db96d006df3ed52a0c3268881bf7e/Packages/com.meta.utilities.narrative/Runtime/NarrativeSequence/TaskHandler.cs#L201)

The analyzer warns about possible negative index access\. Indeed, the code looks suspicious: for some reason, the `currTaskIndex != 0`check 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

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

The PVS\-Studio warning: [V3022](https://pvs-studio.com/en/docs/warnings/v3022/) \[CWE\-570\] Expression '\!CachedSphereCollider\.TryGet\(out var sphereCollider\)' is always false\. [NPC\_DynamicRigs\.cs 522](https://github.com/oculus-samples/Unity-NorthStar/blob/d7c4f7c61f0db96d006df3ed52a0c3268881bf7e/Assets/NorthStar/Animation/AnimationScripts/NPC_DynamicRigs.cs#L522)

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

```cpp
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

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

The PVS\-Studio warning: [V3095](https://pvs-studio.com/en/docs/warnings/v3095/) \[CWE\-476\] The 'm\_texturesWithoutMipmaps' object was used before it was verified against null\. Check lines: 91, 128\. [MipMapChecker\.cs 91](https://github.com/oculus-samples/Unity-NorthStar/blob/d7c4f7c61f0db96d006df3ed52a0c3268881bf7e/Assets/NorthStar/Editor/MipMapChecker.cs#L91) 

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:

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

### Unreliable null check

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

The PVS\-Studio warning: [V3216](https://pvs-studio.com/en/docs/warnings/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](https://github.com/oculus-samples/Unity-NorthStar/blob/d7c4f7c61f0db96d006df3ed52a0c3268881bf7e/Assets/NorthStar/Scripts/Vfx-Sfx/FadeVolume.cs#L25)

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:

![1279_NorthStarAnalysis/image11.png](https://import.viva64.com/docx/blog/1279_NorthStarAnalysis/image11.png)

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

```cpp
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](https://pvs-studio.com/en/docs/warnings/v3020/) \[CWE\-670\] An unconditional 'return' within a loop\. [MemberLinkDrawer\.cs 162](https://github.com/oculus-samples/Unity-NorthStar/blob/d7c4f7c61f0db96d006df3ed52a0c3268881bf7e/Packages/com.meta.utilities.narrative/Editor/MemberLinkDrawer.cs#L162)

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:

```cpp
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**

```cpp
[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](https://pvs-studio.com/en/docs/warnings/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](https://github.com/oculus-samples/Unity-NorthStar/blob/d7c4f7c61f0db96d006df3ed52a0c3268881bf7e/Assets/NorthStar/Scripts/System/GlobalSettings.cs#L70)\. 

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:

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



**Issue 2**

```cpp
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](https://pvs-studio.com/en/docs/warnings/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](https://github.com/oculus-samples/Unity-NorthStar/blob/d7c4f7c61f0db96d006df3ed52a0c3268881bf7e/Assets/NorthStar/Animation/AnimationScripts/NPC_DynamicRigs.cs#L868)

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**

```cpp
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](https://pvs-studio.com/en/docs/warnings/v3066/) \[CWE\-683\] Possible incorrect order of arguments passed to 'Render' method: 'frameResources\.cameraDepthTexture' and 'frameResources\.cameraNormalsTexture'\. [UniversalRendererRenderGraph\.cs 308](https://github.com/oculus-samples/Unity-NorthStar/blob/d7c4f7c61f0db96d006df3ed52a0c3268881bf7e/Packages/com.unity.render-pipelines.universal/Runtime/UniversalRendererRenderGraph.cs#L308) 

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

```cpp
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](https://pvs-studio.com/en/docs/warnings/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](https://github.com/oculus-samples/Unity-NorthStar/blob/d7c4f7c61f0db96d006df3ed52a0c3268881bf7e/Assets/NorthStar/Scripts/Player/SyntheticHandRetargetingProcessor.cs#L89)

The analyzer flagged a potentially flawed expression:

```cpp
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`\.
1. 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 `correctionQuaternion`with `??` but overlooked that `??` always has lower precedence than `*`\. The fixed expression may be as follows:

```cpp
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](https://pvs-studio.com/en/pvs-studio/try-free/)\. Check out a tutorial to run your first Unity project analysis [here](https://pvs-studio.com/en/docs/manual/6607/)\.

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