Printable View
You could look at some existing code in libraries that read BMP files. The source for SDL_LoadBMP() comes to mind. SDL_LoadBMP() is #defined as a call to SDL_LoadBMP_RW(). The source for it is available here: http://www.google.com/codesearch?hl=...DL_bmp.c#first
Of course, I'm not sure how portable the SDL BMP loading code is, though I should imagine it's pretty good.
This error is usually cause by passing characters arrays to template functions that take a reference. The problem is that if an array is passed by reference then the whole array is passed, not just a pointer. The solution is to either cast the array you are passing to it to a char pointer yourself, or to explicitly specify the template arguments in the function call.Quote:
Originally Posted by mike_g
This problem also applies to literal strings.
Thanks for all the help. I'll checkout wotsit.org, for other file format stuff. For now I have managed to get my image class to load and save 24bit, uncompressed, BMPs. I plan on adding features to load: TGAs, PNGs, and JPEGs. Though I have heard JPEGs are hard to do. Anyway, heres the code incase anyone finds a use for it. If you can spot anything that could be done better, let me know :)
Code:#include <cstring>
#include <cctype>
#include <fstream>
typedef unsigned char Uint8;
typedef unsigned short Uint16;
typedef unsigned int Uint32;
enum ImageExtensions { NO_EXT, EXT_INVALID, BMP, JPG, PNG, TGA };
enum ImageLoad { LOAD_IMAGE_SUCCESS, LOAD_IMAGE_INVALID, LOAD_IMAGE_UNSUPPORTED_BITDEPTH };
class Image
{
public:
Image(){};
~Image(){delete[] pixels;}
bool Load(char*);
bool Save(char*);
int GetWidth();
int GetHeight();
private:
int GetExt(char*);
int LoadBMP(char*);
int LoadJPG(char*);
int LoadPNG(char*);
int LoadTGA(char*);
int SaveBMP(const char*);
int BytesToInt(char*);
short BytesToShort(char*);
void IntToBytes(char*, Uint32);
void ShortToBytes(char*, Uint16);
int w, h;
Uint32 *pixels;
};
//------------------------------------------------------------------//
//********************* PUBLIC FUNCTIONS ***************************//
//------------------------------------------------------------------//
bool Image::Load(char* filename)
{
switch(GetExt(filename))
{
case NO_EXT:
return false;
case EXT_INVALID:
return false;
case BMP:
LoadBMP(filename);
break;
case JPG:
break;
case PNG:
break;
case TGA:
break;
}
return true;
}
bool Image::Save(char* filename)
{
SaveBMP(filename);
return 0;
}
int Image::GetWidth()
{
return w;
}
int Image::GetHeight()
{
return h;
}
//------------------------------------------------------------------//
//********************* PRIVATE FUNCTIONS **************************//
//------------------------------------------------------------------//
int Image::GetExt(char* filename)
{
//This function gets a file extension and checks if it is valid
char ext[10]="\0";
char *e_ptr = ext;
char *f_ptr = filename;
Uint16 count = 0;
Uint16 index = 0;
//Get index of last full stop
while(*f_ptr++)
{
count++;
if(*f_ptr == '.') index = count;
}
if(!index) return NO_EXT;
f_ptr = filename+index;
//Copy The extension converting to upper case
while(*e_ptr++ = toupper(*f_ptr++));
//Check against valid extensions
if(!strcmp(ext, ".BMP")) return BMP;
if(!strcmp(ext, ".JPG")) return JPG;
if(!strcmp(ext, ".JPEG")) return JPG;
if(!strcmp(ext, ".PNG")) return PNG;
if(!strcmp(ext, ".TGA")) return TGA;
if(!strcmp(ext, ".TARGA"))return TGA;
return EXT_INVALID;
}
int Image::LoadBMP(char* filename)
{
ifstream in;
in.open(filename, ios::binary);
if(! in.is_open()) return LOAD_IMAGE_INVALID;
char bytes[4];
in.seekg(18, ios::beg);
in.read(bytes, 4);
w=BytesToInt(bytes);
in.read(bytes, 4);
h=BytesToInt(bytes);
in.seekg(2, ios::cur);
in.read(bytes, 2);
short bpp=BytesToShort(bytes);
if(bpp != 24) return LOAD_IMAGE_UNSUPPORTED_BITDEPTH;
//Allocate space for image data
pixels = new Uint32[w*h];
Uint32 *px = pixels;
in.seekg(54, ios::beg);
for(int y=h; y>0; y--)
{
for(int x=0; x<w; x++)
{
in.read(bytes, 3);
*px=((Uint8)bytes[2]<<16)+((Uint8)bytes[1]<<8)+(Uint8)bytes[0];
px++;
}
}
in.close();
return true;
}
int Image::BytesToInt(char* i)
{
return ((Uint8)i[3]<<24)+((Uint8)i[2]<<16)+((Uint8)i[1]<<8)+(Uint8)i[0];
}
short Image::BytesToShort(char* i)
{
return ((Uint8)i[1]<<8)+(Uint8)i[0];
}
int Image::SaveBMP(const char* filename)
{
ofstream out;
out.open(filename, ios::binary);
if(! out.is_open()) return 1;
char bytes[4]={'B','M','B', 'M'};
//-------HEADER--------------//
out.write(bytes, 2); //Write bitmap ID
IntToBytes(bytes, w*h*3+54);
out.write(bytes, 4); //Uint32 File Size In Bytes
IntToBytes(bytes, 0);
out.write(bytes, 4); //Uint16 reserved 1 and 2
IntToBytes(bytes, 44);
out.write(bytes, 4); //Int Offset To Image Data
//------- FILE INFO --------//
IntToBytes(bytes, 40);
out.write(bytes, 4); //Uint32 Header Size In Bytes
IntToBytes(bytes, (Uint32)w);
out.write(bytes, 4); //Int Image Width
IntToBytes(bytes, (Uint32)h);
out.write(bytes, 4); //Int Image Height
ShortToBytes(bytes, 1);
out.write(bytes, 2); //Unsigned Short Planes = 1
ShortToBytes(bytes, 24);
out.write(bytes, 2); //Unsigend Short Bit Depth = 24
IntToBytes(bytes, 0);
out.write(bytes, 4); //Unsigned Int Compression = 0 (No compression)
IntToBytes(bytes, w*h*3);
out.write(bytes, 4); //Unsigned Int Image Size In Bytes
IntToBytes(bytes, 2835);
out.write(bytes, 4);
out.write(bytes, 4); //Int X_Res, Y_Rex (pixes per meter)
IntToBytes(bytes, 0);
out.write(bytes, 4); //Uint32 number of paletted colours =0
out.write(bytes, 4); //Uint32 Important colours
//------ IMAGE DATA --------//
Uint32 *px = pixels;
for(int y=h; y>0; y--)
{
for(int x=0; x<w; x++)
{
IntToBytes(bytes, *px);
out.write(bytes, 3);
px++;
}
}
out.close();
return 0;
}
void Image::IntToBytes(char* bytes, Uint32 num)
{
bytes[3] = num >> 24;
bytes[2] = (num >> 16) & 255;
bytes[1] = (num >> 8) & 255;
bytes[0] = num & 255;
}
void Image::ShortToBytes(char* bytes, Uint16 num)
{
bytes[1] = num >> 8;
bytes[0] = num & 255;
}
A few comments:
1. you use const inconsistantly. Surely ALL of your filenames are always const?
2. If you ever plan on extending your image to allow inheritance, you may want to make getwidth, getheight, load and save virtual member functions.
3. Why not pass in a reference to the stream that has been already opened and checked that it's a valid stream to your LoadFMT function, rather than duplicating the open and error checking multiple times.
4. I think BytesToInt, IntToBytes, etc are not supposed to be member functions - they have no reason to be part of the class, they don't do things to a class member, they don't (really) operate on something specific to the class. And I also think the numbers should be unsigned also on the return values.
5. Eventually, you'll probably want to modify the pixels within the image, so you will need methods to get one or sets of pixeles, along with something like getStride() that returns the number of bytes per row of pixels.
6.is the same asCode:char ext[10]="\0";
for the use that you have of that variable.Code:char ext[10]="";
7. I think you may want to use a void * for your pixels, as Uint32 * indicates that the pixels are 32-bits, which may not be the case. If it's a void pointer, it certainly forbids any direct use, so there's no temptation of trying to use the pixels directly in an incorrect manner. You can always use a local variable that is a copy of pixels in any code that accesses the pixels.
8.can be done withCode://Get index of last full stop
while(*f_ptr++)
{
count++;
if(*f_ptr == '.') index = count;
}
if(!index) return NO_EXT;
f_ptr = filename+index;
9. There is a "magic" number in the bitmap format that says "this is a BMP file" besides the extension (I can quite easily take a text file and rename it to .bmp, and your application would probably fall over in a heap). Check the first two bytes, it should be "BM".Code://Get index of last full stop
f_ptr = strrchr(filename, '.');
if(!f_ptr) return NO_EXT;
10. You may want some more sanity checking and error checking to see that the values you get are "not completely crazy". For example, there is a size of the image in bytes, and a dimension in pixels wide and high. Multiplied by bitdepth, those should match up.
--
Mats
Thanks for the feedback matsp. To some of the points you mentioned:
1. Yeah I'll change all instances of filenames passed into functions to constant char. Something I hadent got around to doing.
2. Err, I don't know what a virtual member function is, guess I should look it up. On the few occasions I used inheritance, I just changed all my private stuff to protected and it seemed to do what I wanted.
3. Sounds good.
4. Yeah, I dont know, It kind of keeps everything used as part of the class though. I'm not too fussed about that really.
5. I'll definitely be doing that.
7. I think I am just going to have it set at 32bit. Nothing uses lower bit depths nowadays anyway.
8. Cool, I dident know that.
9/10. Youre right I should really check that stuff first.
Anyway, I'll work on this some more tomorrow. Right now I'm off to bed. Nite.
Correct me if I'm wrong, but shouldn't the code use std::strcmp() etc since <cstring> rather than <string.h> is included, and there are no using directives?
I dont know. Would that be important? I added 'using namespace std' in other files included in the project, so thats why I might not notice anything going wrong here.
Hmm, no, maybe not. My compiler doesn't seem to mind this:
Code:#include <cstring>
int main() {
return strcmp("name", "NAME");
}
http://cboard.cprogramming.com/showthread.php?t=86711Quote:
Originally Posted by dwks
(I think you were the one who refreshed my memory by pointing out this thread to me a while back.)
Definitely try to learn how virtual works. It is an important part of object orientation, and it's definitely necessary for polymorphism - I think your class actually should make use of polymorphism in the sense that you probably should have one base-class for "Image", and then multiple inherited classes that implement for example BMP, JPEG, PNG, etc.Quote:
Originally Posted by mike_g
Well, that obviously depends on what you are trying to achieve, but one of the bases for class design is that a "class only contains what it needs". It is very easy to try to make "swiss knife classes", which incorporates too much to be meaningful.Quote:
4. Yeah, I dont know, It kind of keeps everything used as part of the class though. I'm not too fussed about that really.
In this case the functions I pointed out are not dealing with anything inside the class, they are just converting data from one format to another. This is typically best done with a static function in the class implementation file. It shouldn't be part of the class declaration tho'.
The rule should be "if it doesn't have to be in the class, don't put it in the class". Classes tend to grow too large eventually anyways, without having extra stuff added that don't need to be there.
And that applies for all image formats that you intend to support? What if you were to support 16-bit per pixel TIFF, or perhaps GIF that is (AFAIK) always 8BPP?Quote:
7. I think I am just going to have it set at 32bit. Nothing uses lower bit depths nowadays anyway.
There are also plenty of BMP's that are 1BPP because they are black and white or 8BPP grayscale - if it's not in colour, it's no point in spending 4 times the data-size to store it, is there?
I'm not trying to clank down on your code - I'm just trying to get it right before you've written too much of it and then have to make massive changes.
--
Mats
Yeah I'll check out virtual functions and maybe try and break my class down a bit as it would end up pretty big. If you haven't noticed yet I still kind of suck at OOP. I guess taking out the non-essential functions wouldn't hurt either.
As for supporting lower bitdepths, I'm still not bothered about it. Bitmasks are useful but I would probably make a separate class for them. PNG is better than GIF in nearly every respect, so I'm not planning GIF support. As for greyscale; I would be wasting space, but hey RAM is dirt cheap nowadays :)
That's not a good design. The serialization format is a property of the image loader/writer, not of the image itself. Once you've loaded an image, whatever format it was is irrelevant.Quote:
one base-class for "Image", and then multiple inherited classes that implement for example BMP, JPEG, PNG, etc.
Not so cheap that you can afford to waste 75% of the space an image needs.Quote:
RAM is dirt cheap nowadays
Yes, I agree. Perhaps what I described wasn't quite the right way to go about things, but I still think the class should be separated into a common part, and a part that understands a particular image format (e.g. bmp, jpeg, etc).Quote:
Originally Posted by CornedBee
--
Mats
Ok, maybe think about it this way: How many greyscale images is anyone ever likely to have simultaneously loaded in RAM? Probably not many. The only sort of thing like that I can thing of would be bump map data, which fits snuggly in the high 8 bits of an image.Quote:
Not so cheap that you can afford to waste 75% of the space an image needs.
Bump maps, displacement maps (OK, so probably not both of these), height maps, light maps ...