Allegro.cc - Online Community

Allegro.cc Forums » Programming Questions » Allegro 5 crashes when using al_draw_bitmap inside a list container

This thread is locked; no one can reply to it. rss feed Print
Allegro 5 crashes when using al_draw_bitmap inside a list container
Xinomorph
Member #17,927
July 2020

Hi, I was trying to use the bitmap functions. But the program crashes when trying to use the "al_draw_bitmap" function. So I tried to reproduce the error, I created a simple program with a class, and in its constructor I used the lines to create the bitmap, and to load it:

#SelectExpand
1grass = al_create_bitmap(50, 50); 2grass = al_load_bitmap("D:/Documents/SimpleApp/bin/Debug/grass.bmp");

Another function of that class called "print()" has the drawing function:

al_draw_bitmap(grass, 100, 100, 0);

Then I created an instance of that class, and inside the main loop of the code, I used the print() function of my class. It worked fine, as expected.

But then, when instead of creating a single instance of that class, I create a list container with an element of that class, it doesn't work properly:

#SelectExpand
1std::list<THING> thing; 2std::list<THING>::iterator itt; 3thing.push_back(THING());

Then in the main loop of the code, I call the print() function this way:

#SelectExpand
1itt = thing.begin(); 2(*itt).print();

And then the program crashes. It doesn't crash when I create and load the bitmap in the constructor, it crashes when I try to use the "al_draw_bitmap" function. I'm on codeblocks, and when debugging, in the moment I step into the "al_draw_bitmap(grass, 100, 100, 0);" line, the debugger gives me this message:

Cannot open file: C:/dev/allegro_winpkg/universal/allegro/src/bitmap_draw.c

Any idea of why is this happening to me? Everything works fine when I'm not using the list container, it's when I use it that it crashes. I include the "#include <allegro5/allegro_image.h>" line, and also call for the "al_init_image_addon()" after "al_init()" so I have no idea what it could be. Maybe al_draw_bitmap() isn't supposed to work with a list containers? That'd be unfortunate because my main program is based on list and vector containers :'(

DanielH
Member #934
January 2001
avatar

Without more code, I can only offer suggestions.

Loading a bitmap already creates it. Your creating it twice without freeing the first creation.

// allocates memory, grass not points to that memory
grass = al_create_bitmap(50, 50);

// allocates memory, grass not points to that memory
// previous memory allocated now has nothing pointing to it and it wasn't freed
grass = al_load_bitmap("D:/Documents/SimpleApp/bin/Debug/grass.bmp");

also where is the verification that the bitmap was actually loaded?

At what point was the THING constructor called? Was it after display initialization?

Dizzy Egg
Member #10,824
March 2009
avatar

Shouldn't that be:

thing.push_back(new THING());

----------------------------------------------------
Please check out my songs:
https://soundcloud.com/dont-rob-the-machina

Xinomorph
Member #17,927
July 2020

DanielH said:

Loading a bitmap already creates it. Your creating it twice without freeing the first creation.

Thanks, I removed that line as it was unnecessary. The same problem still persists though.

DanielH said:

also where is the verification that the bitmap was actually loaded?

I added one, but I'm not sure if the way I did it is the correct way.

Now, I tried to put all the code of this test program in a single file so I can paste the entire program here. I tried to keep it as simple as I could. It behaves the same way I said above: without the list container, it works fine, but with the list container it crashes. First, this is the code without the list container:

