Thanks for the review. I answered some of your comments below and will push a merge-ready version later on.
Diff comments: > > === modified file 'src/logic/game.cc' > --- src/logic/game.cc 2018-12-13 07:24:01 +0000 > +++ src/logic/game.cc 2019-01-20 23:22:08 +0000 > @@ -603,6 +608,47 @@ > } > > /** > + * Switches to the next part of the syncstream excerpt. > + */ > +void Game::report_sync_request() { > + syncwrapper_.current_excerpt_id_ = (syncwrapper_.current_excerpt_id_ + > 1) % SyncWrapper::kExcerptSize; > + syncwrapper_.excerpts_buffer_[syncwrapper_.current_excerpt_id_].clear(); > +} > + > +/** > + * Triggers writing of syncstream excerpt and adds the playernumber of the > desynced player > + * to the stream. > + * Playernumber should be negative when called by network clients > + */ > +void Game::report_desync(int32_t playernumber) { > + std::string filename = syncwrapper_.dumpfname_; > + filename.replace(filename.length() - kSyncstreamExtension.length(), > kSyncstreamExtension.length(), kSyncstreamExcerptExtension); That function is also stripping the directory from the path, so I would have to add it again as well. Do you want me to do so? Using that function might be a bit easier to read, apart from that I don't see any advantage. > + std::unique_ptr<StreamWrite> file(g_fs->open_stream_write(filename)); > + assert(file != nullptr); > + // Write revision, branch and build type of this build to the file > + file->unsigned_32(build_id().length()); > + file->text(build_id()); > + file->unsigned_32(build_type().length()); > + file->text(build_type()); > + file->signed_32(playernumber); > + // Write our buffers to the file. Start with the oldest one > + const size_t i2 = (syncwrapper_.current_excerpt_id_ + 1) % > SyncWrapper::kExcerptSize; > + size_t i = i2; > + for (;;) { > + file->text(syncwrapper_.excerpts_buffer_[i]); > + syncwrapper_.excerpts_buffer_[i].clear(); > + i = (i + 1) % SyncWrapper::kExcerptSize; > + if (i == i2) { Right... I totally forgot about this loop type, thanks! > + break; > + } > + } > + file->unsigned_8(Syncstream::Desync); > + file->signed_32(playernumber); > + // Restart buffers > + syncwrapper_.current_excerpt_id_ = 0; > +} > + > +/** > * Calculate the current synchronization checksum and copy > * it into the given array, without affecting the subsequent > * checksumming process. > > === modified file 'src/logic/game.h' > --- src/logic/game.h 2018-12-13 07:24:01 +0000 > +++ src/logic/game.h 2019-01-20 23:22:08 +0000 > @@ -63,6 +63,51 @@ > gs_ending > }; > > +// The entry types that are written to the syncstream > +// The IDs are a number in the higher 4 bits and the length in bytes in the > lower 4 bits > +namespace Syncstream { > + // game.cc Game::report_desync() I would like to have an enum, but that would result in explicit static_cast's each time one of these values is written to the syncstream, so I decided against it. Do you want me to change it? > + // s32 id of desynced user, -1 when written on client > + constexpr uint8_t Desync = 0x14; > + // map_object.cc CmdDestroyMapObject::execute() > + // u32 object serial > + constexpr uint8_t DestroyObject = 0x24; > + // economy.cc Economy::process_requests() > + // u8 request type > + // u8 request index > + // u32 target serial > + constexpr uint8_t ProcessRequests = 0x36; > + // economy.cc Economy::handle_active_supplies() > + // u32 assignments size > + constexpr uint8_t HandleActiveSupplies = 0x44; > + // request.cc Request::start_transfer() > + // u32 target serial > + // u32 source(?) serial > + constexpr uint8_t StartTransfer = 0x58; > + // cmd_queue.cc CmdQueue::run_queue() > + // u32 duetime > + // u32 command id > + constexpr uint8_t RunQueue = 0x68; > + // game.h Game::logic_rand_seed() > + // u32 random seed > + constexpr uint8_t RandomSeed = 0x74; > + // game.cc Game::logic_rand() > + // u32 random value > + constexpr uint8_t Random = 0x84; > + // map_object.cc CmdAct::execute() > + // u32 object serial > + constexpr uint8_t CmdAct = 0x94; > + // battle.cc Battle::Battle() > + // u32 first soldier serial > + // u32 second soldier serial > + constexpr uint8_t Battle = 0xA8; > + // bob.cc Bob::set_position() > + // u32 bob serial > + // s16 position x > + // s16 position y > + constexpr uint8_t BobSetPosition = 0xB8; > +} > + > class Player; > class MapLoader; > class PlayerCommand; -- https://code.launchpad.net/~widelands-dev/widelands/bug-better-syncstreams/+merge/361922 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-better-syncstreams into lp:widelands. _______________________________________________ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp