Jump to content
  • Advertisement
Sign in to follow this  
thedustbustr

image class design - c-style vs c++-style

This topic is 4163 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

Hows this design? It uses pointers and memcopy. Is there an elegant design using <vector> or something which allows me to still retain a byte* pointer to the raw tga block?
#ifndef __TGAImage_h
#define __TGAImage_h

typedef unsigned char byte;

class TGAImage
//this is not a finished tga class and will probably do very bad 
//things if some infrequently used fields are nonzero
{
public:
	#pragma pack(push)
	#pragma pack(1)
	struct Header
	{
		byte	idLength;			// size of ID field that follows 18 byte header (0 usually)
		byte	colorMapType;		// type of colour map 0=none, 1=has palette
		byte	imageType;			// type of image 0=none,1=indexed,2=rgb,3=grey,+8=rle packed
		short	colorMapOrigin;		// first colour map entry in palette
		short	colorMapLength;		// number of colours in palette
		byte	colorMapDepth;		// number of bits per palette entry 15,16,24,32
		short	imageOriginX;		// image x origin
		short	imageOriginY;		// image y origin
		short	imageWidth;			// image width in pixels
		short	imageHeight;		// image height in pixels
		byte	imagePixelDepth;	// image bits per pixel 8,16,24,32
		byte	imageDescriptor;	// image descriptor bits (vh flip bits)
	};
	#pragma pack(pop)

public:

	TGAImage(Header*, const byte*);
	const size_t GetNumRawBytes() const;
	const size_t GetNumPixels() const;
	const size_t GetNumPixelBytes() const;
	const size_t GetSize() const;

	const short GetWidth() const;
	const short GetHeight() const;
	const byte GetDepth() const;

	const Header* GetHeader() const;
	const byte* GetRaw() const;
	const byte* GetRawHeader() const;
	const byte* GetRawID() const;
	const byte* GetRawPixels() const;
	
	~TGAImage();
	
private:
	byte* data;
};


#endif //__TGAImage_h
#include "TGAImage.h"

#include <cstring>

TGAImage::TGAImage(Header* header, const byte* pixels)
{
	size_t numBytes = header->imageHeight*header->imageWidth*header->imagePixelDepth/8;
	data = new byte[sizeof(Header)+numBytes];

	std::memcpy(const_cast<byte*>(GetRawHeader()), header, sizeof(Header));
	std::memcpy(const_cast<byte*>(GetRawPixels()), pixels, numBytes);
}

TGAImage::~TGAImage()
{
	delete data;
	data=0;
}

const size_t TGAImage::GetNumRawBytes() const {return GetNumPixelBytes()+sizeof(Header);}
const size_t TGAImage::GetNumPixelBytes() const {return GetNumPixels()*GetDepth();}
const size_t TGAImage::GetNumPixels() const {return GetWidth()*GetHeight();}
const size_t TGAImage::GetSize() const {return GetNumRawBytes();}

const short TGAImage::GetWidth() const {return GetHeader()->imageWidth; }
const short TGAImage::GetHeight() const {return GetHeader()->imageHeight; }
const byte TGAImage::GetDepth() const {return GetHeader()->imagePixelDepth/8; }

const TGAImage::Header* TGAImage::GetHeader() const	{return reinterpret_cast<const Header*>(GetRawHeader());}
const byte* TGAImage::GetRaw() const		{return data;}
const byte* TGAImage::GetRawHeader() const	{return GetRaw()+0;}
const byte* TGAImage::GetRawID() const		{return GetRawHeader() + sizeof(Header);}
const byte* TGAImage::GetRawPixels() const	{return GetRawID()+GetHeader()->idLength;}

Share this post


Link to post
Share on other sites
Advertisement
std::vector<byte> is guaranteed to be contiguous storage, you can your raw pointers with it with a minimum of fuss and gain many safety benefits.

Share this post


Link to post
Share on other sites
Quote:
Original post by thedustbustr
soooo instead of byte*data i have vector<byte> data?


And when you need the pointer, do &data[0], yes

Share this post


Link to post
Share on other sites
Quote:
Original post by thedustbustr
soooo instead of byte*data i have vector<byte> data?


Correct.
Are you aware thats not the only difference though.

Before:

// Instantiation
byte* data;

// Allocation
data = new byte[sizeof(Header)+numBytes];

// Initialisation
std::memcpy(const_cast<byte*>(GetRawHeader()), header, sizeof(Header));
std::memcpy(const_cast<byte*>(GetRawPixels()), pixels, numBytes);

// A pointer
byte* raw = data;

// Destruction
delete[] data;







Notice how you've already made a mistake with the non-vector approach?
You should be using delete[] not delete
You're leaking memory!

After:

// Instantiation
std::vector<byte> data;

