I've done a little work, and I've got some trouble with the last pointer (after the blitsurface), this is the code I've got at the moment:
void* Sprite::load_sprite(SDL_Surface *sprite_image, int startx, int starty, int width, int height, int *arrayptr, int elementnumber) {
//blit clip uses the 4 integers over
SDL_Rect clip;//where to clip
SDL_Surface* loaded_image=NULL;//must load as a NULL for init
SDL_Surface* optimized_image=NULL;
SDL_Surface* finishedimage=NULL;
*loaded_image=*sprite_image;//the real init in this program
optimized_image=SDL_DisplayFormat(loaded_image);
SDL_FreeSurface(loaded_image);
clip.x=startx;
clip.y=starty;
clip.w=width;
clip.h=height;
SDL_BlitSurface(sprite_image, &clip, finishedimage,NULL);
*(arrayptr+elementnumber)=finishedimage;//points to arrayelement number x, buggy
//new usage:
//load_sprite(sprite_image_loaded, 0,0,30,40,myarray[], 3)
}
Thanks again!
SDL animation array
It is still quite unclear what you expect the function to do. You said you want to load it from disk but you are passing an existing surface?
Break up the functionality. A function should do one thing - the thing it is named to do. This makes the code easier to read, debug and maintain. Your functions should certainly not be trying to set array references when this could be easily achieved by the calling function. Simplicity is the key to writing good, correct code.
Based on what I can glean from the somewhat inconsistent requirements, I'd write something like this:
The code in load_sprite is the most confused, because it is unclear what this array is or why I'm using it. I'm sure you can work with that though.
Note how each function meticulously checks for errors - I believe I handle them all gracefully (with the exception of NULL input parameters). Note that using stateless free functions is far easier to read and manage, because you only have to keep a small amount of information in your head at any one time.
Also note that these small functions can be reused easily, for instance if your requirement actually is to clip an in-memory surface then you can skip load_and_clip_image() and use clip_image() directly.
When I said a function should do only one thing, don't let that stop you from writing "helper" functions like load_and_clip_image() if it keeps the rest of the code easier to read. All the helper functions could be in a separate "utility" or "sdl_utility" source file.
Break up the functionality. A function should do one thing - the thing it is named to do. This makes the code easier to read, debug and maintain. Your functions should certainly not be trying to set array references when this could be easily achieved by the calling function. Simplicity is the key to writing good, correct code.
Based on what I can glean from the somewhat inconsistent requirements, I'd write something like this:
SDL_Surface *load_sprite(const char *image){ return IMG_Load(image);}// SDL_Surface *create_surface(SDL_PixelFormat *format, int w, int h){ SDL_Surface *surface = SDL_CreateRGBSurface(SDL_SWSURFACE, w, h, format->BitsPerPixel, format->Rmask, format->Gmask, format->Amask); return surface;}SDL_Surface *clip_image(SDL_Surface *surface, int x, int y, int w, int h){ // Optimise for screen blits (if the screen exists) SDL_Surface *screen = SDL_GetVideoSurface(); // Try not to crash without a screen... SDL_PixelFormat *format = ((screen == NULL) ? surface : screen)->format; SDL_Surface *clipped = create_surface(format, x, y, w, h); SDL_Rect clip = { w, y, w, h }; if(SDL_BlitSurface(loaded, &clip, clipped, NULL) == 0) { return clipped; } SDL_FreeSurface(clipped); return 0;}void load_and_clip_image(const char *image, int x, int y, int w, int h){ SDL_Surface *surface = load_image(image); if(surface == 0) { return 0; } SDL_Surface *clipped = clip_image(surface, x, y, w, h); SDL_FreeSurface(surface); return clipped;}void Spite::load_sprite(const char *image, int x, int y, int w, int h){ SDL_Surface *surface = load_and_clip_image(image, x, y, w, h); if(surface == NULL) { // Use IMG_GetError() and/or SDL_GetError() to discover cause // Possibly throw an exception or generate a default image to use instead } else { sprite_image = surface; someArray[someIndex] = surface; }}The code in load_sprite is the most confused, because it is unclear what this array is or why I'm using it. I'm sure you can work with that though.
Note how each function meticulously checks for errors - I believe I handle them all gracefully (with the exception of NULL input parameters). Note that using stateless free functions is far easier to read and manage, because you only have to keep a small amount of information in your head at any one time.
Also note that these small functions can be reused easily, for instance if your requirement actually is to clip an in-memory surface then you can skip load_and_clip_image() and use clip_image() directly.
When I said a function should do only one thing, don't let that stop you from writing "helper" functions like load_and_clip_image() if it keeps the rest of the code easier to read. All the helper functions could be in a separate "utility" or "sdl_utility" source file.
Author
Last compile bug was that the void* should not be a void pointer, just a void. I do believe that the function is quite clear, but perhaps it could've been better commented. It loads, clips and saves the sprite, and returns it to the sprite array. Thanks for everyones help!
I'll let you know if i find any bugs!
And a copy of the finished function (not tested):
void Sprite::load_sprite(SDL_Surface *sprite_image, int startx, int starty, int width, int height, SDL_Surface* arrayptr, int elementnumber) {
//blit clip uses the 4 integers over
SDL_Rect clip;//where to clip
SDL_Surface* loaded_image=NULL;//must load as a NULL for init
SDL_Surface* optimized_image=NULL;
SDL_Surface* finishedimage=NULL;
*loaded_image=*sprite_image;//the real init in this program
optimized_image=SDL_DisplayFormat(loaded_image);
SDL_FreeSurface(loaded_image);
clip.x=startx;
clip.y=starty;
clip.w=width;
clip.h=height;
SDL_BlitSurface(sprite_image, &clip, finishedimage,NULL);
arrayptr=finishedimage;//points to arrayelement number x
}
I'll let you know if i find any bugs!
And a copy of the finished function (not tested):
void Sprite::load_sprite(SDL_Surface *sprite_image, int startx, int starty, int width, int height, SDL_Surface* arrayptr, int elementnumber) {
//blit clip uses the 4 integers over
SDL_Rect clip;//where to clip
SDL_Surface* loaded_image=NULL;//must load as a NULL for init
SDL_Surface* optimized_image=NULL;
SDL_Surface* finishedimage=NULL;
*loaded_image=*sprite_image;//the real init in this program
optimized_image=SDL_DisplayFormat(loaded_image);
SDL_FreeSurface(loaded_image);
clip.x=startx;
clip.y=starty;
clip.w=width;
clip.h=height;
SDL_BlitSurface(sprite_image, &clip, finishedimage,NULL);
arrayptr=finishedimage;//points to arrayelement number x
}
If you're not going to read the responses then I will probably stop bothering to write them.
Your function has plenty of bugs. The line *loaded_image=*sprite_image; is certainly wrong, it dereferences NULL to start with, and you should never assign SDL_Surfaces like that unless you are 110% sure of what you are doing.
Your function has plenty of bugs. The line *loaded_image=*sprite_image; is certainly wrong, it dereferences NULL to start with, and you should never assign SDL_Surfaces like that unless you are 110% sure of what you are doing.
like rip-off says there are many issues with how you are using pointer in your function
first off like rip-off says you can't do this
loaded_image is NULL and you can't dereferences NULL
you can do this loaded_image=sprite_image
however it is pointless if sprite_image is NULL
and if want this function to work like this "array_containing_animations[number]=load_sprite(spritesheet.gif, 0,0,30,50);"
you need to have a load function call in this function. SDL_LoadBMP or IMG_Load.
and the function needs to return a SDL_Surfaces *
my clipping function (which could use a little clean up) in my engine is the function void Animation::SetClipingBoxForFrame()
and the loading a image from file is bool image::LoadFileSurface()
first off like rip-off says you can't do this
SDL_Surface* loaded_image=NULL;//must load as a NULL for init SDL_Surface* optimized_image=NULL; *loaded_image=*sprite_image;//the real init in this programloaded_image is NULL and you can't dereferences NULL
you can do this loaded_image=sprite_image
however it is pointless if sprite_image is NULL
and if want this function to work like this "array_containing_animations[number]=load_sprite(spritesheet.gif, 0,0,30,50);"
you need to have a load function call in this function. SDL_LoadBMP or IMG_Load.
and the function needs to return a SDL_Surfaces *
my clipping function (which could use a little clean up) in my engine is the function void Animation::SetClipingBoxForFrame()
and the loading a image from file is bool image::LoadFileSurface()
Author
Quote:Original post by rip-off
If you're not going to read the responses then I will probably stop bothering to write them.
Your function has plenty of bugs. The line *loaded_image=*sprite_image; is certainly wrong, it dereferences NULL to start with, and you should never assign SDL_Surfaces like that unless you are 110% sure of what you are doing.
Rip-off: I'm sorry if you feel like that (and I do read them, even though it might not always look like it). I'm aware of the fact that there could (or as you experts say, is) alot of bugs. And reusability is not something I'm to concerned about since this is a super-quick school-project in the middle of a few exams.
Blackcloak: Is a 122kb cpp file plus a 40kb header file a bit to much to have in one file?
To my defense: Last compile bug.....
Btw, using "SDL_Event& event;" and "event.key.keysym.sym;" a bit later in some other part of the code causes almost 500 warnings.
Quote:Blackcloak: Is a 122kb cpp file plus a 40kb header file a bit to much to have in one file?
Definitely it is on my list of todos to break them into separate files and provided static libs that you can link against. Original I had them in two file for easy distribution since I have not had the time to make the static libs.
Quote:Btw, using "SDL_Event& event;" and "event.key.keysym.sym;" a bit later in some other part of the code causes almost 500 warnings.
If this is referring to my engine code, good to know will into it. thanks
BTW there are a list of Know Bugs
Author
Quote:Quote:Btw, using "SDL_Event& event;" and "event.key.keysym.sym;" a bit later in some other part of the code causes almost 500 warnings.
If this is referring to my engine code, good to know will into it. thanks
I'm refering to my own code, but if you get some ~250 errors at the same line, that's the reason.... I haven't tested your engine, but I've had a quick look at it but the length gives me a headache....
Biggest file I've ever written is ~25 kb, but it's still damn large....
Up to 10kb seems like a good rule of thumb to me, but that's not my business. Time to go bughunting tomorrow.... As rip-off said, there is probably alot of bugs but it do compile so that is one milestone.
This topic is closed to new replies.
Advertisement
Popular Topics
Advertisement
