Contribution review

Troubleshooting terramonkey
1

Hello,

I made a simple HeightMap object for my project. But before doing more serious contributions, I would like a small review from jMonkey’s team to give me directions about what’s right and what’s wrong in this code. Thanks

[java]package com.jme3.terrain.heightmap;

import java.io.IOException;

import java.io.ObjectInputStream;

import java.io.ObjectOutputStream;

import java.io.Serializable;

import com.jme3.math.FastMath;

import com.jme3.terrain.heightmap.AbstractHeightMap;

/**

  • <code>RawArrayHeightMap</code> creates a height map directly from a float
  • Array.

    *
  • @author OzoneGrif
  • @version $Id$

    */

    public class RawArrayHeightMap extends AbstractHeightMap implements Serializable

    {

    /

    *

    */

    private static final long serialVersionUID = -4212916737162257164L;

    /
  • Creates an empty heightMap

    *
  • @param size

    */

    public RawArrayHeightMap( int size )

    {

    this.size = size;

    heightData = new float[ size * size ];

    }

    /**
  • Creates a heightMap from a pre-existing float Array

    */

    public RawArrayHeightMap( float[] buffer ) throws Exception

    {

    size = (int) FastMath.sqr( buffer.length );

    if ( size * size == buffer.length )

    heightData = buffer;

    else

    throw new Exception( "buffer size must be 2^n" );

    }

    /**
  • Not needed for <code>RawArrayHeightMap</code>, will return false if not
  • initialized

    */

    @Override

    public boolean load()

    {

    return ((size > 0) && (heightData != null));

    }

    /**
  • Checks if both heightMaps are strictly identical

    */

    @Override

    public boolean equals( Object o )

    {

    if ( o instanceof AbstractHeightMap )

    {

    RawArrayHeightMap rhm = (RawArrayHeightMap) o;

    if ( (size == rhm.size) && (heightScale == rhm.heightScale) && (filter == rhm.filter) )

    {

    for ( int i = 0 ; i < heightData.length ; i++ )

    if ( heightData[ i ] != rhm.heightData[ i ] )

    return false;

    return true;

    } else

    return false;

    } else

    return false;

    }

    /**
  • Returns an unique hashCode strictly linked to this heightMap’s data

    */

    @Override

    public int hashCode()

    {

    int hashCode = (size ^ Float.floatToIntBits( heightScale ) ^ Float.floatToIntBits( filter ));

    for ( int i = 0 ; i < heightData.length ; i++ )

    hashCode ^= Float.floatToIntBits( heightData[ i ] );

    return hashCode;

    }

    /**
  • Serializes the heightMap to the stream

    *
  • @param out
  • @throws IOException

    */

    private void writeObject( ObjectOutputStream out ) throws IOException

    {

    out.defaultWriteObject();

    out.writeInt( size );

    out.writeFloat( heightScale );

    out.writeFloat( filter );

    for ( int i = 0 ; i < heightData.length ; i++ )

    out.writeFloat( heightData[ i ] );

    }

    /**
  • Creates the heightMap from a stream

    *
  • @param in
  • @throws IOException
  • @throws ClassNotFoundException

    */

    private void readObject( ObjectInputStream in ) throws IOException, ClassNotFoundException

    {

    in.defaultReadObject();

    size = in.readInt();

    heightScale = in.readFloat();

    filter = in.readFloat();

    heightData = new float[ size ];

    for ( int i = 0 ; i < heightData.length ; i++ )

    heightData[ i ] = in.readFloat();

    }

    }

    [/java]
2

I have an Exception in BinaryImporter :

[java]public Savable readObject(int id) {

[…]

try {

int loc = locationTable.get(id);[/java]

locationTable.get returns null … and null can’t be stored in a native type int.



It happens while trying to load my heightMapData

[java] BinaryExporter.getInstance().save( heightMapChunk, out ); // Works okay

heightMapChunk = (RawArrayHeightMap) BinaryImporter.getInstance().load( in ); // Exception

[/java]

My savable code :

[java] @Override

public void write( JmeExporter ex ) throws IOException

{

OutputCapsule oc = ex.getCapsule( this );

oc.write( size, “size”, 0 );

oc.write( heightScale, “heightScale”, 1.0f );

oc.write( filter, “filter”, 0.5f );

oc.write( heightData, “heightData”, null );

}





@Override

public void read( JmeImporter im ) throws IOException

{

InputCapsule ic = im.getCapsule( this );

ic.readInt( “size”, 0 );

ic.readFloat( “heightScale”, 1.0f );

ic.readFloat( “filter”, 0.5f );

ic.readFloatArray( “heightData”, null );

}

[/java]



Any idea ?

3

You are using the wrong serialization class. You should implement Savable.

4

Looks good ozonegrif. Javadoc too :slight_smile:

If you switch it to Savable instead of Serializable we can add this to the code base.



Thanks for the contribution!

5

I modified my code to give full control over the stream to BinaryImporter. It doesn’t crash anymore on that line. Importer probably lost its sync to the stream for some reason… sensitive little boy :slight_smile:



Now It fails to instanciate the class. I guess I have to register a serialization object.



Checking this out.

6

Okay, it had nothing to do with a serialization object. It just wanted a public constructor with no parameters.

My tests shows that BinaryImporter is about 5 times slower than the Java system, and that Vectors are merged to the same instance (is it a normal optimization ?)



Here’s the code :

[java]package com.jme3.terrain.heightmap;



import java.io.IOException;



import com.jme3.export.InputCapsule;

import com.jme3.export.JmeExporter;

import com.jme3.export.JmeImporter;

import com.jme3.export.OutputCapsule;

import com.jme3.export.Savable;

import com.jme3.math.FastMath;



/**

  • <code>RawArrayHeightMap</code> creates a height map directly from a float
  • Array.

    *
  • @author OzoneGrif
  • @version $Id$

    */

    public class RawArrayHeightMap extends AbstractHeightMap implements Savable

    {

    /**
  • Empty constructor, used by the <code>Savable</code> implementation.
  • Do not use manually.

    */

    public RawArrayHeightMap()

    {

    }





    /**
  • Creates an empty heightMap

    *
  • @param size

    */

    public RawArrayHeightMap( int size )

    {

    this.size = size;

    heightData = new float[ size * size ];

    }





    /**
  • Creates a heightMap from a pre-existing float Array

    */

    public RawArrayHeightMap( float[] buffer ) throws Exception

    {

    size = (int) FastMath.sqr( buffer.length );

    if ( (size * size) == buffer.length )

    heightData = buffer;

    else

    throw new Exception( "buffer size must be 2^n" );

    }





    /**
  • Not needed for <code>RawArrayHeightMap</code>, will return false if not
  • initialized

    */

    @Override

    public boolean load()

    {

    return ((size > 0) && (heightData != null));

    }





    /**
  • Checks if both heightMaps are strictly identical

    */

    @Override

    public boolean equals( Object o )

    {

    if ( o instanceof AbstractHeightMap )

    {

    RawArrayHeightMap rhm = (RawArrayHeightMap) o;

    if ( (size == rhm.size) && (heightScale == rhm.heightScale) && (filter == rhm.filter) )

    {

    for ( int i = 0 ; i < heightData.length ; i++ )

    if ( heightData[ i ] != rhm.heightData[ i ] )

    return false;



    return true;

    } else

    return false;

    } else

    return false;

    }





    /**
  • Returns an unique hashCode strictly linked to this heightMap’s data

    */

    @Override

    public int hashCode()

    {

    int hashCode = (size ^ Float.floatToIntBits( heightScale ) ^ Float.floatToIntBits( filter ));

    for ( float element : heightData )

    hashCode ^= Float.floatToIntBits( element );



    return hashCode;

    }





    /**
  • Save the heightMap for a BinaryExporter

    *
  • @param out
  • @throws IOException

    */

    @Override

    public void write( JmeExporter ex ) throws IOException

    {

    OutputCapsule oc = ex.getCapsule( this );

    oc.write( size, "size", 0 );

    oc.write( heightScale, "heightScale", 1.0f );

    oc.write( filter, "filter", 0.5f );

    oc.write( heightData, "heightData", null );

    }





    /**
  • Load the heightMap for a BinaryImporter

    *
  • @param in
  • @throws IOException
  • @throws ClassNotFoundException

    */

    @Override

    public void read( JmeImporter im ) throws IOException

    {

    InputCapsule ic = im.getCapsule( this );

    ic.readInt( "size", 0 );

    ic.readFloat( "heightScale", 1.0f );

    ic.readFloat( "filter", 0.5f );

    ic.readFloatArray( "heightData", null );

    }

    }

    [/java]
7

Yes, it also references the spatial objects etc. when they are the same.

8

added to svn, thanks OzoneGrif

9

The reading does not set the variables. It won’t work

E.g. this:

[java]ic.readInt( “size”, 0 );[/java]

Needs to be this:

[java]size = ic.readInt( “size”, 0 );[/java]

10

hah, good catch

11

Thanks a lot for the code review.

And sploreg for the integration and fixes.

12

Hello,



A made a minor modification in TerrainGrid to prevent it from using its cache.

[java] protected boolean useCache = true;



public void run() {

for (int i = 0; i < 4; i++) {

for (int j = 0; j < 4; j++) {

[…]

if (q == null && (useCache || isCenter(quadIdx))) {

[…]

cache.put(temp, q);

}

[…]

}

}

public void setUseCache( boolean useCache )

{

this.useCache = useCache;

if ( !useCache )

cache.clear();

}

[/java]

I’m also looking for a way to deactivate LOD in a TerrainQuad.

13

just don’t add the LOD control and LOD won’t be active.



I would like to change TerrainGrid to have adjustable cache size. But I guess for now being able to turn it off would be an okay option.

14

About the cache code above, I think it keeps old Quads in cache. I changed setCache(), but I don’t like how I’ve done it :

[java] public void setUseCache( boolean useCache )

{

this.useCache = useCache;

if ( !useCache )

cache = new LRUCache<Vector3f, TerrainQuad>(4);

else

cache = new LRUCache<Vector3f, TerrainQuad>(16);

}[/java]



About LOD, I can’t just remove TerrainLODControl() because the Grid wants to know the camera’s position.

And using Terrain.update( cameras ) automatically generates LOD.

15

okay, then we need to separate the two.

Actually, if TerrainGrid uses the camera for position, it can get it from the render() method of the control.

16

I think I discovered another bug with the cache in TerrainGrid (or LRUCache). I had a strange behaviour where the Grid would reload one of the close Quads, although it was not necessary.

Then I remembered that cache.put(temp, q) which was in a strange position in code ; and I now understand why. I guess anthyon wanted to push the Quad on top of the LinkedHashMap, am I right ?

Then I have a bad news, because that won’t work. To push an object on top of the list, you have to remove it first, then put it back in. I made the same mistake in my code :wink:



This should be the fix, in LRUCache :

[java] public synchronized V get(K key) {

V value = this.map.get(key);

if ( value != null )

{

this.map.remove( key );

this.map.put( key, value );

}

return value;

}





public synchronized void put(K key, V value) {

this.map.remove( key );

this.map.put(key, value);

}

[/java]