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.
_______________________________________________
Mailing list: https://launchpad.net/~widelands-dev
Post to : [email protected]
Unsubscribe : https://launchpad.net/~widelands-dev
More help : https://help.launchpad.net/ListHelp