#SelectExpand
1 2#include <allegro5/allegro.h> 3#include <allegro5/allegro_native_dialog.h> 4#include <allegro5/allegro_ttf.h> 5#include <allegro5/allegro_image.h> 6#include <list> 7 8ALLEGRO_DISPLAY *display = NULL; 9ALLEGRO_EVENT_QUEUE *event_queue = NULL; 10 11int initiate_allegro(); 12void destroy_pointers(); 13void exit_allegro(); 14 15void events(ALLEGRO_EVENT &event, bool &cycle, bool &doexit); 16 17class THING{ 18private: 19 ALLEGRO_BITMAP *grass; 20public: 21 THING(); 22 ~THING(); 23 void print(); 24}; 25 26THING::THING(){ 27 grass = NULL; 28 grass = al_load_bitmap("D:/Documents/TestBitmaps/bin/Debug/grass.bmp"); 29 if (grass == NULL){ 30 al_show_native_message_box(al_get_current_display(), "Allegro 5 Error", "Error", "Couldn't load bitmap.", NULL, ALLEGRO_MESSAGEBOX_ERROR); 31 } 32} 33 34THING::~THING(){ 35 if (grass != NULL) al_destroy_bitmap(grass); 36} 37 38void THING::print(){ 39 al_draw_bitmap(grass, 100, 100, 0); 40} 41 42 43int main(int argc, char **argv){ 44 bool doexit = false, cycle = true; 45 46 if (initiate_allegro() != 0){ 47 al_show_native_message_box(al_get_current_display(), "Allegro 5 Error", "Error", "An error has ocurred.", NULL, ALLEGRO_MESSAGEBOX_ERROR); 48 return -1; 49 } 50 51 THING thing; 52 53 while(doexit == false){ 54 ALLEGRO_EVENT event; 55 events(event, cycle, doexit); 56 57 if (cycle == true && al_is_event_queue_empty(event_queue) == true){ 58 cycle = false; 59 60 thing.print(); 61 62 al_flip_display(); 63 } 64 } 65 66 exit_allegro(); 67 return 0; 68} 69 70 71int initiate_allegro(){ 72 if (!al_init()){ 73 al_show_native_message_box(al_get_current_display(), "Allegro 5 Error", "Error", "Failed to initialize Allegro 5.", NULL, ALLEGRO_MESSAGEBOX_ERROR); 74 destroy_pointers(); 75 return -1; 76 } 77 78 al_init_ttf_addon(); 79 al_init_image_addon(); 80 81 display = al_create_display(1280, 720); 82 if (display == NULL){ 83 al_show_native_message_box(al_get_current_display(), "Allegro 5 Error", "Error", "Failed to create a display.", NULL, ALLEGRO_MESSAGEBOX_ERROR); 84 destroy_pointers(); 85 return -1; 86 } 87 88 if (!al_install_mouse()){ 89 al_show_native_message_box(al_get_current_display(), "Allegro 5 Error", "Error", "Failed to install mouse.", NULL, ALLEGRO_MESSAGEBOX_ERROR); 90 destroy_pointers(); 91 return -1; 92 } 93 94 if (!al_install_keyboard()){ 95 al_show_native_message_box(al_get_current_display(), "Allegro 5 Error", "Error", "Failed to install keyboard.", NULL, ALLEGRO_MESSAGEBOX_ERROR); 96 destroy_pointers(); 97 return -1; 98 } 99 100 event_queue = al_create_event_queue(); 101 if (event_queue == NULL){ 102 al_show_native_message_box(al_get_current_display(), "Allegro 5 Error", "Error", "Failed to create event queue.", NULL, ALLEGRO_MESSAGEBOX_ERROR); 103 destroy_pointers(); 104 return -1; 105 } 106 107 al_register_event_source(event_queue, al_get_display_event_source(display)); 108 al_register_event_source(event_queue, al_get_mouse_event_source()); 109 al_register_event_source(event_queue, al_get_keyboard_event_source()); 110 111 return 0; 112} 113 114void destroy_pointers(){ 115 if (display != NULL) al_destroy_display(display); 116 if (event_queue != NULL) al_destroy_event_queue(event_queue); 117} 118 119void exit_allegro(){ 120 destroy_pointers(); 121} 122 123 124void events(ALLEGRO_EVENT &event, bool &cycle, bool &doexit){ 125 al_wait_for_event(event_queue, &event); 126 127 if (event.type == ALLEGRO_EVENT_DISPLAY_CLOSE){ 128 doexit = true; 129 } 130}

Now, with the list container, instead of a single normal instance of the class, here it crashes:

#SelectExpand
1 2#include <allegro5/allegro.h> 3#include <allegro5/allegro_native_dialog.h> 4#include <allegro5/allegro_ttf.h> 5#include <allegro5/allegro_image.h> 6#include <list> 7 8ALLEGRO_DISPLAY *display = NULL; 9ALLEGRO_EVENT_QUEUE *event_queue = NULL; 10 11int initiate_allegro(); 12void destroy_pointers(); 13void exit_allegro(); 14 15void events(ALLEGRO_EVENT &event, bool &cycle, bool &doexit); 16 17class THING{ 18private: 19 ALLEGRO_BITMAP *grass; 20public: 21 THING(); 22 ~THING(); 23 void print(); 24}; 25 26THING::THING(){ 27 grass = NULL; 28 grass = al_load_bitmap("D:/Documents/TestBitmaps/bin/Debug/grass.bmp"); 29 if (grass == NULL){ 30 al_show_native_message_box(al_get_current_display(), "Allegro 5 Error", "Error", "Couldn't load bitmap.", NULL, ALLEGRO_MESSAGEBOX_ERROR); 31 } 32} 33 34THING::~THING(){ 35 if (grass != NULL) al_destroy_bitmap(grass); 36} 37 38void THING::print(){ 39 al_draw_bitmap(grass, 100, 100, 0); 40} 41 42 43int main(int argc, char **argv){ 44 bool doexit = false, cycle = true; 45 46 if (initiate_allegro() != 0){ 47 al_show_native_message_box(al_get_current_display(), "Allegro 5 Error", "Error", "An error has ocurred.", NULL, ALLEGRO_MESSAGEBOX_ERROR); 48 return -1; 49 } 50 51 std::list<THING> thing; 52 std::list<THING>::iterator itt; 53 thing.push_back(THING()); 54 55 while(doexit == false){ 56 ALLEGRO_EVENT event; 57 events(event, cycle, doexit); 58 59 if (cycle == true && al_is_event_queue_empty(event_queue) == true){ 60 cycle = false; 61 62 itt = thing.begin(); 63 (*itt).print(); 64 65 al_flip_display(); 66 } 67 } 68 69 exit_allegro(); 70 return 0; 71} 72 73 74int initiate_allegro(){ 75 if (!al_init()){ 76 al_show_native_message_box(al_get_current_display(), "Allegro 5 Error", "Error", "Failed to initialize Allegro 5.", NULL, ALLEGRO_MESSAGEBOX_ERROR); 77 destroy_pointers(); 78 return -1; 79 } 80 81 al_init_ttf_addon(); 82 al_init_image_addon(); 83 84 display = al_create_display(1280, 720); 85 if (display == NULL){ 86 al_show_native_message_box(al_get_current_display(), "Allegro 5 Error", "Error", "Failed to create a display.", NULL, ALLEGRO_MESSAGEBOX_ERROR); 87 destroy_pointers(); 88 return -1; 89 } 90 91 if (!al_install_mouse()){ 92 al_show_native_message_box(al_get_current_display(), "Allegro 5 Error", "Error", "Failed to install mouse.", NULL, ALLEGRO_MESSAGEBOX_ERROR); 93 destroy_pointers(); 94 return -1; 95 } 96 97 if (!al_install_keyboard()){ 98 al_show_native_message_box(al_get_current_display(), "Allegro 5 Error", "Error", "Failed to install keyboard.", NULL, ALLEGRO_MESSAGEBOX_ERROR); 99 destroy_pointers(); 100 return -1; 101 } 102 103 event_queue = al_create_event_queue(); 104 if (event_queue == NULL){ 105 al_show_native_message_box(al_get_current_display(), "Allegro 5 Error", "Error", "Failed to create event queue.", NULL, ALLEGRO_MESSAGEBOX_ERROR); 106 destroy_pointers(); 107 return -1; 108 } 109 110 al_register_event_source(event_queue, al_get_display_event_source(display)); 111 al_register_event_source(event_queue, al_get_mouse_event_source()); 112 al_register_event_source(event_queue, al_get_keyboard_event_source()); 113 114 return 0; 115} 116 117void destroy_pointers(){ 118 if (display != NULL) al_destroy_display(display); 119 if (event_queue != NULL) al_destroy_event_queue(event_queue); 120} 121 122void exit_allegro(){ 123 destroy_pointers(); 124} 125 126 127void events(ALLEGRO_EVENT &event, bool &cycle, bool &doexit){ 128 al_wait_for_event(event_queue, &event); 129 130 if (event.type == ALLEGRO_EVENT_DISPLAY_CLOSE){ 131 doexit = true; 132 } 133}

