S&Box game engine: Inspecting grains of sand

The market for modern game engines is steadily growing; more and more studios are choosing smaller engines rather than one of the two major players (or, given recent events, just one). Today, let's discuss one of the newcomers to the industry, S&Box. In this case, the newcomer isn't as simple as it seems. To learn more about the project and the errors we detected using PVS-Studio, continue reading the article.

About the project

S&Box is a brand-new game engine from the well-known Facepunch studio, which brought us iconic projects like Rust and Garry's Mod. Both are among the best-selling games on Steam. However, Garry's Mod plays a much more significant role here than being just one of the studio's games. S&Box is the fully realized spiritual successor to all the ideas Garry Newman—the creator of both Garry's Mod and S&Box—wanted to see in Garry's Mod.

S&Box is more than just a game engine. It's an entertainment platform for expressing creativity, where you can play, create, and even monetize your content.

S&Box projects are written in the latest C# 14, and the engine is built on .NET 10. Key features include real-time update support, action graphs (no-code), and shader graphs similar to those in Blender.

Note. If you'd like to learn about the new features in C# 14, we have an overview article.

Notably, this is the first third-party project (not from Valve) to use Source 2, and S&Box is now open-source as well!

Of course, we couldn't overlook the brand-new C# project that leverages all the power of the latest .NET! In case you've forgotten, or if this is your first time joining us, we're the team behind PVS-Studio static analyzer, and we love checking open-source projects! This enables us to evaluate the analyzer capabilities and share the results with you.

Today, we'll discuss the benefits of using static code analyzers during development, especially if you plan to share your project online >:)

The project code is available on GitHub; the analysis is performed on one of the release commits, f69cd48.

Let's check the project

What errors did we pick?

In this article, I'll break down warnings from the BEST tab.

This feature is useful for providing an initial warning overview during a test run of the analyzer. It can also be used to identify high-priority warnings or as a starting point when reviewing a report. The way you use it is up to you.

The feature incorporates a fairly simple yet efficient mechanism. In short, the analyzer has three warning levels (High, Medium, and Low). These are certainty levels.

Why might you need them? That's simple: the analyzer is a tool that aims to understand the full context of a project, but technical limitations prevent it from doing so. To avoid confusing warnings where the analyzer is nearly 100% certain an error exists with those where it merely points out suspicious code that may not contain errors, we divided them into three levels.

For the standard analyzer workflow, we recommend keeping the High and Medium levels enabled and leaving the Low one for situations when you have time to review the warnings. The analyzer includes a feature that always highlights code smells that other people working with the code may also mistake for an error.

If you look at the code while working with the analyzer to determine whether it's actually an error or not, others may not bother doing so :)

It's time to review code

Let's start with a quick warm-up:

public bool CollapseEdge(....)
{
  ....
  GetVerticesConnectedToHalfEdge(
    hFullEdge,
    out var hVertexA,
    out var hVertexB );

  var hEdgeA = hFullEdge;
  var hEdgeB = hFullEdge.OppositeEdge;

  var hFaceA = hEdgeA.Face;
  var hFaceB = hEdgeB.Face;
  ....
  // Disconnect the edge that is being collapsed
  // from the faces and other edges.
  Assert.True(hEdgeA.IsValid &&  hEdgeB.IsValid );
  if ( hEdgeA.IsValid && hEdgeA.IsValid )
  {
    var pNewVertex = hNewVertex;

    var hNextEdgeA = hEdgeA.NextEdge;
    var hPrevEdgeA =
      FindPreviousEdgeInFaceLoop( hEdgeA );

    var hNextEdgeB = hEdgeB.NextEdge;
    var hPrevEdgeB =
      FindPreviousEdgeInFaceLoop( hEdgeB );
  }
}

Try to find an error here.

The answer is just around the corner...

PVS-Studio warning: V3001 There are identical sub-expressions 'hEdgeA.IsValid' to the left and to the right of the '&&' operator. HalfEdgeMesh.cs 2094

If you look closely, the error becomes obvious.

In the If condition, two objects with similar names, hEdgeA and hEdgeB, are compared. Well, in reality, they aren't... The code shows that this was the intended behavior. However, something went wrong: hEdgeA was compared to itself, and I think the result is obvious. Such fragments often contain typos. There are many variables and operations involving them, and they are quite similar! Why struggle when you can use a tool that detects such errors?

Setting priorities

private void RewriteTypeNames(....)
{
  ....
  foreach ( var attribute in node.Attributes )
  {
    ....
    if ( attribute.BoundAttribute?.IsGenericTypedProperty() 
         ?? false && attribute.TypeName != null ) {....}

    else if ( attribute.TypeName == null &&      
      (attribute.BoundAttribute?.IsDelegateProperty() ?? false) ) {....}

    else if ( attribute.TypeName == null &&
      (attribute.BoundAttribute?.IsEventCallbackProperty() ?? false) ) {....}

    else if ( attribute.TypeName == null ) {....}
  }
  ....
}

PVS-Studio warning: V3177 The 'false' literal belongs to the '&&' operator with a higher priority. It is possible the literal was intended to belong to '??' operator instead. ComponentGenericTypePass.cs 346

If we look closely, it's easy to spot the issue, especially with a hint from the analyzer. Let's dig a little deeper and see what's wrong:

attribute.BoundAttribute?.IsGenericTypedProperty() 
  ?? false && attribute.TypeName != null

Most likely, developers had the following order in mind (as shown in parentheses):

(attribute.BoundAttribute?.IsGenericTypedProperty() 
  ?? false) && attribute.TypeName != null

However, this is how it turned out:

attribute.BoundAttribute?.IsGenericTypedProperty() 
  ?? (false && attribute.TypeName != null)

As the analyzer message suggests, && has a higher precedence than ??, which results in a scenario that makes no sense.

It's strange that everything else is correct, and the check works properly (even the parentheses are used!), but that only confirms the error.

The worst part is that the error occurred again despite all this. The developers copied and pasted the code snippet, thus spreading it further—you can see this just below the error:

private void RewriteTypeNames(....)
{
  ....
  foreach ( var childContent in node.ChildContents )
  {
    if ( childContent.BoundAttribute?.IsGenericTypedProperty()
         ?? false && childContent.TypeName != null ) {....}

     else if ( childContent.IsParameterized ){....}
     else{....}
  }  
  ....
}

A dangerous scenario

public partial class Hotload
{
  public void AddUpgrader( IInstanceUpgrader upgrader )
    {....}

  public void AddUpgrader<TUpgrader>()
    where TUpgrader : IInstanceUpgrader, new()
  {
    AddUpgrader( new TUpgrader() );
  }

  public IInstanceUpgrader GetUpgrader( Type upgraderType )
    {....}

  ....

  internal static string FormatAssemblyName( AssemblyName name )
  {
    return AssemblyNameFormatter?
             .Invoke( name ) ?? name.ToString();
  }

  internal static string FormatAssemblyName( Assembly asm )
  {
  return FormatAssemblyName(asm?.GetName());
  }
}

What do you think is wrong with this code fragment? NRE can occur in one of the scenarios. Unfortunately, this error is quite common.

Let's shorten the code and take a closer look:

internal static string FormatAssemblyName( AssemblyName name )
{
  return AssemblyNameFormatter?
           .Invoke( name ) ?? name.ToString();
}

internal static string FormatAssemblyName( Assembly asm )
{
  return FormatAssemblyName(asm?.GetName());
}

PVS-Studio warning: V3105 The result of null-conditional operator is dereferenced inside the 'FormatAssemblyName' method. NullReferenceException is possible. Inspect the first argument 'asm?.GetName()'. Hotload.cs 324

The developers expected the asm parameter to be null in the FormatAssemblyName(Assembly asm) method: FormatAssemblyName(asm?.GetName()). However, they didn't expect that in FormatAssemblyName( AssemblyName name ). There are two scenarios: the first is name = null, and the second is AssemblyNameFormatter = null and name = null.

In the first case, everything works fine, since AssemblyNameFormatter handles the scenario. Meanwhile, the second one is erroneous because name is used without being checked, and an NRE occurs when the name.ToString() method is called.

They forgot something

internal void UnregisterTypes( Assembly assembly )
{
  var assetSourceType = typeof( BaseGameMount );
  var types = assembly
                .GetTypes().Where( t => 
                  assetSourceType.IsAssignableFrom( t ) 
                  && !t.IsAbstract );

  foreach ( var (ident, source) in Sources.ToArray() )
  {
    if ( source.GetType().Assembly != assembly ) continue;
    if ( source.IsMounted ){ PendingMounts.Add( ident ); }

    source.ShutdownInternal();
    Sources.Remove( ident );
  }
}

This code tells a sad story of a forgotten variable and an unexecuted method due to deferred execution.

PVS-Studio warning: V3220 The result of the 'Where' LINQ method with deferred execution is never used. The method will not be executed. MountHost.cs 114

If we look at the types variable and the value the developers assigned to it, we'll see a selection of certain objects:

