lwjglRenderer small things

As I tried to find the problem with my dynamic cubemap, I found a couple of things in that class. It’s nothing really big, but I think it should be looked at.

The first point is a missing break;

The second point is useless cycles being spent on creating a SafeArrayList and not using it, and then creating a toArray. Could we please remove one? Thanks. :slight_smile:

The rest is just IDE warning removals. (Blame my OCD) :stuck_out_tongue:

[java]

This patch file was generated by NetBeans IDE

It uses platform neutral UTF-8 encoding and \n newlines.

— Base (BASE)
+++ Locally Modified (Based On LOCAL)
@@ -55,7 +55,6 @@
import com.jme3.texture.Texture.WrapAxis;
import com.jme3.util.BufferUtils;
import com.jme3.util.ListMap;
-import com.jme3.util.NativeObject;
import com.jme3.util.NativeObjectManager;
import com.jme3.util.SafeArrayList;
import java.nio.*;
@@ -1168,13 +1167,13 @@
if (GLContext.getCapabilities().GL_EXT_framebuffer_blit) {
int srcX0 = 0;
int srcY0 = 0;

  •        int srcX1 = 0;
    
  •        int srcY1 = 0;
    
  •        int srcX1;
    
  •        int srcY1;
    
           int dstX0 = 0;
           int dstY0 = 0;
    
  •        int dstX1 = 0;
    
  •        int dstY1 = 0;
    
  •        int dstX1;
    
  •        int dstY1;
    
           int prevFBO = context.boundFBO;
    

@@ -1774,6 +1773,7 @@
case ThreeDimensional:
case CubeMap: // cubemaps use 3D coords
glTexParameteri(target, GL_TEXTURE_WRAP_R, convertWrapMode(tex.getWrap(WrapAxis.R)));

  •            break;
           case TwoDimensional:
           case TwoDimensionalArray:
               glTexParameteri(target, GL_TEXTURE_WRAP_T, convertWrapMode(tex.getWrap(WrapAxis.T)));
    

@@ -2406,7 +2406,7 @@
}

 private void renderMeshDefault(Mesh mesh, int lod, int count) {
  •    VertexBuffer indices = null;
    
  •    VertexBuffer indices;
    
       VertexBuffer interleavedData = mesh.getBuffer(Type.InterleavedData);
       if (interleavedData != null && interleavedData.isUpdateNeeded()) {
    

@@ -2424,7 +2424,7 @@

// for (Entry<VertexBuffer> entry : buffers) {
// VertexBuffer vb = entry.getValue();

  •    for (VertexBuffer vb : mesh.getBufferList().getArray()) {
    
  •    for (VertexBuffer vb : buffersList) {//mesh.getBufferList().getArray()) {
           if (vb.getBufferType() == Type.InterleavedData
                   || vb.getUsage() == Usage.CpuOnly // ignore cpu-only buffers
                   || vb.getBufferType() == Type.Index) {
    

[/java]

Regarding this:

  • for (VertexBuffer vb : mesh.getBufferList().getArray()) {
  • for (VertexBuffer vb : buffersList)

I haven’t tracked through the code because I don’t have time at the moment but the SafeArrayList will only be created once and from then you get Iterator free iteration. Whereas the second way will create a new Iterator (internally) every time that for-loop is run.

The array-based for-loop will never create an Iterator. It’s one of the points of SafeArrayList.

The thing that annoys me is the buffersList is created but not used. I imagine it was there for the loop, but either it was put on hold for X/Y/Z reason or whoever put it there forgot to add it to the loop. I think the latter happened. My point is, if it’s not used it should be commented, not created and left there, unused. :slight_smile:

Yeah, could be that buffersList should go away… I don’t have the code open at the moment so I can’t check.

cough

Should I commit this? Have you had some time to check this out Paul?

IMO you should keep the getArray stuff and remove this buffersList.
Other than that the patch looks good

Committed

@madjack said: The first point is a missing break;

Looking at this again… I’m starting to wonder if the missing break was intentional like the missing break between two dimensional and one dimensional. I have no idea, though.

Maybe @momoko_fan can confirm one way or the other.

Edit: though I guess if you were having problems before and this fixed them… then maybe there is something else or this fix is good.

@pspeed said: Edit: though I guess if you were having problems before and this fixed them... then maybe there is something else or this fix is good.

To be honest I haven’t seen any difference.

I noticed the missing break when I investigated why the cubemap wasn’t playing nice with other objects on the Sky bucket. I found out later that the order the sky was added was making the problem. The sky will “overlap” everything else that is rendered before it. To make it work you have to make sure it’s the first in the sky bucket.

Since the skybox is fully opaque you won’t see the rest on that bucket. I found that out by making parts of the textures I used to make the cubemap transparent. It’s at that point I saw the other geometries show through those areas.

I can revert the changes if needed.

@madjack said: To be honest I haven't seen any difference.

I noticed the missing break when I investigated why the cubemap wasn’t playing nice with other objects on the Sky bucket. I found out later that the order the sky was added was making the problem. The sky will “overlap” everything else that is rendered before it. To make it work you have to make sure it’s the first in the sky bucket.

Since the skybox is fully opaque you won’t see the rest on that bucket. I found that out by making parts of the textures I used to make the cubemap transparent. It’s at that point I saw the other geometries show through those areas.

I can revert the changes if needed.

Yeah, Skybox is funny that way. Also depth is completely flattened which can do interesting things if you ever put more complex models in there for some reason.

Anyway, the original switch/case looked conceptually like:

[java]
switch() {
case 3D:
do R stuff
case 2D:
do T stuff
case 1D:
do S stuff
break;
}
[/java]

So I suspect the missing break was intentional and only missed a comment. Sorry I didn’t notice it sooner.

So you want me to revert or you prefer to wait for Kirill’s definite answer?

@madjack said: So you want me to revert or you prefer to wait for Kirill's definite answer?

Heheh… that would require that I make a decision. :slight_smile:

I think revert it if it isn’t causing you any issues to do so. I strongly suspect the old code was right and just missing a comment.

Reverted.

@madjack said: Reverted.

Thanks. Sorry I didn’t spot it before the commit churn.

Don’t worry. Still got rid of the extra cycles spent making the SafeArrayList. That was worth it. Oh and my IDE warning OCD was also pleased. :wink:

@madjack said: Don't worry. Still got rid of the extra cycles spent making the SafeArrayList. That was worth it. Oh and my IDE warning OCD was also pleased. ;)

I think it was just grabbing an extra reference… I don’t think it was getting created twice. Two calls to mesh.getBufferList() instead of one. That tweaks my OCD something hard but it shouldn’t have affected GC or anything.

The point of the switch statement there is to setup wrap modes for each texture coordinate (R, S, T), 3D / Cube textures use all 3, 2D textures use only 2, and 1D textures use only 1.

Putting a break after the 3D / Cubemap would introduce a subtle bug with repeating 3D textures not actually repeating. Maybe several years later someone would use repeating 3D textures and then wonder why its not working, and never know…

In such case another " // fall down here is intentional…" would be in place. Just to avoid situation that somebody tries to ‘fix’ it again in few years :wink:

@Momoko_Fan said: The point of the switch statement there is to setup wrap modes for each texture coordinate (R, S, T), 3D / Cube textures use all 3, 2D textures use only 2, and 1D textures use only 1.

Putting a break after the 3D / Cubemap would introduce a subtle bug with repeating 3D textures not actually repeating. Maybe several years later someone would use repeating 3D textures and then wonder why its not working, and never know…

@abies said: In such case another " // fall down here is intentional.." would be in place. Just to avoid situation that somebody tries to 'fix' it again in few years ;)

Yep, thanks guys. I think that’s the conclusion we reached further up as well… though madjack just reverted the change and didn’t add the extra comment. Now that suspicions are confirmed a comment would be a good thing… I can try to remember to do it when I get a chance.

I just committed the comment