StatisticWaresDisplay hits shark infested water

[Klaus Halfmann]

StatisticWaresDisplay is based on some RGBColor colors[] with 31 entries,
after that there is a comment: // shark infested water, run!

we actually have 85 nr_wares so all colors after that are out of Bounds.

Thanks again to the http://clang.llvm.org/docs/AddressSanitizer.html.

I will provide a hotfix now, but as I am sort of colorblind. I will just use
some Gray colors. No idea were this palette comes from.

[GunChleoc (gunchleoc)]

How about you go round robin around the array when we run out of colors? We will get 2 wares with the same colors, but we can't guarantee that we will never run out of them again.

Alternatively, the colors could be defined for each ware in in data/tribes/wares.

[GunChleoc (gunchleoc)]

I looked that the branch you're working on and noticed this for the wares colors, in src/wui/ware_statistics_menu.cc:

uint8_t const nr_wares = parent.get_player()->egbase().tribes().nrwares();

This is the wares for all tribes, but we're only interested in the wares that the player's tribe can use. There are some wares functions in parent.get_player()->tribe();

Watch out if you use the TribeDescr's ware functions, there are of course gaps in the indexes for the wares that the tribe isn't using, so you'd need a separate counter.

We should still keep the cycling for the colors though.

[SirVer (sirver)]

Klaus, did you ever write that hotfix? If so, I did completely miss it. I made a fix for this in the attached branch - turns out there was already code that checked out of bounds. We just needlessly set colors the constructor.

Ups, I accidentally pushed this directly to trunk in http://bazaar.launchpad.net/~widelands-dev/widelands/trunk/revision/8511. Sorry for that :(.

[SirVer (sirver)]

I decided to leave trunk as is - removing this commit and repushing creates a mailbomb. And the fix is pretty uncontroversial I think.

[GunChleoc (gunchleoc)]

Consider your code reviewed :)

[Klaus Halfmann (klaus-halfmann)]

My original fix is here:
   http://bazaar.launchpad.net/~widelands-dev/widelands/bug_1730204-crash/revision/8496

basic fix was just using the % operator.

But while refactoring the code I found some strange duplicate calls I could avoid.

std::vector<uint32_t> const* ware_prod_stats = player->get_ware_production_statistics(cur_ware);
std::vector<uint32_t> const* ware_cons_stats = player->get_ware_consumption_statistics(cur_ware);
std::vector<uint32_t> const* ware_stock_stats = player->get_ware_production_statistics(cur_ware);

Once I have time, sigh. I will setup a new Branhc for that :-)

I will take a look in that staistic in trunk then.

[GunChleoc (gunchleoc)]

Fixed in build20-rc1