widelands-dev team mailing list archive

[SirVer <SirVer@xxxxxx>]

Review: Needs Fixing

serial numbers of objects are not saved. There are MapObject[Saver|Loader] classes that remember at which position an object is saved into the file. On load, the serial numbers are restarted from zero. 

The correct way of doing map object serialization is by stealing from MapObject::Loader and see how that is recreating pointer - and to just keep a pointer to the ships in the AI.

I do not agree with ship_id - I fail to see the use. Serializing of MapObjects is solved in the code base and this is a reinvention with less features. That is not necessary, IMHO.

The names of the ships are really a nice touch :)

Diff comments:

> === modified file 'src/ai/defaultai.cc'
> --- src/ai/defaultai.cc	2016-01-02 12:36:38 +0000
> +++ src/ai/defaultai.cc	2016-01-02 21:40:20 +0000
> @@ -880,9 +893,13 @@
>  
>  		// same defaults are directly saved to avoid inconsistency
>  		player_->set_ai_data(colony_scan_area_, kColonyScan);
> +		player_->set_ai_data(expedition_start_time_, kExpStartTime);

why is this not following the Packages concept that we have everywhere else? Especially, why is this not keeping track of a packet version number, so that we have a chance of backwards compatibility in the future?

> +		player_->set_ai_data(ships_utilization_, kShipUtil);
> +		player_->set_ai_data(no_more_expeditions_, kNoExpeditions);
>  		player_->set_ai_data(last_attacked_player_, kLastAttack);
>  		player_->set_ai_data(least_military_score_, kLeastMilit);
>  		player_->set_ai_data(target_military_score_, kTargetMilit);
> +		player_->set_ai_data(expedition_ship_, kExpShip);		
>  
>  	} else {
>  		log (" %d: restoring saved AI data...\n", player_number());
> @@ -3547,19 +3578,26 @@
>  		}
>  	}
>  
> +	assert (allships.size() >= expeditions_in_progress);
> +
>  	enum class FleetStatus : uint8_t {kNeedShip = 0, kEnoughShips = 1, kDoNothing = 2};
>  
>  	// now we must compare ports vs ships to decide if new ship is needed or new expedition can start
>  	FleetStatus enough_ships = FleetStatus::kDoNothing;
> -	if (static_cast<float>(allships.size()) >= ports_count) {
> +	if (shipyards_count == 0 || !idle_shipyard_stocked || ports_count == 0) {
> +		enough_ships = FleetStatus::kDoNothing;
> +	// We allways need at least one ship in transport mode

weird indent. to what does this comment belong? It seems it belongs to the else if { block. If so, I suggest putting it as first line into that block.

> +	} else if (allships.size() - expeditions_in_progress == 0) {
> +		enough_ships = FleetStatus::kNeedShip;
> +	// Otherwise depending on currents ships utilization
> +	} else if (ships_utilization_ > 5000) {
> +		enough_ships = FleetStatus::kNeedShip;
> +	} else {
>  		enough_ships = FleetStatus::kEnoughShips;
> -	} else if (static_cast<float>(allships.size()) < ports_count) {
> -		enough_ships = FleetStatus::kNeedShip;
>  	}
>  
>  	// building a ship? if yes, find a shipyard and order it to build a ship
> -	if (shipyards_count > 0 && enough_ships == FleetStatus::kNeedShip && idle_shipyard_stocked &&
> -	    ports_count > 0) {
> +	if (enough_ships == FleetStatus::kNeedShip) {
>  
>  		for (const ProductionSiteObserver& ps_obs : productionsites) {
>  			if (ps_obs.bo->is_shipyard_ && ps_obs.site->can_start_working() &&
> @@ -3608,9 +3649,72 @@
>  	bool action_taken = false;
>  
>  	if (!allships.empty()) {
> -		// iterating over ships and executing what is needed
> +		// iterating over ships and doing what is needed
>  		for (std::list<ShipObserver>::iterator i = allships.begin(); i != allships.end(); ++i) {
>  
> +			const uint8_t ship_state = i->ship->get_ship_state();

Pull out into a separate function? There is a lot of right-drifting in this code.

> +
> +			// Here we manage duration of expedition and related variables
> +			if (ship_state == Widelands::Ship::EXP_WAITING ||
> +			    ship_state == Widelands::Ship::EXP_SCOUTING ||
> +				ship_state == Widelands::Ship::EXP_FOUNDPORTSPACE) {
> +
> +					// consistency check
> +					assert (expedition_ship_ == i->ship->get_ship_id() || expedition_ship_ == kNoShip);
> +
> +					// This is obviously new expedition
> +					if (expedition_ship_ == kNoShip) {
> +						assert (expedition_start_time_ == kNoExpedition);
> +						expedition_start_time_ = gametime;
> +						player_->set_ai_data(gametime, kExpStartTime);
> +						expedition_ship_ = i->ship->get_ship_id();
> +						player_->set_ai_data(expedition_ship_, kExpShip);
> +
> +					// Already known expedition, all we do now, is decreasing colony_scan_area_
> +					// based on lapsed time
> +					} else if (gametime - expedition_start_time_ < kExpeditionMaxDuration) {
> +						assert (expedition_start_time_ > kNoExpedition);
> +						// remaining_time is a percent so in range 0-100
> +						const uint32_t remaining_time
> +							= 100 - ((gametime - expedition_start_time_) / (kExpeditionMaxDuration / 100));
> +						assert (remaining_time <= 100);
> +
> +						// We calculate expected value and actual value (colony_scan_area_
> +						// is changed only when needed)
> +						const uint32_t expected_colony_scan = kColonyScanMinArea
> +							+
> +							(kColonyScanStartArea - kColonyScanMinArea) * remaining_time / 100;
> +						assert (expected_colony_scan >= kColonyScanMinArea
> +							&&
> +							expected_colony_scan <= kColonyScanStartArea);
> +
> +						// So changing it if needed
> +						if (expected_colony_scan < colony_scan_area_) {
> +							colony_scan_area_ = expected_colony_scan;
> +							player_->set_ai_data(colony_scan_area_, kColonyScan);
> +						}
> +
> +					// Expedition overdue. Setting no_more_expeditions_=true
> +					// But we do not cancel it, the code for cancellation does not work properly now

add a TODO?

> +					} else if (gametime - expedition_start_time_ >= kExpeditionMaxDuration) {
> +						assert (expedition_start_time_ > 0);
> +						colony_scan_area_ = kColonyScanMinArea;
> +						player_->set_ai_data(kColonyScanMinArea, kColonyScan);
> +						no_more_expeditions_ = true;
> +						player_->set_ai_data(true, kNoExpeditions);
> +					}
> +
> +
> +			// We are not in expedition mode (or perhaps building a colonisation port)
> +			// so resetting start time
> +			} else if (expedition_ship_ == i->ship->get_ship_id()) {
> +				// Obviously expedition just ended
> +				expedition_start_time_ = kNoExpedition;
> +				expedition_ship_ = kNoShip;
> +				player_->set_ai_data(kNoExpedition, kExpStartTime);
> +				player_->set_ai_data(expedition_ship_, kExpShip);
> +			}
> +
>  			// only two states need an attention
>  			if ((i->ship->get_ship_state() == Widelands::Ship::EXP_WAITING ||
>  			     i->ship->get_ship_state() == Widelands::Ship::EXP_FOUNDPORTSPACE) &&
> @@ -3630,6 +3734,20 @@
>  				expedition_management(*i);
>  				action_taken = true;
>  			}
> +
> +			// Checking utilization
> +			if (i->ship->get_ship_state() == Widelands::Ship::TRANSPORT) {
> +

remove empty line?

> +				// Good utilization is 10 pieces of ware onboard, to track utilization we use range 0-10000
> +				// to avoid float or rounding errors if integers in range 0-100
> +				const int16_t tmp_util = (i->ship->get_nritems() > 10) ? 10000 : i->ship->get_nritems() * 1000;
> +				// This number is kind of average
> +				ships_utilization_ = ships_utilization_ / 20 * 19 + tmp_util / 20;

I think integer math is screwing you over here. For example:

ships_utilization_ = 18
ships_utilization_ / 20 * 19 == 0

is that what you wanted here?

> +				player_->set_ai_data(ships_utilization_, kShipUtil);
> +
> +				// Arithmetics check
> +				assert (ships_utilization_ >= 0 && ships_utilization_ <= 10000);
> +			}
>  		}
>  	}
>  
> @@ -4543,6 +4655,17 @@
>  	} else {
>  		allships.back().island_circ_direction = IslandExploreDirection::kCounterClockwise;
>  	}
> +
> +	if (type == NewShip::kBuilt) {
> +		marineTaskQueue_.push_back(kStopShipyard);

marine_task_queue

> +	} else {
> +		seafaring_economy = true;
> +		if (ship.state_is_expedition()) {
> +			assert (expedition_ship_ == kNoShip);
> +			expedition_ship_ = ship.get_ship_id();
> +			player_->set_ai_data(expedition_ship_, kExpShip);

you are reinventing the wheel here. If you create a AIGamePacket in game_io and call it from the player packet, you can serialize pointers to the ship and do not need set_ai_data.

> +		}
> +	}
>  }
>  
>  // this is called whenever we lose ownership of a PlayerImmovable
> 
> === modified file 'src/logic/player.cc'
> --- src/logic/player.cc	2015-12-12 20:47:04 +0000
> +++ src/logic/player.cc	2016-01-02 21:40:20 +0000
> @@ -1358,6 +1364,37 @@
>  	}
>  }
>  
> +uint32_t Player::next_ship_id (bool increase){

next_ship_id implies that it is always increased. the parameter is confusing. also keeping a ObjPtr to the ship is much preferable - less code and safer.

> +	if (increase) {
> +		return m_next_ship_id++;
> +	}
> +	return m_next_ship_id;
> +}
> +
> +// Mark shipname as used = remove particular index from m_remaining_shipname_indexes
> +void Player::set_shipname_used(uint32_t index) {
> +	for (auto it = m_remaining_shipname_indexes.begin(); it != m_remaining_shipname_indexes.end(); ++it) {
> +		if (*it == index) {
> +			m_remaining_shipname_indexes.erase(it);
> +			return;
> +		}
> +	}
> +	// Should never get here, but not a serious problem
> +	return;
> +}
> +
> +// Pick random index (=ship name) from m_remaining_shipname_indexes
> +uint32_t Player::pick_shipname_index(uint32_t gametime) {

this must use logic_rand() since it is user visible and persisted - otherwise there is the possibility that ships have different names in network games which might lead to desyncs.

> +	if (!m_remaining_shipname_indexes.empty()) {
> +		const uint32_t index = gametime % m_remaining_shipname_indexes.size();
> +		std::unordered_set<uint32_t>::iterator it = m_remaining_shipname_indexes.begin();
> +		std::advance(it, index);
> +		uint32_t name_index = *it;
> +		return name_index;
> +	}
> +	return std::numeric_limits<uint32_t>::max();
> +}
> +
>  /**
>   * Read statistics data from a file.
>   *
> 
> === modified file 'src/logic/ship.cc'
> --- src/logic/ship.cc	2015-11-21 11:34:10 +0000
> +++ src/logic/ship.cc	2016-01-02 21:40:20 +0000
> @@ -98,6 +98,13 @@
>  	Bob::init(egbase);
>  	init_fleet(egbase);
>  	Notifications::publish(NoteShipMessage(this, NoteShipMessage::Message::kGained));
> +	assert(get_owner());
> +
> +	// Assigning ship id and index of ship name (the ship name itself is not stored here)

why not simply store the name of the ship?

> +	m_id = get_owner()->next_ship_id(true);
> +	m_shipname_index = get_owner()->pick_shipname_index(egbase.get_gametime());
> +	get_owner()->set_shipname_used(m_shipname_index);
> +	molog("New ship: %s\n", get_owner()->tribe().get_shipname_by_index(m_shipname_index, m_id).c_str());
>  }
>  
>  /**
> 
> === modified file 'src/logic/ship.h'
> --- src/logic/ship.h	2015-11-04 16:48:56 +0000
> +++ src/logic/ship.h	2016-01-02 21:40:20 +0000
> @@ -153,6 +153,12 @@
>  	/// \returns the current state the ship is in
>  	uint8_t get_ship_state() {return m_ship_state;}
>  
> +	/// \returns the current id of ship
> +	uint32_t get_ship_id() {return m_id;}
> +
> +	/// \returns the current name of ship

I think this should return const string&

> +	uint32_t get_shipname_index() {return m_shipname_index;}
> +
>  	/// \returns whether the ship is currently on an expedition
>  	bool state_is_expedition() {
>  		return
> 
> === modified file 'src/wui/shipwindow.cc'
> --- src/wui/shipwindow.cc	2015-04-07 06:49:51 +0000
> +++ src/wui/shipwindow.cc	2016-01-02 21:40:20 +0000
> @@ -52,7 +52,7 @@
>   * Display information about a ship.
>   */
>  struct ShipWindow : UI::Window {
> -	ShipWindow(InteractiveGameBase & igb, Ship & ship);
> +	ShipWindow(InteractiveGameBase & igb, Ship & ship, std::string & title);

title should be const string&
const Ship& as UI should not change the object

>  	virtual ~ShipWindow();
>  
>  	void think() override;
> 
> === added file 'test/maps/ship_transportation.wmf/scripting/test_many_ships.lua'
> --- test/maps/ship_transportation.wmf/scripting/test_many_ships.lua	1970-01-01 00:00:00 +0000
> +++ test/maps/ship_transportation.wmf/scripting/test_many_ships.lua	2016-01-02 21:40:20 +0000
> @@ -0,0 +1,36 @@
> + 

this test is not asserting anything. What does it test? Could you add a top level comment to explain what it does?

> +run(function()
> +   sleep(100)
> +   game.desired_speed = 10 * 1000
> +
> +   -- used to test naming of ships
> +
> +   i = 0
> +   while i < 10 do
> +        p1:place_ship(map:get_field(10, 10))
> +        i = i + 1
> +        sleep(2000)                                                                                                                    
> +        end
> + 
> +  stable_save("10_ships")
> +   
> +   i = 0
> +   while i < 10 do
> +        p1:place_ship(map:get_field(10, 10))
> +        i = i + 1
> +        sleep(2000)
> +        end
> +
> +  stable_save("20_ships")
> +  
> +    i = 0
> +   while i < 10 do
> +        p1:place_ship(map:get_field(10, 10))
> +        i = i + 1
> +        sleep(2000)
> +        end 
> +        sleep(20000)
> +   
> +   print("# All Tests passed.")
> +   wl.ui.MapView():close()
> +end)
> 
> === modified file 'tribes/atlanteans.lua'
> --- tribes/atlanteans.lua	2015-11-03 18:18:27 +0000
> +++ tribes/atlanteans.lua	2016-01-02 21:40:20 +0000
> @@ -232,6 +232,67 @@
>        "dismantlesite",
>     },
>  
> +   ship_names = {

sort alphabetically. Also the others.

Very nice names :)

> +      "Coliondor",
> +      "Loftomor",
> +      "Opol",
> +      "Ostur",
> +      "Jundlina",
> +      "Sidolus",
> +      "King Askandor",
> +      "King Ajanthul",
> +      "Satul",
> +      "Atlantis",
> +      "Spider",
> +      "Atlantean's Stronghold",
> +      "Juventud",
> +      "Bahama",
> +      "Abaco",
> +      "Nassau",
> +      "Eleuthera",
> +      "Inagua",
> +      "Caicos",
> +      "Anguilla",
> +      "Kitts",
> +      "Nevis",
> +      "Antigua",
> +      "Barbuda",
> +      "Montserrat",
> +      "Guadelope",
> +      "Dominica",
> +      "Martinique", 
> +      "Grenada",
> +      "Barbados",
> +      "Trinidad",
> +      "Tobago",
> +      "Tortuga",
> +      "Blanquilla",
> +      "Orchila",
> +      "Agate",
> +      "Malachite",
> +      "Alexandrite",
> +      "Moonstone",
> +      "Amber",
> +      "Amethyst",
> +      "Mystic Quartz",
> +      "Topaz",
> +      "Obsidian",
> +      "Onyx",
> +      "Aquamarine",
> +      "Beryl",
> +      "Pearl",
> +      "Cassiterite",
> +      "Cat's Eye",
> +      "Tiger Eye",
> +      "Tourmaline",
> +      "Citrine",
> +      "Sapphire",
> +      "Emerald",
> +      "Sphalerite",
> +      "Spinel",
> +      "Sunstone",
> +   },
> +
>     -- Special types
>     builder = "atlanteans_builder",
>     carrier = "atlanteans_carrier",


-- 
https://code.launchpad.net/~widelands-dev/widelands/ai_ship_tweaks/+merge/280192
Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/ai_ship_tweaks.