# Песочница ошибок: проверка игрового движка S&Box

Рынок современных игровых движков постепенно расширяется, и всё больше студий выбирают не кого\-то из двух гигантов \(учитывая последние события, вообще одного\), а движки поменьше\. Сегодня поговорим про одного из новичков индустрии — S&Box\. И это случай, когда новичок не такой простой, каким кажется\. Подробнее о проекте и о том, какие ошибки мы смогли найти с помощью PVS\-Studio, расскажем в статье\. 

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

## Пара слов о проекте

[S&Box](https://sbox.game/) — это новенький игровой движок от довольно известной студии Facepunch, которая подарила нам, можно сказать, культовые проекты — Rust и Garry's Mod\. Оба являются одними из самых продаваемых игр в Steam\. Но Garry's Mod играет тут гораздо более важную роль, чем просто одна из игр студии\. S&Box — это духовный наследник и полноценная версия всех идей, которые хотел видеть Гари Ньюман — создатель обоих игр — в Garry's mod\. 

S&Box не просто движок\. Это развлекательная платформа для творчества, в которой можно одновременно играть, создавать и даже монетизировать свой контент\.

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

Проекты в S&Box пишутся на новеньком C\# 14, а сам движок разработан на базе \.NET 10\. В качестве фич можно выделить поддержку изменений в реальном времени, представление логики с помощью визуальных графов \(no code\) и графы для шейдеров \(как в Blender\)\.


> \*\*Примечание\*\*\\\. Если вы хотите узнать о нововведениях C\\\# 14, то у нас есть \[обзорная статья\]\(https://pvs\-studio\.ru/ru/blog/posts/csharp/1301/\)\\\.



Из особенностей хотелось бы выделить, что это первый сторонний проект \(не от Valve\), который использует Source 2, так он теперь ещё и открытый\!

Естественно, мы не могли пройти мимо новенького C\# проекта, использующего все мощности новейшего \.NET\! Если вдруг кто\-то забыл или встречает нас впервые — мы команда статического анализатора PVS\-Studio, и мы любим проверять открытые проекты\. Это позволяет нам проверить способности анализатора и поделиться с вами результатами\.

Сегодня мы узнаем, почему полезно использовать статические анализаторы кода при разработке, особенно когда вы планируете открывать ваш проект для всего интернета \>:\)

Код проекта доступен на [GitHub](https://github.com/Facepunch/sbox-public) \(анализ проводился на одном из релизных коммитов — [f69cd48](https://github.com/Facepunch/sbox-public/commit/f69cd482fc09699bae64c3c64f9f8e64ab06119a)\)\.

## Проверка проекта

### Какие ошибки мы отобрали?

В статье мы будем разбирать срабатывания из вкладки BEST\.

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

Этот функционал используется в основном для первого просмотра срабатываний при пробном запуске анализатора, но его также можно использовать для поиска приоритетных срабатываний или как точку старта при начале работы с отчётом\. Выбор сценария за вами\.

Функционал содержит достаточно простой, но эффективный механизм\. Если кратко, то в анализаторе есть разделение на три уровня срабатываний \(High, Medium и Low\)\. Они отвечают за **уровень достоверности**\. 

А зачем он нужен? Всё просто\. Анализатор — это инструмент, который старается понять весь контекст вашего проекта, но технические ограничения не дают ему это сделать до конца\. Чтобы не перемешивать срабатывания, где анализатор почти на 100% уверен в наличии ошибки, и места, где он указывает на просто сомнительные части кода, которые могут не содержать ошибки, мы сделали разделение по трём разным уровням\.

При стандартной работе с анализатором мы советуем оставлять включёнными уровни High и Medium, а Low оставлять на случаи, когда вы можете уделить время изучению этих предупреждений\. В анализаторе заложен функционал, который всегда указывает на code smell, а это значит, что другой человек при работе с этим кодом тоже может принять его за ошибочный\. 

Если вы при работе с анализатором изучаете кодовую базу и проверяете, реально ли это ошибка или нет, то другой человек такое исследование проводить уже не будет :\)

### Время code review 

Начнём с небольшой разминки:

```cpp
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 );
  }
}
```

Попробуйте найти ошибку сами\.



<details>
   <summary>Ответ где\\\-то рядом\\\.\\\.\\\.</summary>

Предупреждение PVS\-Studio: V3001 There are identical sub\-expressions 'hEdgeA\.IsValid' to the left and to the right of the '&&' operator\. HalfEdgeMesh\.cs 2094


</details>
Если хорошо приглядеться, то ошибка становится очевидной\. 

