Blocktastic optimisation

Question

I have written the following code as the level rendering part of a game engine I am writing. I am very happy how it works but as it is the background / level graphics made up of individual blocks it needs to be quick.

What it does it look take the graphics from a resource loaded elsewhere and scales it onto the block if it is in the viewing window.

Blocksize is the size of the blocks and can be increased of decreased and is used to scale the blocks to give the Zoom in and out effect.

How can I improve this code both in terms of speed and shorten it to improve readability?

public void render(Graphics g){

    //Ensure player co-ords updated
    camerax=playerx-(gamewidth/2);
    cameray=playery-(gameheight/2);


    int blockwidth=blocksize-2;
    //Draw coloured blocks
    int screenx=-(int)camerax-blocksize;
    for (int x=0;x<sizex;x++){
          screenx+=blocksize;
          if (screenx>-blocksize && screenx<gamewidth){
              int screeny=-(int)cameray-blocksize;   
              for (int y=0;y<sizey;y++){
              screeny+=blocksize;    
              if (screeny>-blocksize && screeny<gameheight){              
                if (tiles[x][y][0]>0){
                    g.drawImage(levelgraphics, screenx,screeny, screenx+blockwidth,screeny+blockwidth, graphicsize,0,graphicsize*2,graphicsize, null);
                } else {
                    g.setColor(new Color( tiles[x][y][1]));
                    g.fillRect(screenx,screeny,blockwidth,blockwidth);   

                    g.setColor(Color.WHITE);
                    g.drawString(String.valueOf(x+y*sizex), screenx+5,screeny+20);
                }
              }
            }
        }
     }           

    //world - camera = screen, algebra that world = screen + camera
    //Add camerax to mouse screen co-ords to convert to world co-ords.    
    int cursorx_world=(int)camerax+(int)GameInput.mousex;
    int cursorx_grid=(int)cursorx_world/blocksize;  // World Co-ords / gridsize give grid co-ords
    int cursorx_screen=-(int)camerax+(cursorx_grid*blocksize);

    //Add cameray to mouse screen co-ords to convert to world co-ords.         
    int cursory_world=(int)cameray+(int)GameInput.mousey;
    int cursory_grid=(int)cursory_world/blocksize;
    int cursory_screen=-(int)cameray+(cursory_grid*blocksize);  


    //Draw on screen cursor
   if (cursorx_grid>=0 && cursory_grid>=0 && cursorx_grid<sizex && cursory_grid<sizey){
            //if (detect(cursorx_grid,cursory_grid)){
            //if (tiles[cursorx_grid][cursory_grid][0]>1){
                g.setColor(Color.yellow);
                g.drawRect(cursorx_screen,cursory_screen,blockwidth,blockwidth);       
            //}
    }
    //Draw on screen map
    for (int x=0;x<sizex;x++){
        for (int y=0;y<sizey;y++){
            g.setColor(new Color((int) tiles[x][y][1]));
            g.fillRect(mapx+x*mapsize, mapy+y*mapsize, mapsize, mapsize);
        }
    } 

   //Outline
            g.drawRect(mapx,mapy,mapsize*sizex ,mapsize*sizey );

   //Map location box
            g.setColor(Color.RED);
            g.drawRect(mapx+mapsize*((int)camerax/blocksize),mapy+mapsize*((int)cameray/blocksize),(gamewidth/blocksize)*mapsize ,(gameheight/blocksize)*mapsize );
}

The last bit of the code adds a map onto the screen and pops a scaled box on it so players can see what part of the map they are looking at.

Answer 1

White spaces are free, use them

Your code is hard to read as all operators are cramped together.

Avoid branches

A missed branch prediction can force your CPU to discard all speculative execution it has done. Although modern CPU's are pretty good at branch prediction, it's a good practice to avoid branches when possible. And in your case it will reduce iterations in the loop.

int screenx = -(int)camerax - blocksize;
for (int x = 0; x < sizex; x++){
    screenx += blocksize;
    if (screenx > -blocksize && screenx < gamewidth){

should be:

int screenx = -(int)camerax - blocksize;
int xstart = Math.max(0, -(blocksize + screenx) / blocksize);
int xend = Math.min(sizex, (gamewidth - screenx) / blocksize);
screenx += blocksize * xstart; // Edit: We need to keep this value updated too.
for (int x = xstart; x < xend; ++x){
    screenx += blocksize;

and similarly for the inner loop. Depending on the ratio camera size to level size (if I'm interpreting your code correctly) this can give a big speedup.

Hint: The y direction iteration start and end values are independent of the current iteration in x direction so calculate the yend and ystart before entering the first loop.

Caution: The above xstart and xend may be off-by-one as I don't know what all the variables are, adjust to taste.

Edit: Oops just realized this is what Neil mentioned in the first point on his answer. But I'll leave this here as it's an example of what he said.

Edit2: Missed the incrementation of screenx prior to entering the loop.

Answer 2

Three things I noticed:

1. Generally speaking, when you have a for with a gigantic if composing the contents of the for, it is because you're checking for a condition in order to do something. If this condition is related to what you're iterating on, then you can modify the for loop to guarantee that the condition is true.

   Without knowing precisely how your code works, I don't feel comfortable trying to fix it myself, however if done properly, you shouldn't need to contain that gigantic inner if.

2. You're cycling the entire matrix twice. This is inefficient and shouldn't be done unless you require information that can only be obtained by having passed all data at least once (for example matrix value sum). If you're worried this will clutter your code further, then see the point below.

3. Separate each drawing task into its own method. This separation of tasks makes it easier to maintain and read, as you don't need to be looking at code for drawing the map location box if you're trying to fix a problem related to drawing the coloured blocks.

   In the case of a nested loop such as when you cycle through all matrix items, you should place the entire loop inside its own method, and for each draw operation to be performed for each block, call its own method passing whatever information you'd need.

I hope that helps.