Our website uses cookies to enhance your browsing experience.
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!
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.
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.
Update.Learn more on this topic in these articles:
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!
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!
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.
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.
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)
{
....
}
}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.
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;
}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.
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.
null, the result will also be null.??. 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.
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!
0
0
0
76
0
36
0
139
1
0