В условии `If` происходит сравнение двух объектов со схожими названиями `hEdgeA` и `hEdgeB`\. Но как бы не происходит\.\.\. Из кода видно, что хотели именно так, но что\-то случилось и сравнили уже `hEdgeA` и `hEdgeA`, а результат, я думаю, все понимают\. Как раз в таких местах и происходят очепятки\. Много переменных и операций над ними, так они ещё и очень похожи\! Зачем мучить себя, если можно воспользоваться инструментом, который найдёт все подобные ошибки\.\.\.

### Расставляем приоритеты

```cpp
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: [V3177](https://pvs-studio.ru/ru/docs/warnings/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

Если приглядеться, то увидеть проблему легко, особенно с подсказкой от анализатора\. Давайте взглянем поближе и разберём, что произошло:

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

Скорее всего, разработчик предполагал такой порядок действий \(изобразим скобочками\):

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

Но вышло так:

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

Как можно догадаться из сообщения анализатора, дело в том, что оператор `&&` имеет больший приоритет, чем оператор `??`, и происходит сценарий, лишённый какого\-либо смысла\. 

Странно то, что в остальных местах всё корректно и проверка происходит правильно \(даже скобки используются\!\), но это скорее подтверждение ошибки, чем её оправдание\.

И, несмотря на всё это, самое неприятное в том, что ошибка повторилась\. Разработчик "скопипастил" кусок кода и тем самым распространил её дальше — это можно увидеть чуть ниже текущей ошибки:

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

### Опасный сценарий

```cpp
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());
  }
}
```

Как думаете, что не так с этим фрагментом? В одном из сценариев можно получить NRE\. К сожалению, не самая редкая ошибка\. 

Давайте сократим код и взглянем поближе:

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

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

Предупреждение PVS\-Studio: [V3105](https://pvs-studio.ru/ru/docs/warnings/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

Разработчик изначально предполагает, что в методе `FormatAssemblyName( Assembly asm )` параметр `asm` может быть `null` — ` FormatAssemblyName(asm?.GetName())`\. Но уже в `FormatAssemblyName( AssemblyName name )` такое не ожидается\.  У нас есть два сценария: первый — `name = null`, второй — `AssemblyNameFormatter = null` и `name = null`\.

В первом случае всё будет нормально, т\. к\. в `AssemblyNameFormatter` есть обработка такого сценария\. А во втором уже будет ошибка, поскольку `name` используется без проверки, и в момент вызова метода `name.ToString()`происходит NRE\.

### Про кого\-то забыли

```cpp
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 );
  }
}
```

В этом коде скрыта грустная история о забытой переменной и невыполненном методе из\-за отложенного выполнения\.

Предупреждение PVS\-Studio: [V3220](https://pvs-studio.ru/ru/docs/warnings/v3220/) The result of the 'Where' LINQ method with deferred execution is never used\. The method will not be executed\. MountHost\.cs 114

Если взглянем на переменную `types` и на значение, которое ей задаёт разработчик, то увидим выборку из определённых объектов:

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

Но отбора не произойдёт, т\. к\. `types` больше нигде в коде не упоминается\. Подобное поведение обусловлено тем, что при вызове `Where` фильтрация не выполняется в момент вызова, а является отложенной\. Следовательно, до тех пор, пока не будет произведено итерирование по коллекции, код делегата не выполнится\.

В представленном случае последствия не такие серьёзные, потому что от делегата в `Where` не ожидается дополнительных действий, но если код будет другим, то может потеряться важный функционал, например:

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



В метод `Where` передаётся делегат, который, помимо условия фильтрации, выводит на консоль элементы и их позиции из исходной коллекции\. Но, как вы уже поняли, `filteredNames` может не быть упомянут, по этой причине отфильтрованная коллекция не будет выведена на консоль\. Более того, фильтрация в принципе не выполнится\.

### Чрезмерная проверка

```cpp
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: [V3053](https://pvs-studio.ru/ru/docs/warnings/v3053/) An excessive expression\. Examine the substrings 'IESNA' and 'IESNA:LM\-63'\. Bitmap\.Loading\.Ies\.cs 242

В случае, если подстрока `IESNA` будет найдена, то дальнейшая проверка не выполнится\. Если подстрока `IESNA` не будет найдена, то и поиск более длинной подстроки `IESNA:LM-63` не имеет смысла\. Обычно мы советуем просто убрать излишний код, но тут может быть немного другая история\.

В коде идёт дополнительная проверка на `IESNA:LM-63`, и если разработчики хотели убедиться, что стандарт IESNA будет именно "LM\-63", нужно было написать так:

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

Это новый и старый форматы IESNA \(в современных версиях префикс просто `IES:), а текущая проверка не выполняет это условие и пропустит любую кодировку/версию стандарта.`

### Забытый Deserialize 

Попробуйте найти ошибку в коде:

```cpp
JsonSerializer.Deserialize<Vector3>( ref reader );
```

Получилось?

Давайте дам подсказку\.

<details>
   <summary>Сообщение PVS\\\-Studio\\\.\\\.\\\.</summary>

V3010 The return value of function 'Deserialize' is required to be utilized\. PolygonMesh\.Serialize\.cs 54


</details>


Я в вас не сомневался\!

Ладно, оставим шутки — надо разобраться\. У нас есть модификатор `ref` и логика и так отработает, может проблемы\-то и нет?

Дело в том, что это десериализация в этом фрагменте встречается часто\. Давайте взглянем:

```cpp
....
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 ) );

....
```

В коде ещё много такого\. Смысл в том, что возвращаемое значение используется для копирования и других действий, но лишь в одном месте его пропустили\. Очень подозрительный фрагмент, в котором, скорее всего, просто забыли ;\_;\.

### Релизный TODO

```cpp
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: [V3191](https://pvs-studio.ru/ru/docs/warnings/v3191/) Iteration through the 'results' collection makes no sense because it is always empty\. ConVarSystem\.AutoComplete\.cs 23

Анализатор указывает, что итерация по `results` не имеет смысла, т\. к\. коллекция всегда пустая\. И с ним тяжело не согласиться\! Ведь функционал, добавляющий что\-либо в эту коллекцию, был закомментирован\.

Учитывая остальные комментарии, можно понять две вещи: какая\-то логика присутствует и в коде смысл есть; часть функционала отправили в TODO\.

Казалось бы, тогда это и не ошибка, ведь всё осмысленно сделано\! Но у меня вопрос: считать ли такой случай плохой практикой? Нужно учитывать, что этот проект — база для многих других\. Логика и работа многих игр будет зависеть от того, как хорошо написаны исходники движка\. Этот фрагмент лежит с самого релиза, а на момент написания статьи прошло уже более полугода\.

Как вы считаете, это плохо или в этом ничего такого нет? Напишите своё мнение в комментариях\. 

### Мёртвая зона

```cpp
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: [V3040](https://pvs-studio.ru/ru/docs/warnings/v3040/) The 'triggerDeadzone' constant of the 'float' type is compared to a value of the 'int' type\. InputRouter\.Input\.cs 207

Здесь переменная `value` имеет тип `int`, и сравнение этой переменной со значением 0\.75f выглядит подозрительно, особенно с использованием оператора `>=`, т\. к\. ближайшие значения это 0 и 1\. Тем более странно видеть это в коде, связанном с кнопками контроллера, где важна точность\. И как раз тут мы работаем с "мёртвой зоной" кнопки, — важно понимать, где ТОЧНО она начинается\.

Сложно сказать, что именно хотел разработчик, учитывая, что переменная `value` имеет тип `int` и используется в других конструкциях\. Так что ошибка это или нет, решать уже авторам проекта\.

### Секунда Шрёдингера 

```cpp
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: [V3025](https://pvs-studio.ru/ru/docs/warnings/v3025/) Incorrect format\. A different number of format items is expected while calling 'Format' function\. Arguments not used: secs % 60\. NumberExtensions\.cs 102

Анализатор утверждает, что количество параметров не соответствует строке формата, в которую передаётся\. И мы можем в этом убедиться\.

В первой строке передаются секунды, и это происходит без проблем, всё корректно: 

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

Но затем мы можем заметить, что, начиная с третьей проверки, секунды передаются, но не используются:

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

Если посмотреть на код целиком, то секунды передаются **везде**, и есть вероятность, что это было сделано для читаемости, т\. к\. секунды используются в получении минут, часов и т\. д\.

Можно сказать, что анализатор ошибся, но он же прав, не так ли? :\)

Он не соврал, и в строку формата правда передаётся больше аргументов, чем ожидается\. Если в этом месте это было сделано специально, то какова вероятность, что в остальных случаях будет так же? Мы советуем вам не рисковать и лишний раз проверять код на случай непредусмотренных моментов\.

Ну а если вы на 100% уверены в своём коде, то всегда можете просто дать понять анализатору с помощью комментария, что тут можно не ругаться:

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

## Заключение

Несмотря на найденные ошибки, код проекта выглядит довольно качественно, а его команда старается исправлять проблемы или потенциальные ошибки как можно скорее \(если судить по GitHub\)\.

Если вам интересно, что ещё нашёл PVS\-Studio в S&Box \(в отчёте более 1000 ошибок\), или вы хотите проверить свой собственный проект, то скачать и попробовать анализатор можно [по ссылке](https://pvs-studio.ru/ru/pvs-studio/try-free/)\.


> \*\*Примечание\\\.\*\* А для тех, кто дочитал статью до конца, у нас есть бонус: \[возможность записаться на программу раннего доступа\]\(https://pvs\-studio\.ru/ru/pvs\-studio\-eap/\) к нашим новым анализаторам\\\! EAP доступно по направлениям JavaScript, TypeScript и Go\\\.