Author: Kirill Epifanov

Have you ever wondered about game engines written in Java? jMonkeyEngine is such an engine, and a popular one. In this article, we will look at it and check its source code for errors. We may even find out why games are usually written in C# and C++ instead of Java.

About project

Here is some information about the project from its website:

jMonkeyEngine is a modern, developer-friendly game engine written primarily in Java. Its minimalist, code-first approach makes it ideal for developers who need game engine support while retaining full control over their code and the capability to extend and customize the engine to fit their workflow.

I've put together a brief summary and highlighted a few things I'd like to mention:

1. The engine uses LWJGL (a graphics library that provides access to the OpenGL and Vulcan APIs) by default. I'd like to note that the engine doesn't support DirectX due to the cross-platform nature of Java and the Windows-only focus of DX12.

2. It uses jBullet, a Java port of the Bullet library, to model physical space. We'll analyze it as well (the future me breaks in and ruins the intrigue by saying that there are some suspicious fragments, indeed).

3. The engine has an SDK (they call it jmokeyplatform) based on the NetBeans IDE. It provides a material editor, scene editor, filter editor, and a glsl-shader lexer. Of course, we'll have a look at it, too.

4. Apparently, developers don't use code analyzers. I can rate the quality of the source code as good, but not without some strange fragments. I'll tell you about them bellow. Oh, and don't take my words about Java gamedev to heart, just kidding.

Looking for errors

• To check the project, we'll use the PVS-Studio static analyzer. The article author is one of its developers.
• You'll see code examples as you read. I've shortened most of them so as not to overwhelm you. I've also put an ellipsis to mark the abbreviated code: "....".
• At the time of the check, the latest revision was the e584cb1 commit. We checked it using the static analyzer.
• You can find all checked sources and other source files on which all the judgments in the article are based — the links are permanent.
• The article contains only the errors that seemed interesting to its author (yep, imho). If you'd like to see other errors, you can always download the analyzer and check the project yourself.

Just some weird stuff

Let's start our morning with an unreachable return. Here is a fragment:

The ListSort.java(877) file

public void innerMergeHigh(Comparator<T> comp, 
                           T[] tempArray, T[] arr, int idxA) {
  lengthA--;
  if (lengthA == 0 || lengthB == 1) {
    return;
  }
  if (lengthB == 1) {
    return;
  }
  while (true) {
    ....
  }
}

The analyzer issues a warning:

V6039 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. ListSort.java(874), ListSort.java(877)

Logically, a duplicate check should have a variable other than zero in its body. Which one, exactly? We have all the fields of this class to choose from, and believe me, there are many.

In the comments to the ListSort class, there are links to the C code on which it's based. Here's the link. The function is called merge_hi in its C form. You can easily find it by its name:

static Py_ssize_t merge_hi(....) {
  ....
  --na;
  if (na == 0)
    goto Succeed;
  if (nb == 1)
    goto CopyA;
  ....
}

I think it's just extra, copy-pasted code rather than an error due to a missing check during porting. Maybe Java not having a 'goto' operator made a difference?

Wonders of list handling

Shall we try to find an error in this method?

The FbxMesh.java(334) file

private List<Geometry> createGeometries() throws IOException {
  Mesh mesh = new Mesh();
  mesh.setMode(Mode.Triangles);
  // Since each vertex should contain unique texcoord 
  // and normal we should unroll vertex indexing
  // So we don't use VertexBuffer.Type.Index for elements drawing
  // Moreover quads should be triangulated (this increases number of vertices)
  if(indices != null) {
    ....
  } else {
    // Stub for no vertex indexing (direct mapping)
    iCount = vCount = srcVertexCount;
    vertexMap = new ArrayList<>(vCount);     // <=
    indexMap = new ArrayList<>(vCount);
    reverseVertexMap = new ArrayList<>(vCount);
    for (int i = 0; i < vCount; ++i) {
      vertexMap.set(i, i);                   // <=
      indexMap.set(i, i); 
      List<Integer> l = new ArrayList<>(1);
      l.add(i);
      reverseVertexMap.add(l);
    }
  }
  ....
}

As a hint, have a look at the warning issued by the analyzer:

V6025 Index 'i' is out of bounds. FbxMesh.java(336)

The else block always throws the IndexOutOfBoundsException exception. Let me explain why this is the case. Once in the else block, we create empty collections and try to replace their elements with i (just a counter that should have filled vertexMap and indexMap with values from 0 to vCount) in the for loop. However, the set method implementation has made its own adjustments unnoticed by the developer. It can replace only existing elements, not add new ones. As a result, the code doesn't work as intended but throws an exception.

This code should be called only if there are no vertex indices in the FBX model. However, the format is proprietary, and there is no official documentation for it. I've learned about it here and there.

I'm wondering: is it possible to find an FBX model without specified polygon indices? For example, the OpenFBX library throws an error if an object file has no indices. I encourage you to discuss this matter in the comments. I'll happily work out this dilemma with you.

Consequences of refactoring

The next warning is also related to reading the FBX format:

The FbxTexture.java(101) file

