What's hidden in the code of a game engine written in Java?

Have you ever been interested in game engines written in Java? In this article we will review and check for errors in the source code of one of the popular game engines – jMonkeyEngine. We might even find out why games are written in C# and C++ rather than Java.

about the project

To describe the project, I will provide data from their website:

jMonkeyEngine is a modern, developer-friendly game engine written primarily in Java. Its minimalistic, code-first approach makes it ideal for developers who want game engine support and retain full control over their code, with the ability to extend and adapt the engine to their workflow.

I have compiled a brief description and will highlight a few points that I want to mention:

1. Uses LWJGL by default (a graphics library that provides access to the OpenGL and Vulkan APIs). I note that the use of DirectX by the engine is not implied due to the cross-platform nature of Java and the orientation of DX12 only on Windows.

2. Uses jBullet to model the physical space – a port of the Bullet library to java. We will analyze it further (I will burst in from the future and say without intrigue that there really are suspicious places there).

3. The engine has an SDK (called jmokeyplatform) based on the NetBeans IDE. It includes an editor for materials, scenes, filters, and a glsl shader lexer. We will definitely check it out too.

4. Developers don't seem to use code analyzers. I can rate the quality of the source code as good, but not without strange places, which I’ll tell you about now. And I was joking about the fact that games are not written in Java 🙂

Looking for errors

Just weirdness

Let's start the morning with the unattainable return. I present to you a fragment:

File ListSort.java(877)

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) {
    ....
  }
}

Got triggered:

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)

From a logical point of view, a duplicate check should have in its body another variable compared to zero. Which one specifically? We have all the fields in this class to choose from, and believe me, there are many of them.

In the comments to the class ListSort There are links to the C code on which it is based. Here link. The function itself in C-form is called merge_hiit’s easy to find by name:

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

I'm inclined to think that this is extra, “buried” code, rather than some kind of error related to the loss of verification during porting. Maybe it played a role that there is no 'goto' operator in Java?

The wonders of working with a list

Let's try to find the error in this method together?

File FbxMesh.java(334)

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, I suggest you look at the analyzer message:

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

Block else will always result in an exception IndexOutOfBoundsException. Now I will explain why this happens. Getting into the block elsewe create empty collections in a loop for we are trying to replace their elements with i (just a counter that should have been filled vertexMap and indexMap values ​​from 0 to vCount). But implementation of the set method made its adjustments unnoticed by the developer and can only replace existing elements, and not add new ones. Therefore, this code will not be friendly with us, but will throw an exception.

This location should only be called if there are no vertex indexes in the FBX model, but the format is proprietary and there is no official documentation for it. Personally, I got my knowledge from here: poke, poke.

I wonder if it is possible to find an FBX model without specifying polygon indices? For example, the OpenFBX library throws an error, if the object file has no indexes. I suggest discussing this issue in the comments. I am willingly ready to sort this dilemma out with you.

Consequence of refactoring

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

File FbxTexture.java(101)

@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 complains about calling a method get without using its result:

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

Strange cycle: in the first if we don't use the result, and the second one is commented out altogether. Since the loop, in principle, does not perform any useful actions, this code should simply be removed. Having analyzed the class changes, I can say that this place has appeared due to refactorings. Let us note the relevance of static analysis for finding this kind of errors – it will certainly give an indication of places that require attention after optimizing the code.

Game “find the problem”

I suggest you find the problem here:

File SelectionPropertyEditor.java(61)

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;
    }
  }
  ....
}

Here the passed parameter in the setter method is assigned to itself, and this is really strange. I think the developer made a typo and forgot to add a keyword this. And, of course, the analyzer will tell us about this:

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

Checking JBullet

JBullet – port of a real-time physics engine Bullet in Java. It is not a direct development of the jmonkey team, but fork one of their team's developers is used as a third-party library in the main project, so it's worth checking out as well.

How to get lost in three pines

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

File HashedOverlappingPairCache.java(233)

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

It seems as if the developer got something wrong here, and even the analyzer stood up and said that:

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

Having studied the source code of the original library, I learned that the current error is solely a product of the java port. In the original it is used for elements function with title b3Swap. I want you to see what it looks like:

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

Reminds me of something, doesn’t it? 🙂

Given the C++ implementation that we have already seen, it will not be difficult for us to determine the fix:

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

Link != value

Let me present you with a piece of code that seems very strange to me:

File CProfileNode.java(70)

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;
}

Diagnostic triggering:

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

The developer here made probably one of the most common and annoying mistakes in Java – comparison String variables by reference, not by value.

The comparison itself may or may not work. The point is that Java, using the mechanism string pool will try to make sure that strings with the same internal content are the same objects, but it’s not a fact that it will work 🙂 It is quite possible that objects that have different references will be compared, but their contents are the same. And in this case, the comparison will give us a negative result, although we expected the truth.

Copy-Paste-Oriented Programming

Let's take a look at the most common example of Copy-Paste errors:

File ConeTwistConstraint.java(403)

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, although based on its signature it would be logical to return something else. Most often, such errors appear due to inattention. The programmer copied, changed the name of the method, forgot to change the returned field, and as a result we see the analyzer triggered. And yes, 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 collected all the errors both in the article and in pull requests for the developers of this project (poke, poke, poke). This is the second time I’ve been able to help the programming community by making a contribution to Open Source projects, and I’m very happy about it.

Do you want to try static analysis? You can always implement it into your project using the PVS-Studio tool. You can download the trial version Here. For open source projects there is an option free licensing.

If you want to share this article with an English-speaking audience, please use the translation link: Kirill Epifanov. Code of game engine written in Java: what does it hide?.