|
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: 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: 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: 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
|
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
|
Shouldn't that be: thing.push_back(new THING());
---------------------------------------------------- |
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: 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: 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"} 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: 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: 1std::list<THING*> thing;
2std::list<THING*>::iterator itt;
3thing.push_back(new THING());
1itt = thing.begin();
2(**itt).print();
But regardless of why it worked, thank you a lot!! |
DanielH
Member #934
January 2001
|
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 **************** |
Dizzy Egg
Member #10,824
March 2009
|
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.
---------------------------------------------------- |
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 **************** 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. 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) The first one would be more straightforward.
|
torhu
Member #2,727
September 2002
|
You could also use emplace_back to construct the object in-place, avoiding the pointless copy. |
bamccaig
Member #7,536
July 2006
|
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);
-- acc.js | al4anim - Allegro 4 Animation library | Allegro 5 VS/NuGet Guide | Allegro.cc Mockup | Allegro.cc <code> Tag | Allegro 4 Timer Example (w/ Semaphores) | Allegro 5 "Winpkg" (MSVC readme) | Bambot | Blog | C++ STL Container Flowchart | Castopulence Software | Check Return Values | Derail? | Is This A Discussion? Flow Chart | Filesystem Hierarchy Standard | Clean Code Talks - Global State and Singletons | How To Use Header Files | GNU/Linux (Debian, Fedora, Gentoo) | rot (rot13, rot47, rotN) | Streaming |
Edgar Reynaldo
Major Reynaldo
May 2007
|
For more information, google shallow and deep copies, pass by value, and pass by reference. My Website! | EAGLE GUI Library Demos | My Deviant Art Gallery | Spiraloid Preview | A4 FontMaker | Skyline! (Missile Defense) Eagle and Allegro 5 binaries | Older Allegro 4 and 5 binaries | Allegro 5 compile guide |
DanielH
Member #934
January 2001
|
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
|
...or use smart pointers and it will automatically delete the memory when it's no longer used. -- acc.js | al4anim - Allegro 4 Animation library | Allegro 5 VS/NuGet Guide | Allegro.cc Mockup | Allegro.cc <code> Tag | Allegro 4 Timer Example (w/ Semaphores) | Allegro 5 "Winpkg" (MSVC readme) | Bambot | Blog | C++ STL Container Flowchart | Castopulence Software | Check Return Values | Derail? | Is This A Discussion? Flow Chart | Filesystem Hierarchy Standard | Clean Code Talks - Global State and Singletons | How To Use Header Files | GNU/Linux (Debian, Fedora, Gentoo) | rot (rot13, rot47, rotN) | Streaming |
DanielH
Member #934
January 2001
|
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. |
|