Вы когда-нибудь интересовались игровыми движками, написанными на Java? В этой статье мы рассмотрим и проверим на наличие ошибок в исходном коде один из популярных игровых движков — jMonkeyEngine. Возможно, мы даже узнаем, почему игры пишутся на C# и C++, а не на Java.

О проекте

Для описания проекта приведу данные с их сайта:

jMonkeyEngine — это современный и удобный для разработчиков игровой движок, написанный в основном на Java. Его минималистичный подход "сначала код" делает его идеальным для разработчиков, которым нужна поддержка игрового движка и сохранность полного контроля над своим кодом с возможностью расширения и адаптации движка к своему рабочему процессу.

Я собрал краткую характеристику и выделю несколько моментов, которые хочу упомянуть:

1. Использует по умолчанию LWJGL (графическая библиотека, предоставляющая доступ к API OpenGL и Vulkan). Отмечу, что использование DirectX движком не подразумевается по причине кроссплатформенности Java и ориентированности DX12 только на Windows.

2. Использует для моделирования физического пространства jBullet — порт библиотеки Bullet на Java. Мы дополнительно его проанализируем (я из будущего ворвусь и без интриги скажу, что там действительно есть подозрительные места).

3. Движок имеет SDK (именуют его jmonkeyplatform), основанный на IDE NetBeans. Он включает в себя редактор материалов, сцен, фильтров, лексер glsl-шейдеров. Его мы тоже обязательно проверим.

4. Разработчики, судя по всему, не используют анализаторы кода. Я могу оценить качество исходного кода как хорошее, но не без странных мест, о которых сейчас и расскажу. А про то, что игры не пишутся на Java, я пошутил :)

Ищем ошибки

Просто странности

Начнём утро с недостижимого return. Представляю вам фрагмент:

Файл 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) {
    ....
  }
}

Получили срабатывание:

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)

С точки зрения логики дублирующая проверка должна была бы иметь в своём теле другую сравниваемую с нулём переменную. Какую конкретно? На выбор у нас есть все поля этого класса, и поверьте мне — их много.

В комментариях к классу ListSort есть ссылки на С-шный код, на котором он основан. Вот ссылка. Сама функция в С-виде называется merge_hi, её несложно найти по названию:

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

Я склоняюсь к тому, что это лишний, "закопипащенный" код, нежели какая-либо ошибка, связанная с потерей проверки при портировании. Может быть, сыграло роль, что в Java нет оператора 'goto'?

Чудеса работ со списком

Попробуем найти ошибку в этом методе вместе?

Файл 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);
    }
  }
  ....
}

В качестве подсказки предлагаю вам посмотреть сообщение анализатора:

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

Блок else всегда приведёт к исключению IndexOutOfBoundsException. Сейчас объясню, почему так происходит. Попадая в блок else, мы создаём пустые коллекции и в цикле for пытаемся заменить их элементы на i (просто счётчик который должен бы был заполнить vertexMap и indexMap значениями от 0 до vCount). Но реализация метода set незаметно для разработчика внесла свои коррективы и может заменять только уже существующие элементы, а не добавлять новые. Поэтому этот код с нами дружить не будет, а будет выкидывать исключение.

Это место должно вызываться только в том случае, если индексы вершин в FBX модели отсутствуют, но формат является проприетарным, и официальной документации на него нет. Лично я черпал свои знания отсюда: тык, тык.

Интересно, можно ли найти модель FBX без указания индексов полигонов? Например, библиотека OpenFBX выкидывает ошибку, если у объектного файла индексы отсутствуют. Предлагаю обсудить этот вопрос в комментариях. Охотно готов с вами разобраться в этой дилемме.

Последствие рефакторинга

Следующее срабатывание тоже связано с чтением FBX формата:

Файл 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);
      }*/
    }
  }
}

Анализатор ругается на вызов метода get без использования его результата:

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

Странный цикл: в первом if мы не используем результат, а второй вообще закомментирован. Так как цикл в принципе не выполняет никаких полезных действий, то этот код стоит просто удалить. Проанализировав изменения класса, могу сказать, что это место появилось вследствие рефакторингов. Подметим актуальность статического анализа для поиска такого рода ошибок — он обязательно даст указание на места, требующие внимания после оптимизации кода.

Игра "найди проблему"

Предлагаю вам найти проблему вот тут:

Файл 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;
    }
  }
  ....
}

Здесь передаваемый параметр в сеттер-методе присваивается самому себе, и это действительно странно. Я думаю, разработчик опечатался и забыл добавить ключевое слово this. И, конечно же, анализатор нам об этом сообщит:

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

Проверяем JBullet

JBullet — порт физического движка реального времени Bullet на Java. Он не является прямой разработкой команды jmonkey, но форк одного из разработчиков их команды используется как сторонняя библиотека в основном проекте, поэтому её тоже стоит проверить.

Как заблудиться в трёх соснах

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

Файл 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; // <=
  }
  ....
}

Кажется, как будто разработчик здесь что-то напутал, и даже анализатор встал в стойку и сказал, что:

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

Изучив исходники оригинальной библиотеки, я узнал, что текущая ошибка — порождение исключительно java-порта. В оригинале для элементов используется функция с названием b3Swap. Я хочу, чтобы вы посмотрели, как она выглядит:

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

Что-то напоминает, не правда ли? :)

Учитывая реализацию на C++, которую мы уже увидели, определить исправление нам будет совсем не сложно:

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

Ссылка != значение

Представлю вам фрагмент кода, который мне кажется очень странным:

Файл 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;
}

Срабатывание диагностики:

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

Разработчик здесь допустил, наверное, одну из самых популярных по неприятности ошибку в Java — сравнение String переменных по ссылке, а не по значению.

Само сравнение может как работать, так и не работать. Суть в том, что Java при помощи механизма пула строк будет стараться сделать так, чтобы строки с одинаковым внутренним содержимым являлись одинаковыми объектами, но не факт, что получится :) Вполне возможен сценарий, что будут сравниваться объекты, которые имеют разные ссылки, при этом их содержимое одинаково. И в таком случае сравнение даст нам отрицательный результат, хотя мы ожидали истину.

Copy-Paste-ориентированное программирование

Давайте взглянем на самый частый пример Copy-Paste ошибок:

Файл ConeTwistConstraint.java(403)

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

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

В этом примере программист возвращает поле из метода, хотя по его сигнатуре было бы логично вернуть другое. Чаще всего такие ошибки появляются из-за невнимательности. Программист скопировал, поменял название метода, забыл поменять возвращаемое поле, и в итоге мы видим срабатывание анализатора. И да, кстати, вот оно:

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

Заключение

На этом на сегодня всё. Все ошибки я собрал как в статье, так и в pull request'ах для разработчиков этого проекта (тык, тык, тык). Уже второй раз мне удаётся помочь сообществу программистов, внося посильный вклад в Open Source проекты, и я этому очень рад.

А вы хотите попробовать статический анализ? Вы всегда можете внедрить его в свой проект, используя инструмент PVS-Studio. Скачать триальную версию можно здесь. Для открытых проектов существует вариант бесплатного лицензирования.

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Kirill Epifanov. Code of game engine written in Java: what does it hide?.