var types = assembly
                .GetTypes().Where( t => 
                  assetSourceType.IsAssignableFrom( t ) 
                  && !t.IsAbstract );

However, no filtering will happen because types isn't mentioned anywhere else in the code. This happens because calling Where doesn't execute the filtering immediately—it uses deferred execution. So, the delegate code won't be executed until the collection is iterated over.

In this case, the consequences aren't that serious because no further action is expected from the delegate in Where. However, if the code were different, important features could be lost, for example:

int it = 0;
var filteredNames = names.Where(name => 
                    {
                      ++it;
                      Console.WriteLine($"{it}) {name}");
                      return name.StartsWith(startLetter); 
                    });

A delegate that both applies a filter condition and prints elements along with their positions from the original collection to the console is passed to the Where method. However, as you've probably already noticed, filteredNames may not be specified, so the filtered collection won't be printed to the console, and filtering won't occur at all.

An over-check

public static bool IsIes( byte[] data )
{
  if ( data == null || data.Length < 10 )
    return false;

  var header = Encoding.ASCII.GetString
    ( data, 0, Math.Min( 20, data.Length ) ).Trim();

return 
   header.StartsWith( "IESNA" ) 
   || header.StartsWith( "IESNA:LM-63" );
}

PVS-Studio warning: V3053 An excessive expression. Examine the substrings 'IESNA' and 'IESNA:LM-63'. Bitmap.Loading.Ies.cs 242

If the IESNA substring is found, no further checks will be performed. If the IESNA substring isn't found, then there's no point in searching for the longer IESNA:LM-63 substring. We usually recommend removing redundant code, but this case may be a little different.

The code includes an additional check for IESNA:LM-63. If the developers wanted to ensure that the IESNA standard version was exactly "LM-63," they should've written it this way:

return header.StartsWith("IESNA:LM-63") 
       || header.StartsWith("IES:LM-63");

These are the new and old IESNA formats (modern versions use the IES: prefix), but the current check doesn't meet this condition and will allow any encoding or version of the standard.

Forgotten Deserialize

Can you spot an error in the following snippet?

JsonSerializer.Deserialize<Vector3>( ref reader );

Did you find it?

Let me give you a hint.

PVS-Studio warning: V3010 The return value of function 'Deserialize' is required to be utilized. PolygonMesh.Serialize.cs 54

I never doubted you!

All right, jokes aside—let's get to the bottom of this. We have the ref modifier, and the logic works fine anyway—maybe this isn't really an issue?

Deserialization occurs frequently in this code snippet. Let's take a look:

