widelands-dev team mailing list archive

[Klaus Halfmann <klaus.halfmann@xxxxxxxx>]

Review is ok for me:
 * more const is never wrong
 * Did not grasp that string thing, though.

Will compile and testplay this now, perhaps fo my next youtube video?

Diff comments:

> 
> === modified file 'src/logic/player.cc'
> --- src/logic/player.cc	2018-07-12 04:41:20 +0000
> +++ src/logic/player.cc	2018-07-13 13:41:35 +0000
> @@ -1357,25 +1358,45 @@
>   *
>   * \param fr source stream
>   */
> -void Player::read_statistics(FileRead& fr) {
> +void Player::read_statistics(FileRead& fr, const uint16_t packet_version) {
>  	uint16_t nr_wares = fr.unsigned_16();
> -	uint16_t nr_entries = fr.unsigned_16();
> +	size_t nr_entries = fr.unsigned_16();
> +
> +	// Stats are saved as a single string to reduce number of hard disk write operations
> +	const auto parse_stats = [nr_entries](std::vector<std::vector<uint32_t>>* stats, const DescriptionIndex ware_index, const std::string& stats_string, const std::string& description) {
> +		if (!stats_string.empty()) {
> +			std::vector<std::string> stats_vector;
> +			boost::split(stats_vector, stats_string, boost::is_any_of("|"));
> +			if (stats_vector.size() != nr_entries) {
> +				throw GameDataError("wrong number of %s statistics - expected %" PRIuS " but got %" PRIuS, description.c_str(), nr_entries, stats_vector.size());

Hmm,  could we handle this more gracefully so the game can be loaded (with some loss perhaps)

> +			}
> +			for (size_t j = 0; j < nr_entries; ++j) {
> +				stats->at(ware_index)[j] = static_cast<unsigned int>(atoi(stats_vector.at(j).c_str()));
> +			}
> +		} else if (nr_entries > 0) {
> +			throw GameDataError("wrong number of %s statistics - expected %" PRIuS " but got 0", description.c_str(), nr_entries);
> +		}
> +	};
>  
>  	for (uint32_t i = 0; i < current_produced_statistics_.size(); ++i)
>  		ware_productions_[i].resize(nr_entries);
>  
>  	for (uint16_t i = 0; i < nr_wares; ++i) {
> -		std::string name = fr.c_string();
> -		DescriptionIndex idx = egbase().tribes().ware_index(name);
> +		const std::string name = fr.c_string();
> +		const DescriptionIndex idx = egbase().tribes().ware_index(name);
>  		if (!egbase().tribes().ware_exists(idx)) {
>  			log("Player %u statistics: unknown ware name %s", player_number(), name.c_str());
>  			continue;
>  		}
>  
>  		current_produced_statistics_[idx] = fr.unsigned_32();
> -
> -		for (uint32_t j = 0; j < nr_entries; ++j)
> -			ware_productions_[idx][j] = fr.unsigned_32();
> +		if (packet_version < 22) {

This should be kCurrentPacketVersion?

> +			for (uint32_t j = 0; j < nr_entries; ++j) {
> +				ware_productions_[idx][j] = fr.unsigned_32();
> +			}
> +		} else {
> +			parse_stats(&ware_productions_, idx, fr.c_string(), "produced");
> +		}
>  	}
>  
>  	// Read consumption statistics
> @@ -1439,39 +1470,52 @@
>  }
>  
>  /**
> - * Write statistics data to the give file
> + * Write statistics data to the given file

Please ann an example of the written string, it might be interesting to read this in the dumped fiel perhaps?

>   */
>  void Player::write_statistics(FileWrite& fw) const {
> +	// Save stats as a single string to reduce number of hard disk write operations
> +	const auto write_stats = [&fw](const std::vector<std::vector<uint32_t>>& stats, const DescriptionIndex ware_index) {
> +		std::ostringstream oss("");
> +		if (!stats[ware_index].empty()) {
> +			for (uint32_t i = 0; i < stats[ware_index].size() - 1; ++i) {
> +				oss << stats[ware_index][i] << "|";
> +			}
> +			oss << stats[ware_index][stats[ware_index].size() - 1];
> +		}
> +		fw.c_string(oss.str());
> +	};
> +
> +	const Tribes& tribes = egbase().tribes();
> +	const std::set<DescriptionIndex>& tribe_wares = tribe().wares();
> +	const size_t nr_wares = tribe_wares.size();
> +
>  	// Write produce statistics
> -	fw.unsigned_16(current_produced_statistics_.size());
> +	fw.unsigned_16(nr_wares);
>  	fw.unsigned_16(ware_productions_[0].size());
>  
> -	for (uint8_t i = 0; i < current_produced_statistics_.size(); ++i) {
> -		fw.c_string(egbase().tribes().get_ware_descr(i)->name());
> -		fw.unsigned_32(current_produced_statistics_[i]);
> -		for (uint32_t j = 0; j < ware_productions_[i].size(); ++j)
> -			fw.unsigned_32(ware_productions_[i][j]);
> +	for (const DescriptionIndex ware_index : tribe_wares) {
> +		fw.c_string(tribes.get_ware_descr(ware_index)->name());
> +		fw.unsigned_32(current_produced_statistics_[ware_index]);
> +		write_stats(ware_productions_, ware_index);
>  	}
>  
>  	// Write consume statistics
> -	fw.unsigned_16(current_consumed_statistics_.size());
> +	fw.unsigned_16(nr_wares);
>  	fw.unsigned_16(ware_consumptions_[0].size());
>  
> -	for (uint8_t i = 0; i < current_consumed_statistics_.size(); ++i) {
> -		fw.c_string(egbase().tribes().get_ware_descr(i)->name());
> -		fw.unsigned_32(current_consumed_statistics_[i]);
> -		for (uint32_t j = 0; j < ware_consumptions_[i].size(); ++j)
> -			fw.unsigned_32(ware_consumptions_[i][j]);
> +	for (const DescriptionIndex ware_index : tribe_wares) {
> +		fw.c_string(tribes.get_ware_descr(ware_index)->name());
> +		fw.unsigned_32(current_consumed_statistics_[ware_index]);
> +		write_stats(ware_consumptions_, ware_index);
>  	}
>  
>  	// Write stock statistics
> -	fw.unsigned_16(ware_stocks_.size());
> +	fw.unsigned_16(nr_wares);
>  	fw.unsigned_16(ware_stocks_[0].size());
>  
> -	for (uint8_t i = 0; i < ware_stocks_.size(); ++i) {
> -		fw.c_string(egbase().tribes().get_ware_descr(i)->name());
> -		for (uint32_t j = 0; j < ware_stocks_[i].size(); ++j)
> -			fw.unsigned_32(ware_stocks_[i][j]);
> +	for (const DescriptionIndex ware_index : tribe_wares) {
> +		fw.c_string(tribes.get_ware_descr(ware_index)->name());
> +		write_stats(ware_stocks_, ware_index);
>  	}
>  }
>  }
> 
> === modified file 'src/logic/save_handler.cc'
> --- src/logic/save_handler.cc	2018-04-07 16:59:00 +0000
> +++ src/logic/save_handler.cc	2018-07-13 13:41:35 +0000
> @@ -83,14 +83,11 @@
>   * @return true if game should be saved ad next think().

That typo was perhaps mine :-) ad -> at

>   */
>  bool SaveHandler::check_next_tick(Widelands::Game& game, uint32_t realtime) {
> -
>  	// Perhaps save is due now?
>  	if (autosave_interval_in_ms_ <= 0 || next_save_realtime_ > realtime) {
>  		return false;  // no autosave or not due, yet
>  	}
>  
> -	next_save_realtime_ = realtime + autosave_interval_in_ms_;
> -
>  	// check if game is paused (in any way)
>  	if (game.game_controller()->is_paused_or_zero_speed()) {
>  		return false;


-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1678987-save-handler/+merge/349096
Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1678987-save-handler into lp:widelands.