widelands-dev team mailing list archive

[SirVer <SirVer@xxxxxx>]

>>> from Gun
I think we could further simplify FieldOverlayManager::get_overlays by making kLevelForBuildHelp part of the OverlayLevel enum class, something like this:

 auto it = overlays_.lower_bound(c);
 while (it != overlays_.end() && it->first == c) {
            if (buildhelp_ && it->second.level == OverlayLevel::kLevelForBuildHelp) {
                result->emplace_back(buildhelp_infos_[get_buildhelp_overlay(c)]);
            } else {
  result->emplace_back(it->second.pic, it->second.hotspot);
  ++it;
            }
 }

We should then be able to use a range-bases for loop too?
<<<

OverlayLevel is part of the public API of this class and kLevelForBuildHelp should never be used when 'register_overlay' is called. I therefore opted to not make it part of the enum class to make misuse harder.

Also, you cannot range base over all values in an enum class easily [1] and since no overlay will ever be inserted into 'overlays_' where 'it->second.level == OverlayLevel::kLevelForBuildHelp' - since buildhelp is handled differently from all other overlays - the suggested loop simplification unfortunately does not work.

I am not super happy with how this class looks and agree with your push to make it simpler. Unfortunately the correct way I see for this would be to make buildhelp not be special - which would mean that the class will require much more memory which is also not super desirable. Alternatively we could scratch the multimap and instead have callbacks for each OverlayLevel and calculate which overlays are required each frame on the fly. But that is also a pretty deep change. I think for now I am satisfied with having pushed the class a bit towards simplification.

[1] https://stackoverflow.com/questions/8498300/allow-for-range-based-for-with-enum-classes
-- 
https://code.launchpad.net/~widelands-dev/widelands/toggle_resources/+merge/328958
Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/toggle_resources into lp:widelands.