....
else if ( propertyName == "TextureOrigin" )
{
  if ( reader.TokenType == JsonTokenType.StartArray )
  {
    mesh
   .TextureOriginUnused
   .CopyFrom( JsonSerializer.Deserialize<Vector3[]>( ref reader ) );
  }
  else
  {
    JsonSerializer.Deserialize<Vector3>( ref reader );   // <=
  }
}
else if ( propertyName == nameof( TextureCoord )
{
  mesh
 .TextureCoord
 .CopyFrom( JsonSerializer.Deserialize<Vector2[]>( ref reader ) );

  hasTextureCoords = true;
}

else if ( propertyName == "TextureRotation" )
  mesh
  .TextureRotationUnused
  .CopyFrom( JsonSerializer.Deserialize<Rotation[]>( ref reader ) );

else if ( propertyName == nameof( TextureUAxis ) )
  mesh
  .TextureUAxis
  .CopyFrom( JsonSerializer.Deserialize<Vector3[]>( ref reader ) );

....

There are many more examples like this in the code. The point is that the return value is used for copying and other operations, but it was omitted just once. This is a very suspicious fragment, where they most likely just forgot ;_;.

Release TODO

public static ConCmdAttribute.AutoCompleteResult[]
  GetAutoComplete(....)
{
  var parts = partial.SplitQuotesStrings();

  List<ConCmdAttribute.AutoCompleteResult> results = new();

  // if we have more than one part, complete a specific command
  if ( parts.Length > 1 )
  {
    if ( !Members.TryGetValue( parts[0], out var command ) 
      return Array
             .Empty<ConCmdAttribute.AutoCompleteResult>();

  //results.Add( new ConCmd.AutoCompleteResult 
  //  { Command = command.Name, Description = command.Help } );

  // TODO - dig into it for auto complete
  return results.Take( count ).ToArray();
}

PVS-Studio warning: V3191 Iteration through the 'results' collection makes no sense because it is always empty. ConVarSystem.AutoComplete.cs 23

The analyzer points out that iterating over results is pointless, since the collection is always empty. And it's hard to argue with that! After all, the feature that adds anything to this collection is commented out.

Based on other comments, we can draw two conclusions: the code does contain some logic and a clear intent, but part of the functionality remains marked as TODO.

At first glance, that might not seem like an error—everything looks deliberate. I have a question, though: should we consider this case a poor example? Keep in mind that this project serves as a foundation for many others. The behavior and logic of numerous games will depend on how well the engine's source code is written. This fragment has been there since the release, and at the time of writing, more than six months have passed.

What do you think: does this count as an issue, or is it acceptable? Share your thoughts in the comments.

A deadzone

internal static void OnGameControllerAxis(
  int deviceId,
  GameControllerAxis axis,
  int value )
{

  controller.SetAxis( axis, value );

  ....

  const float triggerDeadzone = 0.75f;

  // I hate this but okay
  GamepadCode code =
    axis switch
    {
      GameControllerAxis.TriggerLeft =>
        GamepadCode.LeftTrigger,

      GameControllerAxis.TriggerRight =>
        GamepadCode.RightTrigger,

      _ =>
        GamepadCode.None,
    };

  OnGamepadCode(deviceId, code, value >= triggerDeadzone );
}

PVS-Studio warning: V3040 The 'triggerDeadzone' constant of the 'float' type is compared to a value of the 'int' type. InputRouter.Input.cs 207

The value variable here has the int type, so comparing it to 0.75f seems suspicious, especially when the >= operator is used, since the nearest values are 0 and 1. It's even more unusual to encounter such an issue in code related to controller input, where precision matters. In this case, we're dealing with the button's deadzone, so it's crucial to know exactly where it starts.

It's hard to say what the developers had in mind given that the value variable is an int and is used in other constructs. So, it's up to the project authors to decide whether this is an error or not.

Schrödinger's second

public static string FormatSecondsLong(
  this long secs )
{
  var m =System.Math.Floor( (float)secs / 60.0f );

  var h =System.Math.Floor( (float)m / 60.0f );

  var d =System.Math.Floor( (float)h / 24.0f );

  var w =System.Math.Floor( (float)d / 7.0f );

  if ( secs < 60 )return string.Format("{0} seconds",secs );

  if ( m < 60 )return string.Format("{1} minutes,
                                     {0} seconds",
                                     secs % 60,
                                     m );

  if ( h < 48 )
    return string.Format(
      "{2} hours and {1} minutes", 
        secs % 60, 
        m % 60, 
        h );

  if ( d < 7 )
    return string.Format(
      "{3} days, {2} hours and {1} minutes",
        secs % 60,
        m % 60,
        h % 24,
        d );

  return string.Format(
    "{4} weeks, {3} days, {2} hours and {1} minutes",
      secs % 60,
      m % 60,
      h % 24,
      d % 7,
      w );
}

PVS-Studio warning: V3025 Incorrect format. A different number of format items is expected while calling 'Format' function. Arguments not used: secs % 60. NumberExtensions.cs 102

The analyzer reports that the number of parameters doesn't match the format string to which they are being passed. Let's see for ourselves.

Seconds are passed in the first line, and this works fine, everything is correct:

if ( secs < 60 )return string.Format("{0} seconds",secs );

Then, starting from the third check, the seconds are passed but not used:

if ( h < 48 )
  return string.Format(
    "{2} hours and {1} minutes", 
      secs % 60, 
      m % 60, 
      h );

If we look at the code as a whole, we can see that seconds are passed everywhere. The developers may have done this to improve readability, since seconds are used to calculate minutes, hours, and so on.

One might say the analyzer is wrong, but it's actually right :)

It didn't lie: the format string does take more arguments than expected. If this was intentional there, what are the chances that the same will be true in other cases? To avoid any risks, consider double-checking your code to catch any unexpected issues.

If you're completely confident in your code, you can use a comment to inform the compiler that it's not an issue:

if ( h < 48 ) return string.Format( 
  "{2} hours and {1} minutes", secs % 60, m % 60, h ); //-V3025

Conclusion

Despite the errors we found, the project code looks polished. Its team is working hard to fix issues and potential bugs as quickly as possible (judging by their GitHub).

If you're curious to see more of what PVS-Studio detected in S&Box (the report lists over 1,000 errors), or if you'd like to check your own project, you can download and try the analyzer by clicking the link.

Note. You've made it this far, and we want to reward you: here's your chance to sign up for an early access program for our new analyzers! The EAP is available for JavaScript, TypeScript, and Go analyzers.