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.

I have an axis-aligned bounding box (AABB) that I've coded, but at the moment it is very redundant. I feel like there are variables that I can get rid of. I need the AABB to be very fast, and I'd like it to be more lightweight than it is.

#pragma once

#include "Engine.h"
// Collision
#include "Vec2D.h"
// End Collision

#include "Renderer.h"

struct ENGINE_API AABB {
private:
    Vec2D min;
    Vec2D max;

    double x;
    double y;
    double width;
    double height;
    double xwidth; // x + width
    double yheight; // y + height

    double vertMid; // vertical mid, cached for quadtree
    double horzMid; // horizontal mid, cached for quadtree

    // Normalize
    void calcMid() {
        vertMid = x + (width / 2);
        horzMid = y + (height / 2);
        xwidth = x + width;
        yheight = y + height;
    }

    void init(Vec2D, Vec2D);
    // End Normalize
public:
    // Constructor
    AABB(Vec2D min, Vec2D max) {
        init(min, max);
    }

    AABB(double x, double y, double width, double height){
        init(Vec2D(x, y), Vec2D(x + width, y + height));
    }
    // End Constructor

    // Getters
    double getX()const {
        return x;
    }

    double getY()const {
        return y;
    }

    double getWidth()const {
        return width;
    }

    double getHeight()const {
        return height;
    }

    double getHorzMid()const {
        return horzMid;
    }

    double getVertMid()const {
        return vertMid;
    }

    double getXWidth()const {
        return xwidth;
    }

    double getYHeight()const {
        return yheight;
    }
    // End Getters

    // Base
    bool collides(AABB other) {
        if (xwidth < other.x || x > other.xwidth) { return false; }
        if (yheight < other.y || y > other.yheight) { return false; }

        return true;
    }

    bool contains(Vec2D point) {
        if (x > point.getX() || xwidth < point.getX()) { return false; }
        if (y > point.getY() || yheight < point.getY()) { return false; }

        return true;
    }

    void move(double x, double y) {
        this->x = x;
        this->y = y;
        calcMid();
    }
    // End Base
    // Testing
    void render(Renderer& renderer) {
        sf::RectangleShape shape;
        shape.setPosition(x, y);
        shape.setOutlineColor(sf::Color::Red);
        shape.setOutlineThickness(3);
        shape.setSize(sf::Vector2f(width, height));
        renderer.render(shape);
    }
    // End Testing
};

And the messy part:

void AABB::init(Vec2D one, Vec2D two) {
    double x1 = one.getX();
    double y1 = one.getY();
    double xx = two.getX();
    double yy = two.getY();

    if (x1 > xx) {
        if (y1 > yy) {
            min = Vec2D(xx, yy);
            max = Vec2D(x1, y1);
        }
        else {
            min = Vec2D(xx, y1);
            max = Vec2D(x1, yy);
        }
    }
    else {
        if (y1 > yy) {
            min = Vec2D(x1, yy);
            max = Vec2D(xx, y1);
        }
        else {
            min = Vec2D(x1, y1);
            max = Vec2D(xx, yy);
        }
    }

    x = min.getX();
    y = min.getY();
    width = max.getX() - x;
    height = max.getY() - y;

    calcMid();
}
share|improve this question
    
Please add the definition of your init method. –  ChrisW Mar 25 at 17:08
    
Sorry @ChrisW, added –  Tips48 Mar 25 at 20:49
    
Thanks. I updated my answer slightly after you accepted it. –  ChrisW Mar 25 at 21:03

2 Answers 2

up vote 4 down vote accepted

In a comment in your other question you claimed,

Thank you, Caching vertMid and horzMid really helped.

Whether caching is helpful depends on:

  • How often you used the cached data
  • How tight your performance requirements are
  • Whether your cache implementation is reliable

One of your comments says,

// vertical mid, cached for quadtree

Only your testing can show whether that optimization is helpful.

From a performance point of view, in general:

  • If it's not used it's harmful to cache
  • If it's used once then it's harmless/useless to cache
  • If it's used several times then it's useful to cache

However making something bigger (by caching extra values) can also have some negative effect on performance, e.g. because you can't fit so many objects in the CPU cache (which is faster than memory): which is why you should profile/test to see whether caching makes it faster in practice as well as in theory.


If you want to eliminate duplicate variables, you only need 4 doubles:

  • min and max
  • or, x and either width or xwidth, and y and either height and yheight

I found your xwidth and yheight names confusing to read; I would have preferred 'right' and 'bottom'.


If you have large structs, you may improve performance by curing your habit of passing objects by value instead of by reference. For example, this ...

bool collides(AABB other)

... should be ...

bool collides(const AABB& other) const

You can also use different classes for different purposes. For example, the AABB bounds member of your Quadtree class needs getHorzMid() and getVertMid() members, but those methods aren't needed/used by the AABB which you return using Locatable::getAABB(). So you could use two classes, e.g. AABB (without getHorzMid and getVertMid members), and a SplittableAABB class (used as a member of Quadtree) which adds those members.


I don't much like AABB as a class name: partly because all-upper-case looks like a macro to me: consider Bounds or Rect or Rectangle instead.


Your current code creates but doesn't read the initialize the min and max data members.

Your move method doesn't update the data in these members (rendering that data untrue).

Maybe Vec2D min and Vec2D max should be (temporary) local variables of your init function, not (permanent) member data of the class.

The parameters passed to the AABB(Vec2D min, Vec2D max) constructor should probably be named one and two not min and max (and should possibly be passed by reference not by value).

share|improve this answer
    
I've decided to do a couple things: Vec2D x and y are now public, AABB no longer has x, y, width, height, There are now two methods getWidth() and getHeight(), and min and max are now public, and are used regularly. –  Tips48 Mar 25 at 21:17

adding and halving is cheap, don't bother optimizing it out unless it is a problem

all you need is min and max and derive the rest from that:

double getMinX()const {
    return min.getX();
}


double getWidth()const {
    return max.getX()-min.getX();
}


double getXMid()const {
    return (max.getX()+min.getX())/2;
}


double getMaxX()const {
    return max.getX();
}

bool contains(Vec2D point) {
    if (min.getX() > point.getX() || max.getX() < point.getX()) { return false; }
    if (min.getY() > point.getY() || max.getY() < point.getY()) { return false; }

    return true;
}

and so on

however given that this is just a POD you can make the fields public and not bother with the accessor methods

share|improve this answer
    
FWIW compiler can usually optimize the overhead of getter/setter call away, so I'd prefer having it encapsulated... –  Vogel612 Mar 25 at 17:19

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.