Merge lp:~widelands-dev/widelands/bug-1721121-workers-invisible-wares into lp:widelands

[GunChleoc]

Restored ware hotspot and animation to workers. Made some variables in WorkerDescr private and/or const.

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 3376. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/366705865.
Appveyor build 3182. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1721121_workers_invisible_wares-3182.

[Notabilis (notabilis27)]

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.

review: Approve (diff, testing)

[GunChleoc (gunchleoc)]

Thanks for the review - fixes applied :)

@bunnybot merge

=== modified file 'src/logic/map_objects/tribes/carrier.cc'
--- src/logic/map_objects/tribes/carrier.cc	2018-04-07 16:59:00 +0000
+++ src/logic/map_objects/tribes/carrier.cc	2018-05-05 09:26:43 +0000
@@ -549,32 +549,10 @@
 	fw.signed_32(promised_pickup_to_);
 }
 
-void Carrier::draw_inner(const EditorGameBase& game,
-                         const Vector2f& point_on_dst,
-                         const float scale,
-                         RenderTarget* dst) const {
-	assert(get_owner() != nullptr);
-	Worker::draw_inner(game, point_on_dst, scale, dst);
-
-	if (WareInstance const* const carried_ware = get_carried_ware(game)) {
-		const RGBColor& player_color = get_owner()->get_playercolor();
-		const Vector2f hotspot = descr().get_ware_hotspot().cast<float>();
-		const Vector2f location(
-		   point_on_dst.x - hotspot.x * scale, point_on_dst.y - hotspot.y * scale);
-		dst->blit_animation(
-		   location, scale, carried_ware->descr().get_animation("idle"), 0, player_color);
-	}
-}
-
 CarrierDescr::CarrierDescr(const std::string& init_descname,
                            const LuaTable& table,
                            const EditorGameBase& egbase)
-   : WorkerDescr(init_descname, MapObjectType::CARRIER, table, egbase),
-     ware_hotspot_(Vector2i(0, 15)) {
-	if (table.has_key("ware_hotspot")) {
-		std::unique_ptr<LuaTable> items_table = table.get_table("ware_hotspot");
-		ware_hotspot_ = Vector2i(items_table->get_int(1), items_table->get_int(2));
-	}
+   : WorkerDescr(init_descname, MapObjectType::CARRIER, table, egbase) {
 }
 
 /**

=== modified file 'src/logic/map_objects/tribes/carrier.h'
--- src/logic/map_objects/tribes/carrier.h	2018-04-07 16:59:00 +0000
+++ src/logic/map_objects/tribes/carrier.h	2018-05-05 09:26:43 +0000
@@ -33,16 +33,10 @@
 	~CarrierDescr() override {
 	}
 
-	Vector2i get_ware_hotspot() const {
-		return ware_hotspot_;
-	}
-
 protected:
 	Bob& create_object() const override;
 
 private:
-	Vector2i ware_hotspot_;
-
 	DISALLOW_COPY_AND_ASSIGN(CarrierDescr);
 };
 
@@ -71,12 +65,6 @@
 
 	static Task const taskRoad;
 
-protected:
-	void draw_inner(const EditorGameBase& game,
-	                const Vector2f& point_on_dst,
-	                const float scale,
-	                RenderTarget* dst) const override;
-
 private:
 	void find_pending_ware(Game&);
 	int32_t find_closest_flag(Game&);

=== modified file 'src/logic/map_objects/tribes/worker.cc'
--- src/logic/map_objects/tribes/worker.cc	2018-04-07 16:59:00 +0000
+++ src/logic/map_objects/tribes/worker.cc	2018-05-05 09:26:43 +0000
@@ -2999,6 +2999,14 @@
 
 	dst->blit_animation(
 	   point_on_dst, scale, get_current_anim(), game.get_gametime() - get_animstart(), player_color);
+
+	if (WareInstance const* const carried_ware = get_carried_ware(game)) {
+		const Vector2f hotspot = descr().ware_hotspot().cast<float>();
+		const Vector2f location(
+		   point_on_dst.x - hotspot.x * scale, point_on_dst.y - hotspot.y * scale);
+		dst->blit_animation(
+		   location, scale, carried_ware->descr().get_animation("idle"), 0, player_color);
+	}
 }
 
 /**

=== 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-05-05 09:26:43 +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 to trigger an exception if this is violated.
+     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());
@@ -54,7 +57,6 @@
 	std::unique_ptr<LuaTable> items_table;
 
 	if (table.has_key("buildcost")) {
-		buildable_ = true;
 		const Tribes& tribes = egbase_.tribes();
 		items_table = table.get_table("buildcost");
 		for (const std::string& key : items_table->keys<std::string>()) {
@@ -90,12 +92,6 @@
 		add_directional_animation(&walkload_anims_, "walkload");
 	}
 
-	// Read what the worker can become and the needed experience
-	if (table.has_key("becomes")) {
-		becomes_ = egbase_.tribes().safe_worker_index(table.get_string("becomes"));
-		needed_experience_ = table.get_int("experience");
-	}
-
 	// Read programs
 	if (table.has_key("programs")) {
 		std::unique_ptr<LuaTable> programs_table = table.get_table("programs");
@@ -114,11 +110,6 @@
 			}
 		}
 	}
-	if (table.has_key("default_target_quantity")) {
-		default_target_quantity_ = table.get_int("default_target_quantity");
-	} else {
-		default_target_quantity_ = std::numeric_limits<uint32_t>::max();
-	}
 }
 
 WorkerDescr::WorkerDescr(const std::string& init_descname,

=== 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-05-05 09:26:43 +0000
@@ -62,6 +62,10 @@
 		return buildcost_;
 	}
 
+	const Vector2i& ware_hotspot() const {
+		return ware_hotspot_;
+	}
+
 	/// How much of the worker type that an economy should store in warehouses.
 	/// The special value std::numeric_limits<uint32_t>::max() means that the
 	/// the target quantity of this ware type will never be checked and should
@@ -82,9 +86,6 @@
 			default_target_quantity_ = 1;
 	}
 
-	const DirAnimations& get_walk_anims() const {
-		return walk_anims_;
-	}
 	const DirAnimations& get_right_walk_anims(bool const carries_ware) const {
 		return carries_ware ? walkload_anims_ : walk_anims_;
 	}
@@ -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 INVALID_INDEX if the worker cannot level up.
+	 */
+	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);
 };
 }