So I'm currently working on a project that requires me to have a map with provinces. I am currently having a map drawn in GIMP2.0 and loading the map with the standard SFML image loading function. This works fine. The place where I have an issue is reading the image file color data to determine what part of the map is what province. The map has each province colored in a unique color. Borders are added and then a new image without province colors is generated. So the new map has land one color, inland sea another color, and oceans another color. Borders are a lighter color than the land. The color map is kept so that it reads the mouse position when you click checks the color and says, oh you clicked on province X.

This is all great except I feel like the loading time is excessive. There are currently 2860 land provinces and there will probably be many more, plus inland sea, and ocean provinces. When the colored map is loaded a function goes through and checks the color of each pixel and adds the pixel to the appropriate province as an owned pixel of that province.

This data is used by other functions such as drawing the country map. So the program checks the country of a province and colors every pixel of each province the appropriate country color on the country map. The country map is a specific filter; there are many others. pixelsOwned is just a list of coordinates for each pixel, x,y.

My profiler tells me that the functions that generates each province's list of owned pixels is the most time consuming of the loading functions. Although many of the functions that involve coloring the map take quite a bit of time. It takes about 4 seconds to load up just if I comment out all the map coloring functions.

Is there some superior way to reading the data from every pixel?

Here is the function below:


void MapManager::loadMapPixelData(sf::Image *image) {
sf::Vector2u pixelSize = image->getSize();
int width = pixelSize.x;
int height = pixelSize.y;
for (int i = 0; i < width; ++i) {
for (int j = 0; j < height; ++j) {
sf::Color color = image->getPixel(i, j);
int red = color.r;
int green = color.g;
int blue = color.b;
for (int k = 0; k < provinces.size(); ++k) {
if (red == mapProvinceValues[k].red && green == mapProvinceValues[k].green && blue == mapProvinceValues[k].blue) {
getProvinceById(k+1)->pixelsOwned.push_back({i, j});
break;
}
}
provincePixels.push_back({i, j, red, green, blue});
}

}

}

There are a number of issues with your code that can be addressed without changing what data you're storing:

 for (int i = 0; i < width; ++i) {
        for (int j = 0; j < height; ++j) {

Generally it's faster to iterate through a 2d image's pixels in the order that they exist in memory. So swapping these two lines (assuming pixels are stored scanline by scanline) might speed things up. However, the slowness of what's going on inside of your loop will probably dominate your performance, I so doubt you'll see anything noticeable by making this change.

            for (int k = 0; k < provinces.size(); ++k) {

You're potentially iterating through 2860 provinces for every pixel. That's kind of ridiculous. Store them in an unordered_map and use the color as a key.

                     getProvinceById(k+1)->pixelsOwned.push_back({i, j});

You're adding to a pixelsOwned vector incrementally. Do you pre-size it to a reasonable capacity beforehand? If not, you're going to be hitting a ton of a reallocations as the vector grows.

Have you profiled your code to see where your actual bottleneck is? This should help determine what the best first course of action is.

Anyway, I would question the need to store these "pixel lists" at all. I don't know exactly how you're doing your drawing, but I'm sure there is a cheaper way. For instance, instead of a list of pixels for each country, just store a bounding rectangle, and compare each location against the original data.

The profiler I used told me that that function, and similar functions, suck up the majority of the load time. I don't think it can get much more specific. Well it did say that some thing with the word allocate in it and the [] operator were the parts eating up the time within that function. I suspect that was the reallocation thing you mentioned.