widelands-dev team mailing list archive

[GunChleoc <fios@xxxxxxxxxxxxxxxxxxx>]

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.