widelands-dev team mailing list archive
-
widelands-dev team
-
Mailing list archive
-
Message #14364
Re: [Merge] lp:~widelands-dev/widelands/rework-resource-indicators into lp:widelands
Code review on the diff done - see comments.
Diff comments:
> === modified file 'data/tribes/atlanteans.lua'
> --- data/tribes/atlanteans.lua 2017-11-23 09:13:06 +0000
> +++ data/tribes/atlanteans.lua 2018-09-01 08:09:17 +0000
> @@ -73,6 +75,31 @@
> },
> },
>
> + resource_indicators = {
> + [""] = {
Please call this "none" for consistency
> + [0] = "atlanteans_resi_none",
> + },
> + coal = {
> + [10] = "atlanteans_resi_coal_1",
> + [20] = "atlanteans_resi_coal_2",
> + },
> + iron = {
> + [10] = "atlanteans_resi_iron_1",
> + [20] = "atlanteans_resi_iron_2",
> + },
> + gold = {
> + [10] = "atlanteans_resi_gold_1",
> + [20] = "atlanteans_resi_gold_2",
> + },
> + stones = {
> + [10] = "atlanteans_resi_stones_1",
> + [20] = "atlanteans_resi_stones_2",
> + },
> + water = {
> + [100] = "atlanteans_resi_water",
> + },
> + },
> +
> -- Wares positions in wares windows.
> -- This also gives us the information which wares the tribe uses.
> -- Each subtable is a column in the wares windows.
>
> === added file 'data/tribes/immovables/resi/atlanteans/helptexts/stones_1.lua'
> --- data/tribes/immovables/resi/atlanteans/helptexts/stones_1.lua 1970-01-01 00:00:00 +0000
> +++ data/tribes/immovables/resi/atlanteans/helptexts/stones_1.lua 2018-09-01 08:09:17 +0000
> @@ -0,0 +1,27 @@
> +function immovable_helptext(tribe)
> + local helptext = {
> + atlanteans = pgettext("sentence_separator", "%s %s"):bformat(
Make this one default and kick out the barbarian and empire helptexts
> + -- TRANSLATORS: Helptext for an Atlantean resource: Stones
> + _("Precious stones are used in the construction of big buildings. They can be dug up by a crystal mine. You will also get granite from the mine."),
> + -- TRANSLATORS: Helptext for an Atlantean resource: Stones
> + _("There are only a few precious stones here.")),
> + barbarians = pgettext("sentence_separator", "%s %s"):bformat(
> + -- TRANSLATORS: Helptext for a Barbarian resource: Stones
> + _("Granite is a basic building material and can be dug up by a granite mine."),
> + -- TRANSLATORS: Helptext for a Barbarian resource: Stones
> + _("There is only a little bit of granite here.")),
> + empire = pgettext("sentence_separator", "%s %s"):bformat(
> + -- TRANSLATORS: Helptext for an Empire resource: Stones
> + _("Marble is a basic building material and can be dug up by a marble mine. You will also get granite from the mine."),
> + -- TRANSLATORS: Helptext for an Empire resource: Stones
> + _("There is only a little bit of marble here."))
> + }
> + local result = ""
> + if tribe then
> + result = helptext[tribe]
> + else
> + result = helptext["default"]
> + end
> + if (result == nil) then result = "" end
> + return result
> +end
>
> === added file 'data/tribes/immovables/resi/atlanteans/helptexts/stones_2.lua'
> --- data/tribes/immovables/resi/atlanteans/helptexts/stones_2.lua 1970-01-01 00:00:00 +0000
> +++ data/tribes/immovables/resi/atlanteans/helptexts/stones_2.lua 2018-09-01 08:09:17 +0000
> @@ -0,0 +1,27 @@
> +function immovable_helptext(tribe)
> + local helptext = {
> + atlanteans = pgettext("sentence_separator", "%s %s"):bformat(
Make this one default and kick out the barbarian and empire helptexts
> + -- TRANSLATORS: Helptext for an Atlantean resource: Stones
> + _("Precious stones are used in the construction of big buildings. They can be dug up by a crystal mine. You will also get granite from the mine."),
> + -- TRANSLATORS: Helptext for an Atlantean resource: Stones
> + _("There are many precious stones here.")),
> + barbarians = pgettext("sentence_separator", "%s %s"):bformat(
> + -- TRANSLATORS: Helptext for a Barbarian resource: Stones
> + _("Granite is a basic building material and can be dug up by a granite mine."),
> + -- TRANSLATORS: Helptext for a Barbarian resource: Stones
> + _("There is a lot of granite here.")),
> + empire = pgettext("sentence_separator", "%s %s"):bformat(
> + -- TRANSLATORS: Helptext for an Empire resource: Stones
> + _("Marble is a basic building material and can be dug up by a marble mine. You will also get granite from the mine."),
> + -- TRANSLATORS: Helptext for an Empire resource: Stones
> + _("There is a lot of marble here."))
> + }
> + local result = ""
> + if tribe then
> + result = helptext[tribe]
> + else
> + result = helptext["default"]
> + end
> + if (result == nil) then result = "" end
> + return result
> +end
>
> === added file 'data/tribes/immovables/resi/barbarians/helptexts/stones_1.lua'
> --- data/tribes/immovables/resi/barbarians/helptexts/stones_1.lua 1970-01-01 00:00:00 +0000
> +++ data/tribes/immovables/resi/barbarians/helptexts/stones_1.lua 2018-09-01 08:09:17 +0000
> @@ -0,0 +1,27 @@
> +function immovable_helptext(tribe)
> + local helptext = {
> + atlanteans = pgettext("sentence_separator", "%s %s"):bformat(
> + -- TRANSLATORS: Helptext for an Atlantean resource: Stones
> + _("Precious stones are used in the construction of big buildings. They can be dug up by a crystal mine. You will also get granite from the mine."),
> + -- TRANSLATORS: Helptext for an Atlantean resource: Stones
> + _("There are only a few precious stones here.")),
> + barbarians = pgettext("sentence_separator", "%s %s"):bformat(
Make this one default and kick out the atlantean and empire helptexts
> + -- TRANSLATORS: Helptext for a Barbarian resource: Stones
> + _("Granite is a basic building material and can be dug up by a granite mine."),
> + -- TRANSLATORS: Helptext for a Barbarian resource: Stones
> + _("There is only a little bit of granite here.")),
> + empire = pgettext("sentence_separator", "%s %s"):bformat(
> + -- TRANSLATORS: Helptext for an Empire resource: Stones
> + _("Marble is a basic building material and can be dug up by a marble mine. You will also get granite from the mine."),
> + -- TRANSLATORS: Helptext for an Empire resource: Stones
> + _("There is only a little bit of marble here."))
> + }
> + local result = ""
> + if tribe then
> + result = helptext[tribe]
> + else
> + result = helptext["default"]
> + end
> + if (result == nil) then result = "" end
> + return result
> +end
>
> === added file 'data/tribes/immovables/resi/barbarians/helptexts/stones_2.lua'
> --- data/tribes/immovables/resi/barbarians/helptexts/stones_2.lua 1970-01-01 00:00:00 +0000
> +++ data/tribes/immovables/resi/barbarians/helptexts/stones_2.lua 2018-09-01 08:09:17 +0000
> @@ -0,0 +1,27 @@
> +function immovable_helptext(tribe)
> + local helptext = {
> + atlanteans = pgettext("sentence_separator", "%s %s"):bformat(
> + -- TRANSLATORS: Helptext for an Atlantean resource: Stones
> + _("Precious stones are used in the construction of big buildings. They can be dug up by a crystal mine. You will also get granite from the mine."),
> + -- TRANSLATORS: Helptext for an Atlantean resource: Stones
> + _("There are many precious stones here.")),
> + barbarians = pgettext("sentence_separator", "%s %s"):bformat(
Make this one default and kick out the atlantean and empire helptexts
> + -- TRANSLATORS: Helptext for a Barbarian resource: Stones
> + _("Granite is a basic building material and can be dug up by a granite mine."),
> + -- TRANSLATORS: Helptext for a Barbarian resource: Stones
> + _("There is a lot of granite here.")),
> + empire = pgettext("sentence_separator", "%s %s"):bformat(
> + -- TRANSLATORS: Helptext for an Empire resource: Stones
> + _("Marble is a basic building material and can be dug up by a marble mine. You will also get granite from the mine."),
> + -- TRANSLATORS: Helptext for an Empire resource: Stones
> + _("There is a lot of marble here."))
> + }
> + local result = ""
> + if tribe then
> + result = helptext[tribe]
> + else
> + result = helptext["default"]
> + end
> + if (result == nil) then result = "" end
> + return result
> +end
>
> === added file 'data/tribes/immovables/resi/empire/helptexts/stones_1.lua'
> --- data/tribes/immovables/resi/empire/helptexts/stones_1.lua 1970-01-01 00:00:00 +0000
> +++ data/tribes/immovables/resi/empire/helptexts/stones_1.lua 2018-09-01 08:09:17 +0000
> @@ -0,0 +1,27 @@
> +function immovable_helptext(tribe)
> + local helptext = {
> + atlanteans = pgettext("sentence_separator", "%s %s"):bformat(
> + -- TRANSLATORS: Helptext for an Atlantean resource: Stones
> + _("Precious stones are used in the construction of big buildings. They can be dug up by a crystal mine. You will also get granite from the mine."),
> + -- TRANSLATORS: Helptext for an Atlantean resource: Stones
> + _("There are only a few precious stones here.")),
> + barbarians = pgettext("sentence_separator", "%s %s"):bformat(
> + -- TRANSLATORS: Helptext for a Barbarian resource: Stones
> + _("Granite is a basic building material and can be dug up by a granite mine."),
> + -- TRANSLATORS: Helptext for a Barbarian resource: Stones
> + _("There is only a little bit of granite here.")),
> + empire = pgettext("sentence_separator", "%s %s"):bformat(
Make this one default and kick out the atlantean and barbarian helptexts
> + -- TRANSLATORS: Helptext for an Empire resource: Stones
> + _("Marble is a basic building material and can be dug up by a marble mine. You will also get granite from the mine."),
> + -- TRANSLATORS: Helptext for an Empire resource: Stones
> + _("There is only a little bit of marble here."))
> + }
> + local result = ""
> + if tribe then
> + result = helptext[tribe]
> + else
> + result = helptext["default"]
> + end
> + if (result == nil) then result = "" end
> + return result
> +end
>
> === added file 'data/tribes/immovables/resi/empire/helptexts/stones_2.lua'
> --- data/tribes/immovables/resi/empire/helptexts/stones_2.lua 1970-01-01 00:00:00 +0000
> +++ data/tribes/immovables/resi/empire/helptexts/stones_2.lua 2018-09-01 08:09:17 +0000
> @@ -0,0 +1,27 @@
> +function immovable_helptext(tribe)
> + local helptext = {
> + atlanteans = pgettext("sentence_separator", "%s %s"):bformat(
> + -- TRANSLATORS: Helptext for an Atlantean resource: Stones
> + _("Precious stones are used in the construction of big buildings. They can be dug up by a crystal mine. You will also get granite from the mine."),
> + -- TRANSLATORS: Helptext for an Atlantean resource: Stones
> + _("There are many precious stones here.")),
> + barbarians = pgettext("sentence_separator", "%s %s"):bformat(
> + -- TRANSLATORS: Helptext for a Barbarian resource: Stones
> + _("Granite is a basic building material and can be dug up by a granite mine."),
> + -- TRANSLATORS: Helptext for a Barbarian resource: Stones
> + _("There is a lot of granite here.")),
> + empire = pgettext("sentence_separator", "%s %s"):bformat(
Make this one default and kick out the atlantean and barbarian helptexts
> + -- TRANSLATORS: Helptext for an Empire resource: Stones
> + _("Marble is a basic building material and can be dug up by a marble mine. You will also get granite from the mine."),
> + -- TRANSLATORS: Helptext for an Empire resource: Stones
> + _("There is a lot of marble here."))
> + }
> + local result = ""
> + if tribe then
> + result = helptext[tribe]
> + else
> + result = helptext["default"]
> + end
> + if (result == nil) then result = "" end
> + return result
> +end
>
> === added file 'data/tribes/immovables/resi/frisians/helptexts/stones_1.lua'
> --- data/tribes/immovables/resi/frisians/helptexts/stones_1.lua 1970-01-01 00:00:00 +0000
> +++ data/tribes/immovables/resi/frisians/helptexts/stones_1.lua 2018-09-01 08:09:17 +0000
> @@ -0,0 +1,27 @@
> +function immovable_helptext(tribe)
> + local helptext = {
> + atlanteans = pgettext("sentence_separator", "%s %s"):bformat(
Create a default helptext for the frisians and kick out the others
> + -- TRANSLATORS: Helptext for an Atlantean resource: Stones
> + _("Precious stones are used in the construction of big buildings. They can be dug up by a crystal mine. You will also get granite from the mine."),
> + -- TRANSLATORS: Helptext for an Atlantean resource: Stones
> + _("There are only a few precious stones here.")),
> + barbarians = pgettext("sentence_separator", "%s %s"):bformat(
> + -- TRANSLATORS: Helptext for a Barbarian resource: Stones
> + _("Granite is a basic building material and can be dug up by a granite mine."),
> + -- TRANSLATORS: Helptext for a Barbarian resource: Stones
> + _("There is only a little bit of granite here.")),
> + empire = pgettext("sentence_separator", "%s %s"):bformat(
> + -- TRANSLATORS: Helptext for an Empire resource: Stones
> + _("Marble is a basic building material and can be dug up by a marble mine. You will also get granite from the mine."),
> + -- TRANSLATORS: Helptext for an Empire resource: Stones
> + _("There is only a little bit of marble here."))
> + }
> + local result = ""
> + if tribe then
> + result = helptext[tribe]
> + else
> + result = helptext["default"]
> + end
> + if (result == nil) then result = "" end
> + return result
> +end
>
> === added file 'data/tribes/immovables/resi/frisians/helptexts/stones_2.lua'
> --- data/tribes/immovables/resi/frisians/helptexts/stones_2.lua 1970-01-01 00:00:00 +0000
> +++ data/tribes/immovables/resi/frisians/helptexts/stones_2.lua 2018-09-01 08:09:17 +0000
> @@ -0,0 +1,27 @@
> +function immovable_helptext(tribe)
> + local helptext = {
> + atlanteans = pgettext("sentence_separator", "%s %s"):bformat(
Create a default helptext for the frisians and kick out the others
> + -- TRANSLATORS: Helptext for an Atlantean resource: Stones
> + _("Precious stones are used in the construction of big buildings. They can be dug up by a crystal mine. You will also get granite from the mine."),
> + -- TRANSLATORS: Helptext for an Atlantean resource: Stones
> + _("There are many precious stones here.")),
> + barbarians = pgettext("sentence_separator", "%s %s"):bformat(
> + -- TRANSLATORS: Helptext for a Barbarian resource: Stones
> + _("Granite is a basic building material and can be dug up by a granite mine."),
> + -- TRANSLATORS: Helptext for a Barbarian resource: Stones
> + _("There is a lot of granite here.")),
> + empire = pgettext("sentence_separator", "%s %s"):bformat(
> + -- TRANSLATORS: Helptext for an Empire resource: Stones
> + _("Marble is a basic building material and can be dug up by a marble mine. You will also get granite from the mine."),
> + -- TRANSLATORS: Helptext for an Empire resource: Stones
> + _("There is a lot of marble here."))
> + }
> + local result = ""
> + if tribe then
> + result = helptext[tribe]
> + else
> + result = helptext["default"]
> + end
> + if (result == nil) then result = "" end
> + return result
> +end
>
> === modified file 'src/logic/map_objects/tribes/tribe_descr.cc'
> --- src/logic/map_objects/tribes/tribe_descr.cc 2018-04-07 16:59:00 +0000
> +++ src/logic/map_objects/tribes/tribe_descr.cc 2018-09-01 08:09:17 +0000
> @@ -374,41 +391,31 @@
> DescriptionIndex TribeDescr::get_resource_indicator(ResourceDescription const* const res,
> const ResourceAmount amount) const {
> if (!res || !amount) {
> - DescriptionIndex idx = immovable_index("resi_none");
> - if (!has_immovable(idx)) {
> - throw GameDataError("There is no resource indicator for resi_none!");
> - }
> - return idx;
> - }
> -
> - int32_t i = 1;
> - int32_t num_indicators = 0;
> - for (;;) {
> - const std::string resi_filename =
> - (boost::format("resi_%s%i") % res->name().c_str() % i).str();
> - if (!has_immovable(immovable_index(resi_filename))) {
> - break;
> - }
> - ++i;
> - ++num_indicators;
> - }
> -
> - if (!num_indicators) {
> - throw GameDataError("There is no resource indicator for resource %s", res->name().c_str());
> - }
> -
> - int32_t bestmatch =
> - static_cast<int32_t>((static_cast<float>(amount) / res->max_amount()) * num_indicators);
> - if (bestmatch > num_indicators) {
> - throw GameDataError("Amount of %s is %i but max amount is %i", res->name().c_str(),
> - static_cast<unsigned int>(amount),
> - static_cast<unsigned int>(res->max_amount()));
> - }
> - if (amount < res->max_amount()) {
> - bestmatch += 1; // Resi start with 1, not 0
> - }
> -
> - return immovable_index((boost::format("resi_%s%i") % res->name().c_str() % bestmatch).str());
> + auto list = resource_indicators_.find("");
Call this "none" for consistency.
> + if (list == resource_indicators_.end() || list->second.empty())
> + throw GameDataError("Tribe '%s' has no indicator for no resources!", descname_.c_str());
> + return list->second.begin()->second;
> + }
> +
> + auto list = resource_indicators_.find(res->name());
> + if (list == resource_indicators_.end() || list->second.empty())
> + throw GameDataError("Tribe '%s' has no indicators for resource '%s'!", descname_.c_str(), res->name().c_str());
> +
> + uint32_t lowest = 0;
> + for (ResourceIndicatorList::const_iterator it = list->second.begin(); it != list->second.end(); it++) {
You can use a range-based for loop here - no need to manually mess with iterators ;)
> + if (it->first < amount)
Please do not omit optional {}, because that can cause bugs.
> + continue;
> + else if (lowest < amount || it->first < lowest)
> + lowest = it->first;
> + }
> +
> + if (lowest < amount) {
> + throw GameDataError(
> + "Tribe '%s' has no indicators for amount %i of resource '%s' (highest possible amount is %i)!",
> + descname_.c_str(), amount, res->name().c_str(), lowest);
> + }
> +
> + return list->second.find(lowest)->second;
> }
>
> void TribeDescr::resize_ware_orders(size_t maxLength) {
>
> === modified file 'src/logic/map_objects/tribes/tribe_descr.h'
> --- src/logic/map_objects/tribes/tribe_descr.h 2018-04-07 16:59:00 +0000
> +++ src/logic/map_objects/tribes/tribe_descr.h 2018-09-01 08:09:17 +0000
> @@ -49,6 +49,16 @@
> struct Event;
>
> /*
> +Resource indicators:
> +A ResiSet maps the resource name to a ResiList.
Comment does not match actual type name - I guess you renamed it while you were working on it ;)
> +A ResiList maps resource amounts to the DescrIndex of an immovable this player uses.
ResourceIndicatorList, DescriptionIndex
> +Special case: The ResiList mapped to "" contains resis that will be used in locations
"none"
> +without resources. If it has several entries, result is arbitrary.
> +*/
> +using ResourceIndicatorList = std::map<uint32_t, DescriptionIndex>;
> +using ResourceIndicatorSet = std::map<std::string, ResourceIndicatorList>;
> +
> +/*
> Tribes
> ------
>
>
> === modified file 'src/map_io/tribes_legacy_lookup_table.cc'
> --- src/map_io/tribes_legacy_lookup_table.cc 2018-04-07 16:59:00 +0000
> +++ src/map_io/tribes_legacy_lookup_table.cc 2018-09-01 08:09:17 +0000
> @@ -194,9 +194,19 @@
> {"cornfield_m", "cornfield_medium"},
> {"cornfield_s", "cornfield_small"},
> {"cornfield_t", "cornfield_tiny"},
> - {"resi_granite1", "resi_stones1"},
> - {"resi_granite2", "resi_stones2"},
You also need to map {"resi_stones1", "atlanteans_resi_stones_1"} etc. - same for the other tribes below.
> + {"resi_granite1", "atlanteans_resi_stones_1"},
> + {"resi_granite2", "atlanteans_resi_stones_2"},
> {"shipconstruction", "atlanteans_shipconstruction"},
> + {"resi_coal1", "atlanteans_resi_coal_1"},
> + {"resi_iron1", "atlanteans_resi_iron_1"},
> + {"resi_gold1", "atlanteans_resi_gold_1"},
> + {"resi_stones1", "atlanteans_resi_stones_1"},
> + {"resi_coal2", "atlanteans_resi_coal_2"},
> + {"resi_iron2", "atlanteans_resi_iron_2"},
> + {"resi_gold2", "atlanteans_resi_gold_2"},
> + {"resi_stones2", "atlanteans_resi_stones_2"},
> + {"resi_none", "atlanteans_resi_none"},
> + {"resi_water1", "atlanteans_resi_water"},
> }),
> std::make_pair("barbarians",
> std::map<std::string, std::string>{
--
https://code.launchpad.net/~widelands-dev/widelands/rework-resource-indicators/+merge/353996
Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/rework-resource-indicators.
References