The first code gives me this result: {"name":"NVos1jk.png","src":"\/\/djungxnpq2nug.cloudfront.net\/image\/cache\/d\/f\/dfe7e5f7a63eac38ec37b7080a00fcb9.png","w":1280,"h":747,"tn":"\/\/djungxnpq2nug.cloudfront.net\/image\/cache\/d\/f\/dfe7e5f7a63eac38ec37b7080a00fcb9"}NVos1jk.png

But the second one just crashes almost every time. Although in one single occasion it didn't crash and the program did run, but instead of showing the grass bitmap, it showed a white square in the place it should be. I had added that line to check if it was loading:

#SelectExpand
1if (grass == NULL){ 2 al_show_native_message_box(al_get_current_display(), "Allegro 5 Error", "Error", "Couldn't load bitmap.", NULL, ALLEGRO_MESSAGEBOX_ERROR); 3}

But when the program crashes that error message doesn't show up. I can trigger that error message if I, for example, write the wrong file path in the "al_load_bitmap" line.

Sorry if the code is too long for the post, I tried to simplify the program to be able to include it here while still being able to reproduce the same bug.

EDIT:

Dizzy Egg said:

Shouldn't that be:

thing.push_back(new THING());

Now... that worked, it didn't crash and it worked as it was supposed to. But I don't get it... I used to do the list containers that way before, but many people told me it was not ideal to create the elements as pointers. It shouldn't crash even if I create the elements as not-pointers, should it?

So changing these lines to this, solves it, but I don't undersand why:

#SelectExpand
1std::list<THING*> thing; 2std::list<THING*>::iterator itt; 3thing.push_back(new THING());

#SelectExpand
1itt = thing.begin(); 2(**itt).print();

But regardless of why it worked, thank you a lot!!

DanielH
Member #934
January 2001
avatar

Thing is static. When you put in a list. You are adding a copy.

Now you have multiple copies with multiple pointers to one object.

Which one destroys the bitmap? When is the bitmap destroyed?

In the internals, the destructor might be called for anyone of them. Freeing the internal memory for the bitmap, but you still might have other thing objects with pointers to it.

When you create a list of thing pointers, you are only adding one object. The destructor will be called any number of times, but the internal memory is not freed.

EDIT ****************
In your original code, add a print statement (to file if needeD) to the constructor and another to the destructor. See how many times they are called.

Dizzy Egg
Member #10,824
March 2009
avatar

If you don't want to use pointers, or the new keyword, you still have to create the objects before adding them to the list. Saying:

thing.push_back(THING());

doesn't make much sense. That's like saying "add a constructor call to my list".

So you could say:

THING thing_1; //Calls THING() automatically
THING thing_2;
THING thing_3;

and then say:

thing.push_back(thing_1);
thing.push_back(thing_2);
thing.push_back(thing_3);

but IMHO that defeats the point of the list (very subjective I guess). Much neater to say:

for(however many things I want)
{
    thing.push_back(new thing());
}

and then loop through the list to render the tiles or whatever.
That's my thoughts anyway.

----------------------------------------------------
Please check out my songs:
https://soundcloud.com/dont-rob-the-machina

Xinomorph
Member #17,927
July 2020

DanielH said:

Thing is static. When you put in a list. You are adding a copy.

Now you have multiple copies with multiple pointers to one object.

Which one destroys the bitmap? When is the bitmap destroyed?

In the internals, the destructor might be called for anyone of them. Freeing the internal memory for the bitmap, but you still might have other thing objects with pointers to it.

