widelands-dev team mailing list archive

[Arty <diego.scheide@xxxxxx>]

Review: Needs Fixing

Reviewed the code and tested quite a lot. Looks pretty good, but still doesn't work quite right. Not hard to fix though.

1) Now that you changed the design to requiring the buildcost table to be empty instead of listing wares with amount 0, the dismantle button does not appear for such empty tables. This is easily fixed in ‘wui/buildingwindow.cc’ by removing a ware check, i.e., removing lines 287 and 297 (and unindenting the block inbetween). I already tested this, it works fine.

2) Having no items shown in the box when hovering over the dismantle button looks good enough for me, but I can imagine that some might find this not nice. Maybe adding a little text “nothing” or so might be good. And (at a later point) maybe even a little icon.

3) See diff comments.


Diff comments:

> 
> === modified file 'data/campaigns/emp04.wmf/scripting/tribes/farm1.lua'
> --- data/campaigns/emp04.wmf/scripting/tribes/farm1.lua	2018-09-09 17:58:55 +0000
> +++ data/campaigns/emp04.wmf/scripting/tribes/farm1.lua	2018-11-21 09:48:10 +0000
> @@ -10,10 +10,6 @@
>     enhancement = "empire_farm2",
>  
>     return_on_dismantle = {
> -      planks = 0,
> -      granite = 0,
> -      marble = 0,
> -      marble_column = 0
>     },

For farm1, return_on_dismantle should now be gone completely for the campaign to work properly. (The farm click check relies on having no dismantle button, and the text also says that the farms can't be dismantled.)

>  
>     animations = {
> 
> === modified file 'src/logic/map_objects/buildcost.cc'
> --- src/logic/map_objects/buildcost.cc	2018-04-07 16:59:00 +0000
> +++ src/logic/map_objects/buildcost.cc	2018-11-21 09:48:10 +0000
> @@ -37,32 +35,37 @@
>  Buildcost::Buildcost(std::unique_ptr<LuaTable> table, const Tribes& tribes)
>     : std::map<DescriptionIndex, uint8_t>() {
>  	for (const std::string& warename : table->keys<std::string>()) {
> -		int32_t value = INVALID_INDEX;
> -		try {
> -			DescriptionIndex const idx = tribes.safe_ware_index(warename);
> -			if (count(idx)) {
> -				throw GameDataError(
> -				   "A buildcost item of this ware type has already been defined: %s", warename.c_str());
> -			}
> -			value = table->get_int(warename);
> -			const uint8_t ware_count = value;
> -			if (ware_count != value) {
> -				throw GameDataError("Ware count is out of range 1 .. 255");
> -			}
> -			insert(std::pair<DescriptionIndex, uint8_t>(idx, ware_count));
> -		} catch (const WException& e) {
> -			throw GameDataError("[buildcost] \"%s=%d\": %s", warename.c_str(), value, e.what());
> -		}
> +		// Read ware index
> +		if (!tribes.ware_exists(warename)) {
> +			throw GameDataError("Buildcost: Unknown ware: %s", warename.c_str());
> +		}
> +		DescriptionIndex const idx = tribes.safe_ware_index(warename);
> +		if (count(idx) != 0) {

This condition is actually never true, because table->keys() returns a std::set, so even if a key appears multiple times in a lua table, it only occurs once in the set, so this loop also touches each key only once.

This is unrelated to this branch though, but we should probably fix this at some point. Maybe this should be fixed more generally anyway. (Not even sure if multiple occurrences of the same key make sense somewhere.) I guess I’ll make a proper bug report after thinking about it a bit more. But if you know a simple way to check this properly here, you can just add a fix here.

> +			throw GameDataError(
> +			   "Buildcost: Ware '%s' is listed twice", warename.c_str());
> +		}
> +
> +		// Read value
> +		int32_t value = table->get_int(warename);
> +		if (value < 1) {
> +			throw GameDataError("Buildcost: Ware count needs to be > 0 in \"%s=%d\".\nEmpty buildcost tables are allowed if you wish to have an amount of 0.", warename.c_str(), value);
> +		} else if (value > 255) {
> +			throw GameDataError("Buildcost: Ware count needs to be <= 255 0 in \"%s=%d\".", warename.c_str(), value);

1) The 0 after 255 should go away.
2) This throw actually crashes the whole application. The problem is the “<” in the text which - I assume - causes a crash in the rich text handling (attempt to interpret it as opening a tag) when trying to show the message box with the error. (Didn’t check this thoroughly yet, but simply deleting the “<” from the message avoids the crash. I’ll check this properly and make a bug report.)
For now, just replace the “<=” by “at most” or so.

> +		}
> +
> +		// Add
> +		insert(std::pair<DescriptionIndex, uint8_t>(idx, value));
>  	}
>  }
>  
>  /**
>   * Compute the total buildcost.
>   */
> -uint32_t Buildcost::total() const {
> -	uint32_t sum = 0;
> -	for (const_iterator it = begin(); it != end(); ++it)
> +Widelands::Quantity Buildcost::total() const {
> +	Widelands::Quantity sum = 0;
> +	for (const_iterator it = begin(); it != end(); ++it) {
>  		sum += it->second;
> +	}
>  	return sum;
>  }
>  
> 
> === modified file 'src/logic/map_objects/tribes/building.cc'
> --- src/logic/map_objects/tribes/building.cc	2018-11-19 21:38:03 +0000
> +++ src/logic/map_objects/tribes/building.cc	2018-11-21 09:48:10 +0000
> @@ -60,6 +60,7 @@
>     : MapObjectDescr(init_type, table.get_string("name"), init_descname, table),
>       egbase_(egbase),
>       buildable_(false),
> +     can_be_dismantled_(false),

Is there a particular reason we don't also default-initialize "destructible_" here?
It's not really necessary (we default-set it to "true" later in case no lua entry exists) but feels a bit odd.

>       size_(BaseImmovable::SMALL),
>       mine_(false),
>       port_(false),
> @@ -315,7 +316,7 @@
>  	const BuildingDescr& tmp_descr = descr();
>  	if (tmp_descr.is_destructible()) {
>  		caps |= PCap_Bulldoze;
> -		if (tmp_descr.is_buildable() || tmp_descr.is_enhanced())
> +		if (tmp_descr.can_be_dismantled())

{}

>  			caps |= PCap_Dismantle;
>  	}
>  	if (tmp_descr.enhancement() != INVALID_INDEX)


-- 
https://code.launchpad.net/~widelands-dev/widelands/empire04_unused_key_return_on_dismantle/+merge/358273
Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/empire04_unused_key_return_on_dismantle.