widelands-dev team mailing list archive

[GunChleoc <fios@xxxxxxxxxxxxxxxxxxx>]

I think this is an excellent idea, so I've done a preliminary code review. Let me know when you are finished for a final review.

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);

You could use FileSystem::filename_without_ext here and then concatenate the new extension

> +	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) {

Code would be shorter like this:

do {
...
} while i != i2;

> +			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 think it might be more readable if we bundled these into an enum class

> +	// 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;
> 
> === added file 'utils/syncstream/syncexcerpt-to-text.py'
> --- utils/syncstream/syncexcerpt-to-text.py	1970-01-01 00:00:00 +0000
> +++ utils/syncstream/syncexcerpt-to-text.py	2019-01-20 23:22:08 +0000
> @@ -0,0 +1,99 @@
> +#!/usr/bin/env python3
> +
> +import struct
> +import sys
> +
> +# WARNING!!
> +# Keep this file in sync with namespace Syncstream in src/logic/game.h

Add a comment to game.h as well

> +
> +def handle_1(f):
> +	print("Desync:")
> +	# Read 4 bytes and interprete them as an signed 32 bit integer
> +	print("  Desynced player:", struct.unpack('<i', f.read(4))[0])
> +
> +def handle_2(f):
> +	print("DestroyObject:")
> +	print("  Serial:", struct.unpack('<I', f.read(4))[0])
> +
> +def handle_3(f):
> +	print("ProcessRequests:")
> +	print("  Type:", struct.unpack('<B', f.read(1))[0])
> +	print("  Index:", struct.unpack('<B', f.read(1))[0])
> +	print("  Serial:", struct.unpack('<I', f.read(4))[0])
> +
> +def handle_4(f):
> +	print("HandleActiveSupplies:")
> +	print("  Size:", struct.unpack('<I', f.read(4))[0])
> +
> +def handle_5(f):
> +	print("StartTransfer:")
> +	print("  Target serial:", struct.unpack('<I', f.read(4))[0])
> +	print("  Source serial:", struct.unpack('<I', f.read(4))[0])
> +
> +def handle_6(f):
> +	print("RunQueue:")
> +	print("  Duetime:", struct.unpack('<I', f.read(4))[0])
> +	print("  Command:", struct.unpack('<I', f.read(4))[0])
> +
> +def handle_7(f):
> +	print("RandomSeed:")
> +	print("  Seed:", struct.unpack('<I', f.read(4))[0])
> +
> +def handle_8(f):
> +	print("Random:")
> +	print("  Number:", struct.unpack('<I', f.read(4))[0])
> +
> +def handle_9(f):
> +	print("CmdAct:")
> +	print("  Serial:", struct.unpack('<I', f.read(4))[0])
> +
> +def handle_A(f):
> +	print("Battle:")
> +	print("  Serial first soldier:", struct.unpack('<I', f.read(4))[0])
> +	print("  Serial second soldier:", struct.unpack('<I', f.read(4))[0])
> +
> +def handle_B(f):
> +	print("BobSetPosition:")
> +	print("  Serial:", struct.unpack('<I', f.read(4))[0])
> +	print("  Position X:", struct.unpack('<h', f.read(2))[0])
> +	print("  Position y:", struct.unpack('<h', f.read(2))[0])
> +
> +handlers = {
> +		1: handle_1,
> +		2: handle_2,
> +		3: handle_3,
> +		4: handle_4,
> +		5: handle_5,
> +		6: handle_6,
> +		7: handle_7,
> +		8: handle_8,
> +		9: handle_9,
> +		10: handle_A,
> +		11: handle_B
> +	}
> +
> +
> +with open(sys.argv[1], "rb") as f:
> +	print("Created with %s(%s)"
> +		% (str(f.read(struct.unpack('<I', f.read(4))[0]).decode()),
> +		   str(f.read(struct.unpack('<I', f.read(4))[0]).decode())),
> +		end='')
> +	playernumber = struct.unpack('<i', f.read(4))[0]
> +	if playernumber < 0:
> +		print(" as client")
> +	else:
> +		print(" as host, desynced client = %i" % playernumber)
> +	byte = f.read(1)
> +	while byte:
> +		# Parse byte
> +		b = ord(byte)
> +		cmd = b >> 4
> +		length = b & 0x0F
> +		if cmd in handlers:
> +			handlers[cmd](f)
> +		else:
> +			# Unknown command. Skip as many bytes as the entry is long
> +			print("Warning: Unknown command code '%s', skipping %i bytes" % (cmd, length))
> +			for i in range(0, length):
> +				byte = f.read(1)
> +		byte = f.read(1)


-- 
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.