When you create a list of thing pointers, you are only adding one object. The destructor will be called any number of times, but the internal memory is not freed.

EDIT ****************
In your original code, add a print statement (to file if needeD) to the constructor and another to the destructor. See how many times they are called.

Thanks, I now understand. I was ignorant about the difference between declaring a list element as a pointer, and declaring it as a static element. I thought the list container already created the nodes for the objects as pointers, and that declaring it as a pointer myself would be redundant, like two pointers nested. I see I was wrong now.

BTW, I did what you suggested to check how many times the constructor and destructor were called in my original code (the second one I posted, the one with the list but without pointers); and you are right, the destructor was being called once, even before the main loop of the program started. The constructor I declared in my code was only called once, so I assume the copy constructor is being called, and then the destructor is also being called when the element created by the copy constructor disappears.

Dizzy Egg said:

If you don't want to use pointers, or the new keyword, you still have to create the objects before adding them to the list. Saying:

thing.push_back(THING());

doesn't make much sense. That's like saying "add a constructor call to my list".

So you could say:

THING thing_1; //Calls THING() automatically
THING thing_2;
THING thing_3;

and then say:

thing.push_back(thing_1);
thing.push_back(thing_2);
thing.push_back(thing_3);

but IMHO that defeats the point of the list (very subjective I guess). Much neater to say:

for(however many things I want)
{
    thing.push_back(new thing());
}

and then loop through the list to render the tiles or whatever.
That's my thoughts anyway.

No, I think you are right, declaring a different list for every element kinda defeats the purpose of the list. I don't have any problem with declaring the list elements as pointers, in fact I'll do that now! It's just that I didn't know it was necessary, now I see the difference!

Thank you both!! I was trapped in this bug for 2 days!

Peter Hull
Member #1,136
March 2001

Dizzy Egg said:

thing.push_back(THING());

doesn't make much sense. That's like saying "add a constructor call to my list".

It does make sense. It is constructing a temporary THING, adding a copy of it to the list, and then destroying the temporary.

As DanielH said above, the problem is that copying a THING just makes a simple copy of the grass member, so when the temporary is destroyed, the copy inside the list still has a pointer to a bitmap that has been destroyed.

Your choices are to use pointers: list<THING*> and push_back(new THING)
or
implement the copy constructor for THING so it makes a true copy (which would involve al_clone_bitmap I suppose.)

The first one would be more straightforward.

torhu
Member #2,727
September 2002
avatar

You could also use emplace_back to construct the object in-place, avoiding the pointless copy.

bamccaig
Member #7,536
July 2006
avatar

A somewhat better design is to offload the loading/managing/destroying of resources (in this case, a bitmap from a file on disk) to a special "resource manager" object or class. That way instead of your THING knowing about files and loading bitmaps it just knows about the resource manager and can ask for a resource from it by path or name (and type?); either by passing a pointer or reference to the resource manager to the THING constructor, or by the resource manager being a static class and therefore global.

You can also wrap the pointers to objects that Allegro gives you in smart pointers (e.g., std::shared_ptr<> or boost::shared_ptr<>) with the destructor function specified so that your copies of the pointers will all be tracked, and the resource won't actually be freed until it's no longer being referenced anyway.

std::shared_ptr<ALLEGRO_BITMAP> grass(al_load_bitmap(...), al_destroy_bitmap);

Edgar Reynaldo
Major Reynaldo
May 2007
avatar

DanielH
Member #934
January 2001
avatar

Just remember that clearing a list will not delete the memory. You must delete each object. Then clear the list.

std::list<THING*> tlist;

tlist.push_back(new THING());
tlist.push_back(new THING());
tlist.push_back(new THING());

for(std::list<THING*>::const_iterator it = tlist.begin(); it != tlist.end(); ++it)
{
    delete *it;
} 
tlist.clear()

bamccaig
Member #7,536
July 2006
avatar

DanielH
Member #934
January 2001
avatar

Yes, use smart pointers

You can use your original code if you used smart pointers

smart pointers keep a tally of how many copies point to an object and won't delete the object until the last destructor is called.

Go to: