Coe LGTM - added some comments with ideas and small code style fixes.
Diff comments:
>
> === modified file 'src/logic/map_objects/tribes/militarysite.cc'
> --- src/logic/map_objects/tribes/militarysite.cc 2017-06-06 05:26:58
> +0000
> +++ src/logic/map_objects/tribes/militarysite.cc 2017-06-19 07:06:15
> +0000
> @@ -43,6 +43,114 @@
>
> namespace Widelands {
>
> +// TODO(sirver): This method should probably return a const reference.
There is an element being erased in soldierlist.cc, so we can't do that yet.
> +std::vector<Soldier*> MilitarySite::SoldierControl::present_soldiers() const
> {
> + std::vector<Soldier*> soldiers;
> +
> + for (Worker* worker : military_site_->get_workers()) {
> + if (upcast(Soldier, soldier, worker)) {
> + if (military_site_->is_present(*soldier)) {
> + soldiers.push_back(soldier);
> + }
> + }
> + }
> + return soldiers;
> +}
> +
> +// TODO(sirver): This method should probably return a const reference.
> +std::vector<Soldier*> MilitarySite::SoldierControl::stationed_soldiers()
> const {
> + std::vector<Soldier*> soldiers;
> +
> + for (Worker* worker : military_site_->get_workers()) {
> + if (upcast(Soldier, soldier, worker)) {
> + soldiers.push_back(soldier);
> + }
> + }
> + return soldiers;
> +}
> +
> +Quantity MilitarySite::SoldierControl::min_soldier_capacity() const {
> + return 1;
> +}
> +
> +Quantity MilitarySite::SoldierControl::max_soldier_capacity() const {
> + return military_site_->descr().get_max_number_of_soldiers();
> +}
> +
> +Quantity MilitarySite::SoldierControl::soldier_capacity() const {
> + return military_site_->capacity_;
> +}
> +
> +void MilitarySite::SoldierControl::set_soldier_capacity(uint32_t const
> capacity) {
> + assert(min_soldier_capacity() <= capacity);
> + assert(capacity <= max_soldier_capacity());
> + assert(military_site_->capacity_ != capacity);
> + military_site_->capacity_ = capacity;
> + military_site_->update_soldier_request();
> +}
> +
> +void MilitarySite::SoldierControl::drop_soldier(Soldier& soldier) {
> + Game& game = dynamic_cast<Game&>(military_site_->owner().egbase());
> +
> + if (!military_site_->is_present(soldier)) {
> + // This can happen when the "drop soldier" player command is
> delayed
> + // by network delay or a client has bugs.
> + military_site_->molog("MilitarySite::drop_soldier(%u): not
> present\n", soldier.serial());
> + return;
> + }
> + if (present_soldiers().size() <= min_soldier_capacity()) {
> + military_site_->molog("cannot drop last soldier(s)\n");
> + return;
> + }
> +
> + soldier.reset_tasks(game);
> + soldier.start_task_leavebuilding(game, true);
> +
> + military_site_->update_soldier_request();
> +}
> +
> +int MilitarySite::SoldierControl::incorporate_soldier(EditorGameBase&
> egbase, Soldier& s) {
> + if (s.get_location(egbase) != military_site_) {
> + s.set_location(military_site_);
> + }
> +
> + // Soldier upgrade is done once the site is full. In soldier upgrade, we
> + // request one new soldier who is better suited than the existing ones.
> + // Normally, I kick out one existing soldier as soon as a new guy
> starts walking
> + // towards here. However, since that is done via infrequent polling, a
> new soldier
> + // can sometimes reach the site before such kick-out happens. In those
> cases, we
> + // should either drop one of the existing soldiers or reject the new
> guy, to
> + // avoid overstocking this site.
> +
> + if (stationed_soldiers().size() >
> military_site_->descr().get_max_number_of_soldiers()) {
> + return military_site_->incorporate_upgraded_soldier(egbase, s)
> ? 0 : -1;
> + }
> +
> + if (!military_site_->didconquer_) {
> + military_site_->conquer_area(egbase);
> + // Building is now occupied - idle animation should be played
> + military_site_->start_animation(egbase,
> military_site_->descr().get_animation("idle"));
> +
> + if (upcast(Game, game, &egbase)) {
> + military_site_->send_message(
> + *game, Message::Type::kEconomySiteOccupied,
> military_site_->descr().descname(),
> + military_site_->descr().icon_filename(),
> military_site_->descr().descname(),
> + military_site_->descr().occupied_str_, true);
> + }
> + }
> +
> + if (upcast(Game, game, &egbase)) {
> + // Bind the worker into this house, hide him on the map
> + s.reset_tasks(*game);
> + s.start_task_buildingwork(*game);
> + }
> +
> + // Make sure the request count is reduced or the request is deleted.
> + military_site_->update_soldier_request(true);
> +
> + return 0;
> +}
> +
> bool MilitarySite::AttackTarget::can_be_attacked() const {
> return military_site_->didconquer_;
> }
>
> === modified file 'src/logic/map_objects/tribes/soldier.cc'
> --- src/logic/map_objects/tribes/soldier.cc 2017-05-21 22:18:02 +0000
> +++ src/logic/map_objects/tribes/soldier.cc 2017-06-19 07:06:15 +0000
> @@ -865,18 +865,18 @@
> BaseImmovable* const newimm =
> game.map()[state.coords].get_immovable();
> upcast(MilitarySite, newsite, newimm);
> if (newsite && (&newsite->owner() == &owner())) {
> - if (upcast(SoldierControl, ctrl, newsite)) {
> - state.objvar1 = nullptr;
> - // We may also have our location
> destroyed in between
> - if (ctrl->stationed_soldiers().size() <
> ctrl->soldier_capacity() &&
> - (!location ||
> -
> location->base_flag().get_position() != newsite->base_flag().get_position()))
> {
> - molog("[attack] enemy belongs
> to us now, move in\n");
> - pop_task(game);
> - set_location(newsite);
> -
> newsite->update_soldier_request();
> - return schedule_act(game, 10);
> - }
> + const SoldierControl* soldier_control =
> newsite->soldier_control();
Can this be nullptr? Maybe add an assert here.
> + state.objvar1 = nullptr;
> + // We may also have our location destroyed in
> between
> + if
> (soldier_control->stationed_soldiers().size() <
> + soldier_control->soldier_capacity() &&
> + (!location ||
> + location->base_flag().get_position() !=
> newsite->base_flag().get_position())) {
> + molog("[attack] enemy belongs to us
> now, move in\n");
> + pop_task(game);
> + set_location(newsite);
> + newsite->update_soldier_request();
> + return schedule_act(game, 10);
> }
> }
> }
>
> === modified file 'src/logic/map_objects/tribes/trainingsite.cc'
> --- src/logic/map_objects/tribes/trainingsite.cc 2017-04-23 12:11:19
> +0000
> +++ src/logic/map_objects/tribes/trainingsite.cc 2017-06-19 07:06:15
> +0000
> @@ -178,6 +178,78 @@
> }
> }
>
> +std::vector<Soldier*> TrainingSite::SoldierControl::present_soldiers() const
> {
> + return training_site_->soldiers_;
> +}
> +
> +std::vector<Soldier*> TrainingSite::SoldierControl::stationed_soldiers()
> const {
> + return training_site_->soldiers_;
> +}
> +
> +Quantity TrainingSite::SoldierControl::min_soldier_capacity() const {
> + return 0;
> +}
> +Quantity TrainingSite::SoldierControl::max_soldier_capacity() const {
> + return training_site_->descr().get_max_number_of_soldiers();
> +}
> +Quantity TrainingSite::SoldierControl::soldier_capacity() const {
> + return training_site_->capacity_;
> +}
> +
> +void TrainingSite::SoldierControl::set_soldier_capacity(Quantity const
> capacity) {
We have the exact same code in Militarysite. Move to Building?
> + assert(min_soldier_capacity() <= capacity);
> + assert(capacity <= max_soldier_capacity());
> + assert(training_site_->capacity_ != capacity);
> + training_site_->capacity_ = capacity;
> + training_site_->update_soldier_request();
> +}
> +
> +/**
> + * Drop a given soldier.
> + *
> + * 'Dropping' means releasing the soldier from the site. The soldier then
> + * becomes available to the economy.
> + *
> + * \note This is called from player commands, so we need to verify that the
> + * soldier is actually stationed here, without breaking anything if he isn't.
> + */
> +void TrainingSite::SoldierControl::drop_soldier(Soldier& soldier) {
> + Game& game = dynamic_cast<Game&>(training_site_->owner().egbase());
> +
> + std::vector<Soldier*>::iterator it =
> std::find(training_site_->soldiers_.begin(), training_site_->soldiers_.end(),
> &soldier);
> + if (it == training_site_->soldiers_.end()) {
> +
> training_site_->molog("TrainingSite::SoldierControl::drop_soldier: soldier
> not in training site");
> + return;
> + }
> +
> + training_site_->soldiers_.erase(it);
> +
> + soldier.reset_tasks(game);
> + soldier.start_task_leavebuilding(game, true);
> +
> + // Schedule, so that we can call new soldiers on next act()
> + training_site_->schedule_act(game, 100);
> + Notifications::publish(NoteTrainingSiteSoldierTrained(training_site_,
> training_site_->get_owner()));
> +}
> +
> +int TrainingSite::SoldierControl::incorporate_soldier(EditorGameBase&
> egbase, Soldier& s) {
> + if (s.get_location(egbase) != training_site_) {
> + if (stationed_soldiers().size() + 1 >
> training_site_->descr().get_max_number_of_soldiers())
> + return -1;
> +
> + s.set_location(training_site_);
> + }
> +
> + // Bind the worker into this house, hide him on the map
> + if (upcast(Game, game, &egbase))
> + s.start_task_idle(*game, 0, -1);
> +
> + // Make sure the request count is reduced or the request is deleted.
> + training_site_->update_soldier_request();
> +
> + return 0;
> +}
> +
> /*
> =============================
>
>
> === modified file 'src/logic/map_objects/tribes/warehouse.cc'
> --- src/logic/map_objects/tribes/warehouse.cc 2017-05-20 22:42:49 +0000
> +++ src/logic/map_objects/tribes/warehouse.cc 2017-06-19 07:06:15 +0000
> @@ -303,21 +303,77 @@
> }
> }
>
> -/*
> -==============================
> -IMPLEMENTATION
> -==============================
> -*/
> +std::vector<Soldier*> Warehouse::SoldierControl::present_soldiers() const {
> + std::vector<Soldier*> rv;
> + DescriptionIndex const soldier_index =
> warehouse_->owner().tribe().soldier();
> + IncorporatedWorkers::const_iterator sidx =
> warehouse_->incorporated_workers_.find(soldier_index);
> +
> + if (sidx != warehouse_->incorporated_workers_.end()) {
> + const WorkerList& soldiers = sidx->second;
> + for (Worker* temp_soldier : soldiers) {
> + rv.push_back(static_cast<Soldier*>(temp_soldier));
> + }
> + }
> + return rv;
> +}
> +
> +std::vector<Soldier*> Warehouse::SoldierControl::stationed_soldiers() const {
> + return present_soldiers();
> +}
> +
> +Quantity Warehouse::SoldierControl::min_soldier_capacity() const {
> + return 0;
> +}
> +
> +Quantity Warehouse::SoldierControl::max_soldier_capacity() const {
> + return 4294967295U;
Use std::numeric_limits?
> +}
> +
> +Quantity Warehouse::SoldierControl::soldier_capacity() const {
> + return max_soldier_capacity();
> +}
> +
> +void Warehouse::SoldierControl::set_soldier_capacity(Quantity /* capacity
> */) {
> + throw wexception("Not implemented for a Warehouse!");
> +}
> +
> +void Warehouse::SoldierControl::drop_soldier(Soldier&) {
> + throw wexception("Not implemented for a Warehouse!");
> +}
> +
> +int Warehouse::SoldierControl::outcorporate_soldier(Soldier& soldier) {
> + DescriptionIndex const soldier_index =
> warehouse_->owner().tribe().soldier();
> + if (warehouse_->incorporated_workers_.count(soldier_index)) {
> + WorkerList& soldiers =
> warehouse_->incorporated_workers_[soldier_index];
> +
> + WorkerList::iterator i = std::find(soldiers.begin(),
> soldiers.end(), &soldier);
> +
> + soldiers.erase(i);
> + warehouse_->supply_->remove_workers(soldier_index, 1);
> + }
> +#ifndef NDEBUG
> + else
> + throw wexception("outcorporate_soldier: soldier not in this
> warehouse!");
> +#endif
> + return 0;
> +}
> +
> +int Warehouse::SoldierControl::incorporate_soldier(EditorGameBase& egbase,
> Soldier& soldier) {
> + warehouse_->incorporate_worker(egbase, &soldier);
> + return 0;
> +}
>
> Warehouse::Warehouse(const WarehouseDescr& warehouse_descr)
> : Building(warehouse_descr),
> attack_target_(this),
> + soldier_control_(this),
> supply_(new WarehouseSupply(this)),
> next_military_act_(0),
> portdock_(nullptr) {
> next_stock_remove_act_ = 0;
> cleanup_in_progress_ = false;
> set_attack_target(&attack_target_);
> + set_soldier_control(&soldier_control_);
> }
>
> Warehouse::~Warehouse() {
>
> === modified file 'src/logic/playercommand.cc'
> --- src/logic/playercommand.cc 2017-02-12 09:10:57 +0000
> +++ src/logic/playercommand.cc 2017-06-19 07:06:15 +0000
> @@ -1514,10 +1514,20 @@
> }
>
> void CmdChangeSoldierCapacity::execute(Game& game) {
> - if (upcast(Building, building, game.objects().get_object(serial)))
> - if (&building->owner() == game.get_player(sender()))
> - if (upcast(SoldierControl, ctrl, building))
> - ctrl->changeSoldierCapacity(val);
> + if (upcast(Building, building, game.objects().get_object(serial))) {
> + if (&building->owner() == game.get_player(sender()) &&
> + building->soldier_control() != nullptr) {
> + SoldierControl* soldier_control =
> building->mutable_soldier_control();
> + Widelands::Quantity const old_capacity =
> soldier_control->soldier_capacity();
> + Widelands::Quantity const new_capacity =
> + std::min(static_cast<Widelands::Quantity>(
> +
> std::max(static_cast<int32_t>(old_capacity) + val,
> +
> static_cast<int32_t>(soldier_control->min_soldier_capacity()))),
> + soldier_control->max_soldier_capacity());
> + if (old_capacity != new_capacity)
Add {} for code safety
> +
> soldier_control->set_soldier_capacity(new_capacity);
> + }
> + }
> }
>
> void CmdChangeSoldierCapacity::serialize(StreamWrite& ser) {
>
> === modified file 'src/wui/building_statistics_menu.cc'
> --- src/wui/building_statistics_menu.cc 2017-04-03 17:29:50 +0000
> +++ src/wui/building_statistics_menu.cc 2017-06-19 07:06:15 +0000
> @@ -422,8 +422,9 @@
> if
> (!stats_vector[last_building_index_].is_constructionsite) {
> if (upcast(MilitarySite, militarysite,
>
> map[stats_vector[last_building_index_].pos].get_immovable())) {
> - if
> (militarysite->stationed_soldiers().size() <
> -
> militarysite->soldier_capacity()) {
> + auto* soldier_control =
> militarysite->soldier_control();
assert(soldier_control != nullptr);
> + if
> (soldier_control->stationed_soldiers().size() <
> +
> soldier_control->soldier_capacity()) {
> found = true;
> break;
> }
--
https://code.launchpad.net/~widelands-dev/widelands/00_soldier_control/+merge/325902
Your team Widelands Developers is requested to review the proposed merge of
lp:~widelands-dev/widelands/00_soldier_control into lp:widelands.
_______________________________________________
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
