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
> +
> +++ src/logic/map_objects/tribes/worker_descr.cc 2018-04-15 06:18:59
> +
> @@ -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::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
> +
> +++ src/logic/map_objects/tribes/worker_descr.h 2018-04-15 06:18:59
> +
> @@ -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 employers_; // Buildings where ths worker
> can work
> -private:
> + const int32_t needed_experience_;
> +
> + /// Buildings where this worker can work
> + std::set 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.
___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help : https://help.launchpad.net/ListHelp
