2
\$\begingroup\$

I am working on a console renderer and I want a heap-allocated dynamic texture class. Since I am quite new to C++, I just want to check the memory management, if I destroy all pointers correctly etc.

class Texture {
private :

char** pixels; // heap allocated
int width, height;

public :

Texture(int width, int height, const char* texture) {
    this->width = width;
    this->height = height;

    pixels = new char*[width];
    for (int x = 0; x < width; x++) {
        pixels[x] = new char[height];

        for (int y = 0; y < height; y++)
            pixels[x][y] = texture[y*width+x];  

    }

}

Texture(int width, int height, char fill) {

    this->width = width;
    this->height = height;

    pixels = new char*[width];
    for (int x = 0; x < width; x++) {
        pixels[x] = new char[height];

        for (int y = 0; y < height; y++)
            pixels[x][y] = fill;    

    }

}

~Texture() {
    for (int x = 0; x < width; x++)
        delete[] pixels[x];

    delete pixels;
}

Texture getSubTexture(int x, int y, int width, int height) {

    Texture newtex(width, height, '?');

    for (int xx = 0; xx < width; xx++)
        for (int yy = 0; yy < height; yy++) {
            newtex.setPixel(xx, yy, getPixel(x+xx, y+yy));
        }

    return newtex;

}

void setPixel(int x, int y, char c) {
    pixels[x][y] = c;
}

char getPixel(int x, int y) {
    return pixels[x][y];
}

int getWidth() { return width; }
int getHeight() { return height; }

};
\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

getHeight and getWidth should be const.

int getWidth() const { return width; }
int getHeight() const { return height; }

Your destructor does not properly delete the pixels pointer. Since you allocate it with new[] you need to use delete[].

delete [] pixels;

You're storing your textures in column-major order. Depending on how you access them, this can cause performance issues with caching. For example, in the constructor, when you copy in the initial texture values you jump thru memory (one byte every width bytes) rather than reading it sequentially (where x would be the inner loop).

Then there's the inevitable question of why you're using manual memory management, rather than using vector.

\$\endgroup\$
2
  • \$\begingroup\$ I thought vector is more memory demanding and this is a more lightweight approach. \$\endgroup\$
    – Jacob
    Commented Jul 6, 2019 at 17:45
  • 1
    \$\begingroup\$ @Jacob It might use a few more bytes of memory to keep track of the size of the vector and how big of a buffer is allocated, but it is easier, safer, and more concise to use. With a proper vector set up, your "fill" constructor body can be reduced to three "lines". You'd then be able to use the default destructor. And I didn't even mention the copy and move constructors and assignment operators. \$\endgroup\$ Commented Jul 6, 2019 at 17:48

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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