Stride Game Engine error review
Tags: csharp, unity3d, gamedev, opensource
Published: 2022-10-07 09:03:15

Stride is a free, feature\-packed and cross\-platform game engine implemented in C\#\. Stride may certainly become an alternative to Unity, but what about the quality of its source code? Let's check Stride Game Engine with the help of the PVS\-Studio static analyzer\.

![Image description](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/8uvs29utx1m1a91pmdei.png)

## Introduction

Since Stride is a rapidly developing project, it not only acquires new functions and features, but inevitably gets more and more bugs\. In this article we're inspecting the most curious ones\. In fact, the analysis of errors in the project is an excellent way to assess the quality of its code\. However, it's not that easy to check major projects of this kind, that's why we're going to use the PVS\-Studio static code analyzer to spot bugs in Stride\.

We checked the Stride Game Engine 4\.1\.0\.1734 version\. The source code is available on [GitHub](https://github.com/stride3d/stride/releases/tag/releases%2F4.1.0.1734)\.

## Stride Game Engine error review

### Anonymous function is used to unsubscribe from an event

**Issue 1**

```csharp
private void RemoveEditor(....)
{
  ....
  if (....)
  {
    multiEditor.OpenedAssets.CollectionChanged -= (_, e) => 
      MultiEditorOpenAssetsChanged(multiEditor, e);
    ....
  }
  ....
}
```

PVS\-Studio warning: [V3084](https://pvs-studio.com/en/docs/warnings/v3084/?utm_source=website&utm_medium=dev_to&utm_campaign=article&utm_content=0994)\. Anonymous function is used to unsubscribe from 'CollectionChanged' event\. No handlers will be unsubscribed, as a separate delegate instance is created for each anonymous function declaration\. AssetEditorsManager\.cs 444

In the case above, an anonymous function is used to unsubscribe from the *CollectionChanged* event\. However, no handlers are unsubscribed, since an anonymous function is used\. The declaration of an anonymous function creates a new delegate instance, different from the one previously subscribed to *CollectionChanged*\. Even if these two functions have identical bodies, they still represent different objects\. Thus, the hadler is not unsubscribed through an anonymous function and the value of *CollectionChanged* does not change\.

### Possible null dereference while accessing the property

**Issue 2**

```csharp
private void SetIsVisible(bool isVisible)
{
  if (isVisible)
    PreviewService.OnShowPreview(); //<=
  else
    PreviewService.OnHidePreview(); //<=
}
```

PVS\-Studio warning: [V3080](https://pvs-studio.com/en/docs/warnings/v3080/?utm_source=website&utm_medium=dev_to&utm_campaign=article&utm_content=0994)\. Possible null dereference\. Consider inspecting 'PreviewService'\. PreviewViewModel\.cs 109

In this case, the analyzer detected a possible null reference dereference\. Pay attention to the *SetIsVisible* method\. There's no check for null for the *PreviewService* property\. The implementation of this property indicates the importance of the null check:

```csharp
private IAssetPreviewService PreviewService
{
  get
  {
    if (previewService != null)
      return previewService;

    previewService = ServiceProvider.TryGet<IAssetPreviewService>();
    if (previewService == null)
      return null;                   

    previewService.PreviewAssetUpdated += PreviewAssetUpdated;
      return previewService;
  }
}
```

It's clear that the *PreviewService* property can actually return *null*\. That's why the risk of null reference dereference is real\. We can solve it by adding the condition:

```csharp
private void SetIsVisible(bool isVisible)
{
  if (PreviewService != null)       //<=
  {
    if (isVisible)
      PreviewService.OnShowPreview(); 
    else
      PreviewService.OnHidePreview(); 
  }
}
```

### Redundant value assignment

**Issue 3**

```csharp
public TexAtlas(...., TexImage atlas) : base(....)
{
  ....
  Name = atlas.Name;
  ....
  Name = "";
}
```

PVS\-Studio warning: [V3008](https://pvs-studio.com/en/docs/warnings/v3008/?utm_source=website&utm_medium=dev_to&utm_campaign=article&utm_content=0994)\. The 'Name' variable is assigned values twice successively\. Perhaps this is a mistake\. TexAtlas\.cs 48, 43

The *Name* variable is assigned values twice in the constructor\. The developers may have decided to test the class functions with the empty *Name* property\. That's why they added the second assignment but later forgot to delete it\. Now, regardless of the constructor's input parameters, an empty string is always assigned for this property\. This will inevitably break the logic of the program that uses the value of this property\.

### The null check is missed

**Issue 4**

```csharp
public override void EndScreenDeviceChange(....)
{
  ....
  window.FullscreenIsBorderlessWindow = FullscreenIsBorderlessWindow; // <=
  if (....)
  {
    ....
    if (window != null)
    {
      window.ClientSize = new Size2(clientWidth, clientHeight);
    }
    ....
  }
  else
  {
    ....
    if (window != null)
    {
      window.ClientSize = new Size2(clientWidth, clientHeight);
      window.Location = savedFormLocation;
      ....
      window.BringToFront();
    }

  }
  ....
}
```

PVS\-Studio warning: [V3095](https://pvs-studio.com/en/docs/warnings/v3095/?utm_source=website&utm_medium=dev_to&utm_campaign=article&utm_content=0994)\. The 'window' object was used before it was verified against null\. GameWindowSDL\.cs 64, 70\.

At the beginning of the *EndScreenDeviceChange* method, the *window* object is used before it was checked for *null*\. The analyzer's warning is relevant enough according to the rest of the code: the variable is accessed inside the conditional operator that checks for null\.

When checking C\# projects, the analyzer warns about potential errors that result in an exception of the *NullReferenceException* type far more often\. It would be better to keep this in mind when coding\. Let's check another similar warning*\.*

**Issue 5**

```csharp
private async Task Update()
{
  while (....)
  {
    ....
    if (....)
    {
      ....
      materialFilterRenderFeature.MaterialFilter =
        (materialFilterRenderFeature != null && ....)? ....
      ....
    }
  }
}
```

PVS\-Studio warning: [V3095](https://pvs-studio.com/en/docs/warnings/v3095/?utm_source=website&utm_medium=dev_to&utm_campaign=article&utm_content=0994)\. The 'materialFilterRenderFeature' object was used before it was verified against null\. EditorGameRenderModeService\.cs 106, 107\.

In contrast to the previous case, there is a check for *null*, but it's\.\.\. a bit odd\. The *MaterialFilter* property of the *materialFilterRenderFeature* object is accessed\. Then, the result of the ternary operator execution is assigned to this property\. The operator's conditional expression checks *materialFilterRenderFeature* for *null*\. But there's no need for this check\. If *materialFilterRenderFeature* equals to *null*, the program execution results in *NullReferenceException* while accessing the *MaterialFilter* property\.

### When the check for null results in NullReferenceException

**Issue 6**

```csharp
public Palette GlobalPalette
{
  get {....}
  set
  {
    SetTagValue("GlobalPalette", (value != null) ? null : value.Data);
  }
}
```

PVS\-Studio warning: [V3080](https://pvs-studio.com/en/docs/warnings/v3080/?utm_source=website&utm_medium=dev_to&utm_campaign=article&utm_content=0994)\. Possible null dereference\. Consider inspecting 'value'\. MetadataModels\.cs 132

The ternary operator is passed as a second argument to the *SetTagValue* method\. This operator checks the *value* variable for *null*\. If *value* equals *null*, the *value\.Data *is passed as an argument to the method*\.* Accessing this variable's property will throw *NullReferenceException*\. The developers may have mixed up the operands\. Therefore, the correct implementation of the ternary operator may be as follows:

```csharp
(value != null) ? value.Data : null
```

### Same conditional statements with a different purpose

**Issue 7**

```csharp
internal IObjectNode SetTarget(....)
{
  if (....)
  {
    var targetValue = targetNode.Retrieve();

    if (targetValue != null && !type.IsInstanceOfType(targetValue))
      throw new InvalidOperationException(@"The type of the retrieved" +
                                           "node content does not match" +
                                           "the type of this reference");

    if (targetValue != null && !type.IsInstanceOfType(targetValue))
      throw new InvalidOperationException("TargetNode type does not " +
                                          "match the reference type.");

    ....
  }
  ....
}
```

PVS\-Studio warning: [V3021](https://pvs-studio.com/en/docs/warnings/v3021/?utm_source=website&utm_medium=dev_to&utm_campaign=article&utm_content=0994)\. There are two 'if' statements with identical conditional expressions\. The first 'if' statement contains method return\. This means that the second 'if' statement is senseless\.ObjectReference\.cs 134

The analyzer detected two similar *if statements* with identical conditions\. An exception is thrown in both conditional statements, thereby interrupting the program execution\. It seems like there's nothing wrong here, just a bit of redundant code\. However, pay attention to the exceptions' error messages, they are different\. The developers probably intended to implement two different checks, that's why they have copied the check and changed the message but forgot to change the condition\. Thus, one possible error remained unhandled and still poses a threat to the full and correct operation of the program\.

According to the second exception's error message, the correct implementation of the second *if* condition is the following:

```csharp
if (targetNode!= null && !type.IsInstanceOfType(targetNode))
  throw new InvalidOperationException("TargetNode type does not " +
                                      "match the reference type.");
```

Let's take a look at a similar error detected by the analyzer\.

**Issue 8**

```csharp
private GraphViewModel(....) : base(serviceProvider)
{
  ....
  if (rootPresenters == null) 
    throw new ArgumentNullException(nameof(rootPresenters));
  ....
  if (rootPresenters == null) 
    throw new ArgumentNullException(nameof(rootNode));
  ....
}
```

PVS\-Studio warning: [V3021](https://pvs-studio.com/en/docs/warnings/v3021/?utm_source=website&utm_medium=dev_to&utm_campaign=article&utm_content=0994)\. There are two 'if' statements with identical conditional expressions\. The first 'if' statement contains method return\. This means that the second 'if' statement is senseless\. GraphViewModel\.cs 40

Again, we have two *if* statements with identical conditions\. As in the previous case, the arguments passed to the exception's constructor indicate the error\. But in this case the arguments are the names of the variables checked\. It's clear that the second conditional statement contains an error\. Its correct implementation may look like this:

```csharp
if (rootNode == null)
  throw new ArgumentNullException(nameof(rootNode));
```

### Copy paste errors or which case operator is the third wheel?

**Issue 9**

```csharp
public override void AddPoint(....)
{
  ....
  switch (Component)
  {
    case VectorComponent.X:
      value.X = realPoint.Y;
      break;

    case VectorComponent.Y:
      value.Y = realPoint.Y;
      break;

    case VectorComponent.Z:
      value.Z = realPoint.Y; // <=
      break;

    case VectorComponent.W:
      value.Z = realPoint.Y; // <=
      break;

    default:
      throw new NotSupportedException();
  }
  ....
}
```

PVS\-Studio warning: [V3139](https://pvs-studio.com/en/docs/warnings/v3139/?utm_source=website&utm_medium=dev_to&utm_campaign=article&utm_content=0994)\. Two or more case\-branches perform the same actions\.  RotationComponentCurveViewModel\.cs 53

Here is the *switch* statement that has two similar case operators\. Both the case labels match the *VectorComponent\.\{0\}* template, which can have any of the following properties instead of *\{0\}*: *X*, *Y*, *Z*, or *W*\. The *value\.\{0\} = realPoint\.Y* assignment is executed in every *case* operator\. Let's take a look at the last case operator\. It has the *VectorComponent\.W* label, and the *value\.Z = realPoint\.Y* operation is executed within the boundaries of the operator\. This operation is identical to the assignement in the previous *case* operator\. According to the template we identified, this statement should contain *value\.W* instead of *value\.Z*\. 

Thus, the correct implementation of the last *case* may be as follows:

```csharp
case VectorComponent.W:
  value.W = realPoint.Y; 
  break;
```

### Don't spare code to handle NullReferenceException

**Issue 10**

```csharp
protected override Size ArrangeOverride(....)
{
  ....
  if (....)
  {
    for (....)
    {
      var child = itemsControl.ItemContainerGenerator
                              .ContainerFromIndex(i) as TreeViewItem;
      ....
      if (child != null)
      {
        child.Arrange(....);
        currentY += child.ActualHeight;
      }
      ....
    }
  }
  else
  {
    for (....)
    {
      var child = itemsControl.ItemContainerGenerator
                              .ContainerFromIndex(i) as UIElement;
            
      child?.Arrange(....);
      currentY += child.DesiredSize.Height; //<=
    }
  }
  ....
}
```

PVS\-Studio warning: [V3125](https://pvs-studio.com/en/docs/warnings/v3125/?utm_source=website&utm_medium=dev_to&utm_campaign=article&utm_content=0994)\. The 'child' object was used after it was verified against null\. VirtualizingTreePanel\.cs 430, 429

Let's inspect *then* and *else* blocks of the conditional statement\. They are somewhat similar, aren't they? In both blocks, the loop is executed, and the *child* variable is initialized through the *as* operator within the executed loop\. However, there is one major difference: the *child* variable access is protected from *NullReferenceException* with the null check in the first loop, unlike the second loop\. The developers may not have added the check for null for the second loop intentionally\. That's why they processed the first access to the *child* variable with the *null\-conditional* operator:

```csharp
child?.Arrange(....);
```

At the same time, they probably didn't pay attention to the second access to the variable:

```csharp
currentY += child.DesiredSize.Height;
```

This operation can still result in *NullReferenceException*\.

## Conclusion

The analyzer detected some potentially dangerous code fragments in the Stride Game Engine, that turned out to be pretty curious\. But only developers can determine the actual errors in these code fragments by analyzing their project with the help of PVS\-Studio\. 

In any case, no vulnerability is much better than having one\. The static analyzer allows you to quickly find errors and thus minimize their number in the project without wasting a lot of time and effort searching for errors manually\.

You can easily see it yourself and try the PVS\-Studio trial version on your projects [for free](https://pvs-studio.com/pvs-studio/try-free/?utm_source=website&utm_medium=dev_to&utm_campaign=article&utm_content=0994&utm_term=link_try-free)\.

But first, we recommend you to learn the basics of using the PVS\-Studio analyzer\. Check the [documentation](https://pvs-studio.com/en/docs/?utm_source=website&utm_medium=dev_to&utm_campaign=article&utm_content=0994) for more info\.

I hope you enjoyed reading this article :\)

Good luck and clean code to you\! Thank you and see you soon\!