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:

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:

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

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

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?

Shouldn't that be:

thing.push_back(new THING());

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

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:

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

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:

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:

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:

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

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.

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.

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.

thing.push_back(THING());

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

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

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

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!

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.

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

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);

For more information, google shallow and deep copies, pass by value, and pass by reference.

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()

...or use smart pointers and it will automatically delete the memory when it's no longer used.

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.