Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

This code is for a simple 2D tile-based game. x1 and y1 are the mouse coordinates in the world. entity->x1 and entity->y1 is the point where the player is, the shot origin.

I would like to know how to keep the current output while simplifying the code and improving performance.

void shoot(Map *map, Entity *entity, int x1, int y1)
{
    float slope = (y1 - entity->y1) / (x1 - entity->x1);
    float y = entity->y1; //Weapon position
    Block *block;

    if(x1 > entity->x1){
        y += slope;
        for(float x = entity->x1 + 1; x < map->width && y < map->height && y > 0; ++x){

            //Get block at position x y. Check if found a hit
            if((block = map_block_at(map, x, y))->type != EMPTY){
                block_cause_damage(block, entity->hand_item->weapon.damage);
                return;
            }

            y += slope;
        }
    }

    else
    if(x1 < entity->x1){
        y -= slope;
        for(float x = entity->x1 - 1; x > 0 && y < map->height && y > 0; --x){

            //Get block at position x y. Check if found a hit
            if((block = map_block_at(map, x, y))->type != EMPTY){
                block_cause_damage(block, entity->hand_item->weapon.damage);
                return;
            }

            y -= slope;
        }
    }

    //When the player shoots up or down 
    else {
        slope = (y1 > entity->y1) ? 1 : - 1;
        y += slope;
        while(y < map->height && y > 0){

            //Get block at position x y. Check if found a hit
            if((block = map_block_at(map, x1, y))->type != EMPTY){
                block_cause_damage(block, entity->hand_item->weapon.damage);
                return;
            }

            y += slope;
        }
    }
}

The code for Entity and Map:

typedef struct Entity {
    Renderable renderable;
    plist_id render_id;
    Point (*get_current_position)(struct Entity *, uint32_t);
    int health, attack_damage, running, jumping;
    float x0, y0, x1, y1;
    uint32_t t0, t1;
    enum {LEFT, RIGHT} side;
    Image **texture;
    Item *hand_item;
    Backpack backpack;
} Entity;

typedef struct {
    Renderable renderable;
    plist_id render_id;
    int width, height;
    Block *blocks;
    Camera *camera;
} Map;
share|improve this question

2 Answers 2

up vote 10 down vote accepted

It's a little difficult to do a good review because there are so many pieces missing, but I've guessed at a number of things and I believe I can help.

Specifically, it looks like your Map is a rectangular grid of Blocks and that all x and y coordinates are integers. If so, then your shot routine is really doing the equivalent of drawing a line from the entity location to the passed x1,y1 coordinates and one very efficient way to do that is to use Bresenham's line-drawing algorithm. Using a slight modification of that algorithm, since your shot routine seems to want to go until it either hits something or goes off the Map, we get a very efficient and very small routine:

void shoot(Map *map, Entity *entity, int x1, int y1)
{
    int sx = entity->x1 < x1 ? 1 : -1;
    int sy = entity->y1 < y1 ? 1 : -1;
    int dx = abs(x1 - entity->x1);
    int dy = abs(y1 - entity->y1);
    int err = (dx>dy ? dx : -dy)/2;
    int e2;

    x1 = entity->x1;
    y1 = entity->y1;
    Block *block;
    while (inbounds(map,x1,y1)) {
        e2 = err;
        if (e2 > -dx) {
            err -= dy;
            x1 += sx;
        }
        if (e2 < dy) {
            err += dx; 
            y1 += sy;
        }
        if((block = map_block_at(map, x1, y1))->type != EMPTY){
            block_cause_damage(block, entity->hand_item->weapon.damage);
            return;
        }
    }
}

I've assumed that you have or can easily write an inbounds routine that returns true if the passed coordinates are within the Map.

share|improve this answer
    
Thank you for answering. I tried the code you provided and it works fine most of the time. But sometimes it gets stuck in the loop. I added another variable get_out, it's set to 0 on every iteration and incremented using else statements right after if (e2 > -dx) and if (e2 < dy) , then I check if get_out == 2 and exit. –  2013Asker Apr 27 at 0:31

I have a few remarks:

  • You may have a problem with this line of code:

    float slope = (y1 - entity->y1) / (x1 - entity->x1);
    

    If entity is already at the longitude x1, this will cause a division by 0. Such a division is undefined behaviour. Anything can happen. A plane may crash on your house. You should check first whether x1 == entity->x1 and handle this case properly. I see that the last else close of your program handles this case (//When the player shoots up or down), but the division by 0 appears before you handle it.

  • I don't know whether entity.x and entity.y are int or float types. If they are int types, I guess that you can get rid of the float variables: the only thing that may need more precision than an integer is slope, and if entity.x and entity.y are integer types, then slope will contain an integer cast to a float (since it was initialized with an integer division).

It lacks a bit of context to provide a complete review. You should post the code of at least Map and Entity so that we don't have to assume things.

share|improve this answer
    
Thank you for answering. I'll post the code for entity and map. –  2013Asker Apr 27 at 0:32

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.