widelands-dev team mailing list archive

[Notabilis <notabilis27@xxxxxx>]

Review: Approve diff, testing

If I am not mistaken, this mostly just moves code around, right?
The lost wares are back and the code is looking good so in my opinion this can go in.
If you want to: two small nits regarding the documentation.

Diff comments:

> 
> === modified file 'src/logic/map_objects/tribes/worker_descr.cc'
> --- src/logic/map_objects/tribes/worker_descr.cc	2018-04-07 16:59:00 +0000
> +++ src/logic/map_objects/tribes/worker_descr.cc	2018-04-15 06:18:59 +0000
> @@ -40,9 +40,12 @@
>                           const LuaTable& table,
>                           const EditorGameBase& egbase)
>     : BobDescr(init_descname, init_type, MapObjectDescr::OwnerType::kTribe, table),
> -     buildable_(false),
> -     needed_experience_(INVALID_INDEX),
> -     becomes_(INVALID_INDEX),
> +     ware_hotspot_(table.has_key("ware_hotspot") ? Vector2i(table.get_table("ware_hotspot")->get_int(1), table.get_table("ware_hotspot")->get_int(2)) : Vector2i(0, 15)),
> +	  default_target_quantity_(table.has_key("default_target_quantity") ? table.get_int("default_target_quantity") : std::numeric_limits<uint32_t>::max()),
> +     buildable_(table.has_key("buildcost")),
> +	  // Read what the worker can become and the needed experience. If one of the keys is there, the other key must be there too. So, we cross the checks.

I am assuming that the crossing is used so the game crashes / aborts when one of the keys is missing, right? Maybe add it to the comment.

> +     becomes_(table.has_key("experience") ? egbase.tribes().safe_worker_index(table.get_string("becomes")) : INVALID_INDEX),
> +	  needed_experience_(table.has_key("becomes") ? table.get_int("experience") : INVALID_INDEX),
>       egbase_(egbase) {
>  	if (helptext_script().empty()) {
>  		throw GameDataError("Worker %s has no helptext script", name().c_str());
> 
> === modified file 'src/logic/map_objects/tribes/worker_descr.h'
> --- src/logic/map_objects/tribes/worker_descr.h	2018-04-07 16:59:00 +0000
> +++ src/logic/map_objects/tribes/worker_descr.h	2018-04-15 06:18:59 +0000
> @@ -116,26 +117,34 @@
>  	}
>  
>  protected:
> -	Quantity default_target_quantity_;
> +	Programs programs_;
> +
> +private:
> +	const Vector2i ware_hotspot_;
> +
>  	DirAnimations walk_anims_;
>  	DirAnimations walkload_anims_;
> -	bool buildable_;
> +
> +	Quantity default_target_quantity_;
> +	const bool buildable_;
>  	Buildcost buildcost_;
>  
>  	/**
> +	 * Type that this worker can become, i.e. level up to (or null).

Instead of "null" I would prefer "NVALID_INDEX".

> +	 */
> +	const DescriptionIndex becomes_;
> +
> +	/**
>  	 * Number of experience points required for leveling up,
>  	 * or INVALID_INDEX if the worker cannot level up.
>  	 */
> -	int32_t needed_experience_;
> -
> -	/**
> -	 * Type that this worker can become, i.e. level up to (or null).
> -	 */
> -	DescriptionIndex becomes_;
> -	Programs programs_;
> -	std::set<DescriptionIndex> employers_;  // Buildings where ths worker can work
> -private:
> +	const int32_t needed_experience_;
> +
> +	/// Buildings where this worker can work
> +	std::set<DescriptionIndex> employers_;
> +
>  	const EditorGameBase& egbase_;
> +
>  	DISALLOW_COPY_AND_ASSIGN(WorkerDescr);
>  };
>  }


-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1721121-workers-invisible-wares/+merge/343272
Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1721121-workers-invisible-wares.