@Override
public void fromElement(FbxElement element) {
  super.fromElement(element);
  if (getSubclassName().equals("")) {
    for (FbxElement e : element.children) {
      if (e.id.equals("Type")) {
        e.properties.get(0);
      } 
      /*else if (e.id.equals("FileName")) {
        filename = (String) e.properties.get(0);
      }*/
    }
  }
}

The analyzer warns us against calling the get method without using its result:

V6010 The return value of function 'get' is required to be utilized. FbxTexture.java(101)

It's a weird loop: in the first if, we don't use the result, and the second is commented out. Since the loop does nothing useful, we could just delete this code. After analyzing the class changes, I can say that this fragment is the result of refactoring. Let's note the importance of static analysis for detecting such errors: it will certainly show you the fragments that need attention after code optimization.

"Find an issue" game

I invite you to find the issue in the following code:

The SelectionPropertyEditor.java(61) file

public class SelectionPropertyEditor implements PropertyEditor {
  ....
  private String value;

  public SelectionPropertyEditor(String[] tags, String value) {
    this.tags = tags;
    this.value = value;
  }

  public void setValue(Object value) {
    if (value instanceof String) {
      value = (String) value;
    }
  }
  ....
}

In the setter method, the passed parameter is assigned to itself, and that's really weird. I think the developer made a typo and forgot to add the this keyword. Of course, the analyzer notifies us:

V6026 This value is already assigned to the 'value' variable. SelectionPropertyEditor.java(61)

Let's check JBullet

JBullet is a Java port of the Bullet real-time physics engine. It's not developed directly by the jmonkey team, but a fork from one of the team members is used as a third-party library in the main project, so it's worth a check as well.

How to lose one's way in broad daylight

Using a temporary variable to swap objects between each other seems like common practice, but not in this case:

The HashedOverlappingPairCache.java(233) file

@Override
public BroadphasePair findPair(BroadphaseProxy proxy0, BroadphaseProxy proxy1) {
  BulletStats.gFindPairs++;
  if (proxy0.getUid() > proxy1.getUid()) {
    BroadphaseProxy tmp = proxy0;
    proxy0 = proxy1;
    proxy1 = proxy0; // <=
  }
  ....
}

It seems like the developer has messed something up here. Even the analyzer issued the following:

V6026 This value is already assigned to the 'proxy1' variable. HashedOverlappingPairCache.java(233)

After looking at the original library source code, I learned that the current error was made only in the Java port. In the original code, a function called b3Swap is used for elements. Let's take a look at it:

template <typename T>
B3_FORCE_INLINE void b3Swap(T &a, T &b)
{
  T tmp = a;
  a = b;
  b = tmp;
}

It reminds you of something, doesn't it? :)

Given the C++ implementation we've already seen, we can easily find a way to fix the error:

@Override
public BroadphasePair findPair(BroadphaseProxy proxy0, BroadphaseProxy proxy1) {
  BulletStats.gFindPairs++;
  if (proxy0.getUid() > proxy1.getUid()) {
    BroadphaseProxy tmp = proxy0;
    proxy0 = proxy1;
    proxy1 = tmp;
  }
  ....
}

Reference != value

Let's take a look at the code fragment that I find very strange:

The CProfileNode.java(70) file

public CProfileNode getSubNode(String name) {
  // Try to find this sub node
  CProfileNode child = this.child;
  while (child != null) {
    if (child.name == name) {          // <=
      return child;
    }
    child = child.sibling;
  }

  // We didn't find it, so add it
  ....
  return node;
}

The analyzer has issued a warning:

V6013 Strings 'child.name' and 'name' are compared by reference. Possibly an equality comparison was intended. CProfileNode.java(70)

The developer here has made probably one of the most common and ugly errors in Java — comparing String variables by reference rather than by value.

The comparison may or may not work. Using the string pool mechanism, Java tries to ensure that strings with the same internal content are the same objects, but it may not work :) It seems quite possible that objects with different references are compared while their content is the same. In this case, the comparison gives us false, even though we were expecting true.

Copy-paste-oriented programming

Let's look at the most common example of a copy-paste error:

The ConeTwistConstraint.java(403) file

public class ConeTwistConstraint extends TypedConstraint {
  ....
  private boolean solveTwistLimit;
  private boolean solveSwingLimit;
  ....

  public boolean getSolveTwistLimit() {
    return solveTwistLimit;
  }

  public boolean getSolveSwingLimit() {
    return solveTwistLimit; // <=
  } 
}

In this example, the programmer returns a field from a method. However, based on the method signature, it would make sense to return a different field. Most often, such errors occur due to inattention. The developer copied and changed the method name but forgot to change the return field. As a result, the analyzer issued a warning. By the way, here it is:

V6091 Suspicious getter implementation. The 'solveSwingLimit' field should probably be returned instead. ConeTwistConstraint.java(407), ConeTwistConstraint.java(74)

Conclusion

That's all for today. I've collected all the errors, both in the article and in pull requests for the project developers (click, click, click). This is the second time I've been able to help the programming community by contributing to open-source projects. I'm very happy about it.

Would you like to try static analysis? You can always introduce it into your project using the PVS-Studio tool. You can download the trial version here. We offer free licensing for open-source projects.