Allegro 5: trouble storing bitmaps in a std map

Question

I started creating a game in Visual Studio 2017 in C++ using Allegro 5. In order to make it easier to manage images, I created an ImageLoader class which would load and store all active images, and destroy them as necessary. It does so using a map which matches filenames to the corresponding images. Currently my main() code looks like this:

int main(){

if (!al_init()) {
    al_show_native_message_box(NULL, NULL, NULL, "Could not intialize allegro 5.", NULL, NULL);
    return -1;
}

ALLEGRO_DISPLAY *display = al_create_display(DEFAULT_SCREEN_WIDTH, DEFAULT_SCREEN_HEIGHT);
al_set_window_title(display, "Game title");

// make usre the display was created correctly
if (!display) {
    al_show_native_message_box(display, "Title", "settings", "Could not create Allegro window.", NULL, NULL);
}

// intialize fonts, primitives, keyboard,etc.
al_init_font_addon();
al_init_ttf_addon();
al_init_primitives_addon();
al_install_keyboard();
if(!al_init_image_addon()) {
al_show_native_message_box(display, "Error", "Error", "Failed to initialize al_init_image_addon!", 
                           NULL, ALLEGRO_MESSAGEBOX_ERROR);
return -1;}
ImageLoader image_loader;

The ImageLoader then loads an image for the player:

ALLEGRO_BITMAP *image = al_load_bitmap(filename);

if (image == NULL) {
    std::cout << filename << std::endl;
    std::cout << "loader failed to load image" << std::endl;
}
image_map[filename] = image;

When I test this part, it seems to work, as the image is not null. Note that image_map is declared like so in ImageLoader.h:

std::map<const char *, ALLEGRO_BITMAP*> image_map;

The problem arises in my main game loop when I then try to get the image from the ImageLoader:

for (GameImage image : imageList) {
            ALLEGRO_BITMAP *draw_image = image_loader.get_current_image(image);
            if (draw_image == NULL) {
                //TODO: error handling
                std::cout << "no image" << std::endl;
            }
            else
                al_draw_bitmap(draw_image, image.get_x(), image.get_y(), NULL);

        }

My check always shows that draw_image is null. Here is the code for get_current_image:

ALLEGRO_BITMAP * ImageLoader::get_current_image(GameImage image)
{
return image_map[image.get_image_filename()];
}

I have tested to make sure I'm using the same filename here as when I load the image and store it in the map by checking if (image_map.find(image.get_image_filename()) == image_map.end()), but even though this returns false the ALLEGRO_BITMAP pointer is still null. I have tried making the map store bitmaps instead of pointers, but this gives me an error when I try to add an element to the map since I'm not allowed to modify map values like this. Why might these pointers become null in between the time where I set them and the time where I retrieve them? I'm also open to other suggestions for how I should store my bitmaps.

EDIT: I revised my project, changing the instances of filenames as char arrays to std::strings. The image map is now declared as std::map<std::string, ALLEGRO_BITMAP*> image_map in ImageLoader.h, and the filename stored by GameImage is now std::string image_filename. load_image in ImageLoader.cpp now looks like this:

ALLEGRO_BITMAP *image = al_load_bitmap(filename.c_str());

if (image == NULL) {
    std::cout << filename << std::endl;
    std::cout << "loader failed to load image" << std::endl;
}
image_map[filename] = image;

Finally, get_current_image is still the same:

ALLEGRO_BITMAP * ImageLoader::get_current_image(GameImage image)
{
return image_map[image.get_image_filename()];
}

However, the same problem still persists. I have also checked the size of the image map and it stays at 1 throughout the duration of my program, with the filename for the image I insert as the key and the value starting as a non-null pointer to an bitmap and appearing to become null at some point.

EDIT 2:

After changing the char arrays to strings and fixing get_current_image() so that it no longer adds to the map if the searched filename is not found, I discovered that I had also made a mistake when loading the images in a line I'd forgotten to include when I initially posted the question:

current_screen.load_images(image_loader);

As it turned out, I had written load_images() like so:

void MainGameScreen::load_images(ImageLoader loader)

...meaning that the calls the function made to loader weren't actually applying to the ImageLoader I'd passed in. I changed it to:

void MainGameScreen::load_images(ImageLoader& loader)

...and now everything works fine.

Answer 1

Your problem is that using const char * as your key means that map is going to perform a direct address comparison, i.e. "Is the address of the string equal to the address of the string I'm holding in memory". This is almost certainly NOT the way you want to perform string comparisons.

There's actually two solutions to this problem, depending on your use-case. The first is to simply change const char * to std::string, which is the simplest solution and what you should have done by default.

std::map<std::string, ALLEGRO_BITMAP*> image_map;

And, broadly speaking, anywhere that you are using a string, you should either be using std::string or std::string const&. There's no reason to use anything else.

...UNLESS you're concerned about performance. And if you're writing a game, performance is almost certainly something you care about, which brings us to the second solution. Having to do lots of lookups to a map is going to invoke a large number of comparisons, and while this isn't normally a huge issue, it is here because each comparison is a full-blown string-based equality check.

The solution is, when you load images, issue each image a unique ID (like an int64_t or uint64_t), and assign those values to the GameImage object instead of the filename or pathname. Then, when you do lookups, use that ID to perform the lookup.

It's up to you. Swapping out const char * with std::string is almost certainly going to fix the logical errors in your code and make it work the way you expect. The rest is more of an optimization problem if you discover that having to do all those string comparisons is slowing down your program too dramatically.

EDIT:

Your new problem is that std::map's operator[] function automatically inserts a default value (in this case, nullptr) if it fails to find any pre-existing image with the name requested. The code you want looks more like this:

ALLEGRO_BITMAP * ImageLoader::get_current_image(GameImage image)
{
    auto it = image_map.find(image.get_image_filename());
    if(it == image_map.end()) return nullptr;
    else return it->second;
    //return image_map[image.get_image_filename()];
}

This way, failure to find an image won't trick any debugging tools you use into thinking that a valid (null) value is stored at that location.

You could also simplify it to this if you wanted to use the built-in exception facilities instead:

ALLEGRO_BITMAP * ImageLoader::get_current_image(GameImage image)
{
    //Will throw an exception if nothing is found
    return image_map.at(image.get_image_filename());
}