Code review

archive development-discussion-jme2
1

I have looked at jMe library for the first time and while browsing code to get a feel of it, I have noticed some issues. I decided to write them down. If I’m wrong at any point, please tell me, it will help me to get the grasp of inner library workings.


  1. LWJGL*State classes have a lot of instance variables with state mappings (jMe->LWJGL). Why they are instance instead of static ? It looks like a big waste of memory for each state instance (example LWJGLAlphaState.glSrcBlend)


  2. Md2Model inner classes are non-static. Why ? Outer class reference is not used anyway.


  3. In LWJGLRenderer, GL_NORMALIZE is always used. Maybe it would make sense to check if scale of model is different from 1 and enable it only then ? Or even better, implement none/GL_RESCALE_NORMAL/GL_NORMALIZE depending on scaling type ? AFAIK, GL_NORMALIZE is quite expensive operation.


  4. In FastMath, all coefficients should be moved to static variables outside methods - in other case, they will get allocated every time method is called.


  5. TriMesh looks a assymetrical to me. On one hand, it supports Vector2/3f/Color for vertices, with FloatBuffer for array access, on other, face indices are organized in plain int[]. Shouldn’t some kind of Face[] or TriangleIndices[] array be used ? Or make vertices reside in float[] ?


  6. Is there any reason for synchronization of deletedEdges in ClodCreator ? If not, then it might be better to use unsynchronized ArrayList


  7. LWJGLFragment/VertexProgramState load method is very… interesting. Creating new Byte for each byte of input is a really neat way of testing gc performance :wink: ByteArrayOutputStream for temporary array would be a lot better choice IMHO.


  8. In Fragment/VertexProgramState, shouldn’t the constructor be new float[96][] instead of new float[96][4] ? Currently, all parameter entries will be initialized to float[4], which is a waste of memory, plus will cause unnecessary binding of 96 variables each call.


  9. It looks like almost everything implements Serializable. It would probably make sense to specify serialVersionUID for all serializable classes - without it, even compiling library in different environment an cause incompatible changes.


  10. Do you plan non-static VBO ? For some use cases (like particles) recreating entire buffer each frame looks like terrible waste. Is there some way in which updateable FloatBuffer for geometry could be exposed for DYNAMIC/STREAMING VBO ?
2

Thank you very much for these suggestions, as this is a community project it’s hard to always do peer reviews like we should.


  1. Good point, over sight from the very beginning. :slight_smile: When JDK 5 is official, I’m thinking of making those enums, but making them static will save a lot.


  2. Md2Model is going away (we now have a jme model loader and exporters to the jme format). So that problem will go away with that.


  3. I’ll look into that… don’t have any reason off hand why I did that.


  4. Good point.


  5. I’m going to look into TriMesh (as well as a couple other classes). It is one that has grown over time and some loose ends need to be tied up.


  6. Can’t comment, didn’t write that.


  7. Again, can’t comment, didn’t write it.


  8. ditto


  9. Interesting, didn’t realize that.


  10. Didn’t implement VBO, can’t comment.



    Thanks again for the comments, nice to have constructive comments.
3

I can comment directly on these:



3. you’re right, we should turn it off for x=y=z=1



6. No, just a carry over from the original c implementation.



10. Yeah, non-static VBO is planned but even static vbo has been… interesting from card to card. We’ll get there though.



EDIT: Implemented 3 & 6, but did not notice any significant changes. Perhaps under certain circumstances. Checked in.

4
"renanse" wrote:
EDIT: Implemented 3 & 6, but did not notice any significant changes. Perhaps under certain circumstances. Checked in.

For 3, it is very hardware dependent. Anyway, it should be visible only for triangle-bound scenes, which are probably very hard to achieve these days.

EDIT:
http://www.opengl.org/resources/features/KilgardTechniques/oglpitfall/

glEnable(GL_NORMALIZE);

This mode is not enabled by default because it involves several additional calculations. Enabling the mode forces OpenGL to normalize transformed normals to be of unit length before using the normals in OpenGL's lighting equations. While this corrects potential lighting problems introduced by scaling, it also slows OpenGL's vertex processing speed since normalization requires extra operations, including several multiplies and an expensive reciprocal square root operation.

I was writing it basing on this info - I have not done any real-world tests to check if GL_NORMALIZE is slow in reality on current GPUs.
5

Made the changes for 1.

6

I put serialization in the last release. I’ll check into 9.