// Allocation
data.reserve(sizeof(Header)+numBytes);

// Initialisation
data.insert( data.end(), header, header + sizeof(Header) );
data.insert( data.end(), pixels, pixels + numBytes );

// A pointer
byte* raw = &data[0];

// Destruction
// no need the vector will die with the image class







Share this post


Link to post
Share on other sites
Quote:
Original post by Spoonbender
Quote:
Original post by thedustbustr
soooo instead of byte*data i have vector<byte> data?


And when you need the pointer, do &data[0], yes

Or, alternatively, &data.front() - which I find nice, but that's a matter of taste [smile].
And replace your memcpy by std::copy:
std::copy(pixels.begin(), pixels.end(), m_pixels.begin());
Instead of:
memcpy(m_pixels, pixels, numpixels);

You shall also know that the whole purpose of an image reader class is to encapsulate the reading of the image file. This is something you fail to do, since the reading of the file is supposed to take place outside - and then you call your constructor with a pointer to the header and a pointer to the pixel data. This is quite strange. It means that once you have read the file, you have to copy the data in the class, instead of directly read the data in the class. That doesn't make much sense IMHO.

Also: stop using these damn pointers. Instead of using pointers, you shall try to use references. Although you can still create a null reference, it's harder to do so, meaning that on the overall your code will be more secure.

HTH,

Share this post


Link to post
Share on other sites
Quote:

Although you can still create a null reference, it's harder to do so


Nope, not in standard C++. You can dereference NULL but god knows what will happen next, but whatever it is it will not be a "null reference", it will be 100% pure undefined goodness [smile].

Pedantic? Maybe. But this is for beginners. Don't want them thinking that a monstrosity such as a null reference is possible.

Share this post


Link to post
Share on other sites
Quote:
Original post by rip-off
Pedantic? Maybe. But this is for beginners. Don't want them thinking that a monstrosity such as a null reference is possible.


"Null references" interact very poorly with multiple inheritence and your future coworker's expectations, so it's hardly just pendantic. Unlike null pointers, GCC at least (last I checked) will actually skip the code to perserve nullness when making pointer offset adjustments when it comes to "null references". As in:

struct A {};
struct B {};
struct C {};
struct Many : A, B, C {};

int main() {
Many* m1 = 0;
A* a1 = m1;
B* b1 = m1;
C* c1 = m1;

assert( !m1 && !a1 && !b1 && !c1 ); // success -- all pointers will test as null

Many& m2 = *m1; // UNDEFINED BEHAVIOR
A& a2 = m2;
B& b2 = m2;
C& c2 = m2;

assert( !&m2 && !&a2 && !&b2 && !&c2 ); // typically FAILS -- probable details:
// &m2 -- likely will test as null
// &a2 -- being the first inherited class, this will likely share the same offset as &m2 and test as null as well
// &b2 -- likely NOT null -- normally, converting to B* would add a sizeof(A) offset, and the test to skip this offset add is skipped for references
// &c2 -- likely NOT null -- normally, converting to C* would add a sizeof(A)+sizeof(B) offset, and the test to skip this offset add is skipped for references
};




Of course, since Many& m2 = *m1; invokes undefined behavior, it's fine for your compiler to cause your program to launch nuclear missiles at a cow ranch in Alaska, so none of my "likely"s should be relied upon, as any of them could prove false (and I would expect my statements WRT &b2 and &c2 to quite possibly prove false on some compiler/platform combinations)

[Edited by - MaulingMonkey on July 24, 2007 4:01:09 PM]

Share this post


Link to post
Share on other sites
Quote:
Original post by MaulingMonkey
Many& m2 = *m1; // UNDEFINED BEHAVIOR

I cant test it but I do hope you get warning messages when trying such a thing?

Quote:
Original post by MaulingMonkey
it's fine for your compiler to cause your program to launch nuclear missiles at a cow ranch in Alaska

Got to love the Microsoft compiler aye.

Share this post


Link to post
Share on other sites
Quote:
Original post by dmatter
Quote:
Original post by MaulingMonkey
Many& m2 = *m1; // UNDEFINED BEHAVIOR

I cant test it but I do hope you get warning messages when trying such a thing?


I wouldn't know. I don't usually compile bad code ;-). But I doubt it, given that it depends on the value of m1. And Many& m2 = *(Many*)0; is just stupid enough as to be fairly obviously wrong -- once you know that's not legal defined-behavior C++ at any rate.

Share this post


Link to post
Share on other sites
Sign in to follow this  

  • Advertisement
×

Important Information

By using GameDev.net, you agree to our community Guidelines, Terms of Use, and Privacy Policy.

GameDev.net is your game development community. Create an account for your GameDev Portfolio and participate in the largest developer community in the games industry.

Sign me up!