widelands-dev team mailing list archive

[Klaus Halfmann <klaus.halfmann@xxxxxxxx>]

Some comments:

* This depends or includes https://code.launchpad.net/~widelands-dev/widelands/bug-1690519-economy-unique_ptr ?

* As Serial is uint32_t so we have 2^32 different Economies, ok
* Please respond tho the inline comments, espcially using unordere_map should bet better.

This was a lot of change to digest, in general the code looks better this way.
Do we have a testcase with a lot of Economies to get an Idea of the Performance change?

I will now check the Attached Bugs, and check the code again how it fixes the issues.


Diff comments:

> 
> === modified file 'src/game_io/game_player_economies_packet.cc'
> --- src/game_io/game_player_economies_packet.cc	2018-04-07 16:59:00 +0000
> +++ src/game_io/game_player_economies_packet.cc	2018-05-13 06:04:57 +0000
> @@ -67,31 +67,21 @@
>  		FileRead fr;
>  		fr.open(fs, "binary/player_economies");
>  		uint16_t const packet_version = fr.unsigned_16();
> -		if (packet_version == 3 || packet_version == kCurrentPacketVersion) {
> +		if (packet_version == kCurrentPacketVersion) {
>  			iterate_players_existing(p, nr_players, game, player) try {
> -				// In packet_version 4 we dump the number of economies a player had at
> -				// save time to debug
> -				// https://bugs.launchpad.net/widelands/+bug/1654897 which is likely
> -				// caused by players having more economies at load than they had at
> -				// save.
> -				Player::Economies& economies = player->economies_;
> -				if (packet_version > 3) {
> -					const size_t num_economies = fr.unsigned_16();
> -					if (num_economies != economies.size()) {
> -						throw GameDataError("Num economies on save (%" PRIuS
> -						                    ") != Num economies on load (%" PRIuS ")",
> -						                    num_economies, economies.size());
> -					}
> -				}
> -
> -				for (uint32_t i = 0; i < economies.size(); ++i) {
> +				const size_t num_economies = fr.unsigned_32();
> +				for (uint32_t i = 0; i < num_economies; ++i) {
>  					uint32_t value = fr.unsigned_32();
>  					if (value < 0xffffffff) {

What kind of magic number is that 0xffffffff here?

>  						if (upcast(Flag const, flag, map[value].get_immovable())) {
> -							assert(flag->get_economy()->owner().player_number() ==
> -							       player->player_number());
> -							EconomyDataPacket d(flag->get_economy());
> -							d.read(fr);
> +							try {
> +								assert(flag->get_economy()->owner().player_number() ==
> +										 player->player_number());
> +								EconomyDataPacket d(flag->get_economy());
> +								d.read(fr);
> +							} catch (const GameDataError& e) {
> +								throw GameDataError("error reading economy data for flag at map index %d: %s", value, e.what());
> +							}
>  						} else {
>  							throw GameDataError("there is no flag at the specified location");
>  						}
> 
> === modified file 'src/logic/player.h'
> --- src/logic/player.h	2018-04-29 09:20:29 +0000
> +++ src/logic/player.h	2018-05-13 06:04:57 +0000
> @@ -515,18 +515,12 @@
>  	void enhance_building(Building*, DescriptionIndex index_of_new_building);
>  	void dismantle_building(Building*);
>  
> -	// Economy stuff
> -	void add_economy(Economy&);
> -	void remove_economy(Economy&);
> -	bool has_economy(Economy&) const;
> -	using Economies = std::vector<Economy*>;
> -	Economies::size_type get_economy_number(Economy const*) const;
> -	Economy* get_economy_by_number(Economies::size_type const i) const {
> -		return economies_[i];
> -	}
> -	uint32_t get_nr_economies() const {
> -		return economies_.size();
> -	}
> +	Economy* create_economy();
> +	Economy* create_economy(Serial serial); // For saveloading only
> +	void remove_economy(Serial serial);
> +	const std::map<Serial, std::unique_ptr<Economy>>& economies() const;

Why not unordered_map, I  do not think we need range iteration?

"unordered_map containers are faster than map containers to access individual elements by their key, although they are generally less efficient for range iteration through a subset of their elements."

> +	Economy* get_economy(Widelands::Serial serial) const;
> +	bool has_economy(Widelands::Serial serial) const;
>  
>  	uint32_t get_current_produced_statistics(uint8_t);
>  


-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1732765-economy-refactoring/+merge/345277
Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1732765-economy-refactoring into lp:widelands.