Issues in com.jme3.terrain.geomipmap.TerrainLodControl

While browsing class TerrainLodControl I noticed it has some issues. (Bugs, excessive creation of new objects, code style). I’d like to help improving it. I’ll add patches to this post over time, if I may.

Code for checking if cameras where moved between updates doesn’t work properly if more than one camera is added to the object.



Fix (please review):

[patch]

Index: src/terrain/com/jme3/terrain/geomipmap/TerrainLodControl.java

===================================================================

— src/terrain/com/jme3/terrain/geomipmap/TerrainLodControl.java (revision 9327)

+++ src/terrain/com/jme3/terrain/geomipmap/TerrainLodControl.java (working copy)

@@ -221,17 +221,16 @@

return updatedPatches != null && !updatedPatches.isEmpty();

}


  • private boolean lastCameraLocationsTheSame(List<Vector3f> locations) {
  •    boolean theSame = true;<br />
    
  •    for (Vector3f l : locations) {<br />
    
  •        for (Vector3f v : lastCameraLocations) {<br />
    
  •            if (!v.equals(l) ) {<br />
    
  •                theSame = false;<br />
    
  •                return false;<br />
    
  •            }<br />
    
  • private boolean lastCameraLocationsTheSame(List<Vector3f> currentCameraLocations) {
  •   assert (currentCameraLocations.size() == lastCameraLocations.size());  // Same size at this point.<br />
    

+

  •    for (int i = 0; i &lt; currentCameraLocations.size(); i++) {<br />
    
  •        if (!currentCameraLocations.get(i).equals(lastCameraLocations.get(i))) {<br />
    
  •            return false;<br />
    

}

}

  •    return theSame;<br />
    

+

  •    return true;  // No differences found. Locations are the same as before.<br />
    

}



private synchronized boolean isLodCalcRunning() {

[/patch]

I know it’s an unlikely scenario but this code will still fail if two cameras both swap position and index.



(i.e. remove last two cameras from the list, add them back in the opposite order with swapped positions).



You may need to use a hashmap of camera->previous location to store the previous settings although that will introduce a lot of object creation if not done carefully.

The scenario you’re describing can not really happen as the class copies the location vectors of the cameras passed through the constructor into a new list the first time the control is updated. When using setCameras() the class immediately copies the location vectors. (Not at all good coding style, by the way.)



My implementation has an error, though, where the assertion in lastCameraLocationsTheSame() is not true when setCameras() is used to update the cameras and the number of cameras changes. My fault. I’ll fix that.



Anyway.

I hope I don’t offend anyone when I say that this class is quite a mess. Apart from its bugs, it creates a minimum of 5 new objects for every frame. (2 less if none of the cameras moved.) For every additional camera at least 2 objects per frame are created.



I’m planning to redo this class as an exercise and will add a patch to this forum post.

I’m sure your efforts will be appreciated. :slight_smile:

Sure post the completed patch and I will review it.

As for multiple cameras, only one is really supported with the LOD right now. Maybe in the future I will add in the multi-camera support for split screen.