Introduce new Syncing libraries #19

SamuelMoriarty · Dec 5, 2017

Each library comes with an extensive doc detailing the principles behind the library, the standard usage scenario as seen by the end user, and in the case of Network, an implementation overview to help you get oriented.

There are also a few minor changes in the other packages that I added during the development of this, that I thought deserved to be in the stdlib.

Frotty · Dec 6, 2017

Frotty reviewed Dec 6, 2017

View changes

Frotty · Dec 6, 2017

wurst/_handles/Trigger.wurst

@@ -1,5 +1,13 @@
 package Trigger
 import NoWurst
+import Player


The changes you made here overlap with stuff from RegisterEvents

Oh, thanks, did not realize it was there. Thought it'd be intuitive to have those extension functions for Trigger.

Frotty · Dec 6, 2017

wurst/closures/Execute.wurst

+public interface ForForceCallback
+	function callback()
+
+force dummy = CreateForce()


You can use bj_FORCE_ALL_PLAYERS instead of a dummy

Unfortunately, to execute the callback exactly once, there needs to be only one player inside the Force. I think using bj_FORCE_ALL_PLAYERS would cause it to execute multiple times instead.

use let or constant, and dumy isn't a suitable name

Frotty · Dec 6, 2017

wurst/network/SyncSimple.wurst

+
+// briefly selects the specified unit by the player
+function player.onceSelect(unit what)
+	let temp = CreateGroup()


use ENUM_GROUP and for from to not have group leaks

https://wurstscript.github.io/manual.html#loops

Thanks. Will do.

Frotty · Dec 6, 2017

wurst/_handles/Player.wurst

@@ -5,9 +5,11 @@ import Unit

 /** Use this array instead of Player() to avoid leaks */
 public player array players
+public player localPlayer


Can this be a constant?

Probably, thanks

Frotty · Dec 6, 2017

wurst/network/SyncSimple.wurst

+	private SynchronizationCallback callback = null
+
+	construct()
+		dummy = CreateUnit(DUMMY_PLAYER, DUMMY_ID, 0, 0, 0)


use cascade ..

https://wurstscript.github.io/manual.html#cascade-operator-dot-dot-operator

Frotty · Dec 6, 2017

wurst/network/Network.wurst

+	gamecache key lengths short.
+**/
+public class DataStream
+	// max characters to store per compacting round


Frotty · Dec 6, 2017

wurst/network/Network.wurst

+	construct(player sender)
+		this.currentState = NetworkState.PREPARING
+		this.sender = sender
+		this.gcIntegerStream = new GamecacheStream(dataCache, mkey, StreamType.INTEGER)


can be inlined?

Frotty · Dec 6, 2017

wurst/network/Network.wurst

+		let syncRounds = maxLength div gcKeysCount + 1
+
+		// save the amount of sync rounds required so that other players know how many times they need to read
+		if localPlayer == sender


This if and the next one can be merged

Frotty · Dec 6, 2017

wurst/network/Network.wurst

+	// receives info about the transmission after it has been sent by the originating player
+	private function receiveMetadata()
+		// check that we are transitioning from the correct state
+		if SAFETY_CHECKS_ENABLED and  currentState != NetworkState.SENDING_META


2 whitespace after and

killmeplease

Frotty · Dec 6, 2017

wurst/network/Network.wurst

+			// destroy this instance
+			destroy this
+
+	// this is the function to start sending all data in the intermediate dataStream buffer


make these comments hotdoc

Frotty · Dec 6, 2017

consider renaming **Stream classes as the name implies different functionality/implementation.
Thanks a lot 🌴

Cokemonkey11 · Dec 6, 2017

Cokemonkey11 reviewed Dec 6, 2017

View changes

Cokemonkey11 · Dec 6, 2017

wurst/_handles/GameCache.wurst

+	SyncStoredUnit(this, missionKey, key)
+
+public function gamecache.syncString(string missionKey, string key)
+	SyncStoredString(this, missionKey, key)


Missing EoF LF

Cokemonkey11 · Dec 6, 2017

Cokemonkey11 reviewed Dec 6, 2017

View changes

Cokemonkey11 · Dec 6, 2017

wurst/_handles/Player.wurst

 init
 	for i = 0 to bj_MAX_PLAYER_SLOTS-1
 		players[i] = Player(i)
+	localPlayer = GetLocalPlayer()


No need to have this in init block, I don't think?

If it is needed: needs a comment

Will inline into a constant

Cokemonkey11 · Dec 6, 2017

Cokemonkey11 reviewed Dec 6, 2017

View changes

Cokemonkey11 · Dec 6, 2017

wurst/closures/Execute.wurst

+force dummy = CreateForce()
+
+init
+	ForceAddPlayer(dummy, GetLocalPlayer())


Can this just be ..addPlayer(localPlayer)? If not, can you add _force API?

Thanks. Will add _force API as well.

Cokemonkey11 · Dec 6, 2017

Cokemonkey11 reviewed Dec 6, 2017

View changes

Cokemonkey11 · Dec 6, 2017

wurst/closures/Execute.wurst

+init
+	ForceAddPlayer(dummy, GetLocalPlayer())
+
+function callback() returns boolean


not a suitable function name for this internal fn

What name would be more suitable?

May I suggest _currentCallback? @Frotty ?

why underscore in front? the other libs (closureforgroups) use currentCallback which should be fine

That's fine too. Why underscore? Costs very little and benefit is slightly improved indication of "this is not public, and its use is very specific/controlled by this package"

Cokemonkey11 · Dec 6, 2017

Cokemonkey11 reviewed Dec 6, 2017

View changes

Cokemonkey11 · Dec 6, 2017

wurst/network/Network.wurst

+
+// max amount to be written/read/synced per an execution to avoid hitting the OP limit
+// this value can be higher if Wurst is compiled without stack traces and with all optimizations turned on
+// and even higher with safety checks off


The above two comment are of the form "optimiations, safety checks, and stack traces affect suitable op-limit values". I don't think they add much value here.

I think these are important considerations for those who may feel like they are experiencing performance issues, and are not necessarily obvious to everyone. Especially not obvious what this constant is for, and why you may want to adjust it, hence, the comment.

May I suggest

// Max amount to be written/read/synced per an execution, to avoid hitting the op-limit. // Beware of changes in maximum value as compiler settings are adjusted.

Cokemonkey11 · Dec 6, 2017

Cokemonkey11 reviewed Dec 6, 2017

View changes

Cokemonkey11 · Dec 6, 2017

wurst/network/Network.wurst

+// only disable this if you are 100% sure your code works correctly
+@configurable constant boolean SAFETY_CHECKS_ENABLED = true
+
+//TODO: I just realized that we can eliminate some delay by sending metadata and the first round together


Don't commit TODOs

Cokemonkey11 · Dec 6, 2017

Cokemonkey11 reviewed Dec 6, 2017

View changes

Cokemonkey11 · Dec 6, 2017

wurst/network/Network.wurst

+
+	Rationale:
+		This library can be used to send arbitrary amounts of integer, real, boolean
+		and string data from one player to the rest.


Why would I want to do that?

Read my other long comment ;)

May I suggest making all the documents clear on this unusual behavior?

"In multiplayer games, this package is for synchronizing data between game clients. It's useful for when one player is the source of game data, such as from gamecache or file IO."

Cokemonkey11 · Dec 6, 2017

Cokemonkey11 reviewed Dec 6, 2017

View changes

Cokemonkey11 · Dec 6, 2017

wurst/network/Network.wurst

+		and data is received from the same buffer inside the callback when it has all been received.
+
+		Read SyncSimple docs for a slightly more in-depth overview of this particular
+		peculiarity of WC3.


Cokemonkey11 · Dec 6, 2017

Cokemonkey11 reviewed Dec 6, 2017

View changes

Cokemonkey11 · Dec 6, 2017

wurst/network/Network.wurst

+		2. Queue some data for sending from the sender. You can write as much as you need, there is no limit:
+			if localPlayer == sender
+				// keep in mind that before you start sending data,
+				// network.getData() is locked in a WRITE-ONLY MODE


Why? What does that mean?

Hopefully makes much more sense with file example explanation.

Sorry, the purpose of my comment was to indicate that your code comment wasn't clear enough.

Rule of thumb: code comments explain why, not what.

Cokemonkey11 · Dec 6, 2017

Cokemonkey11 reviewed Dec 6, 2017

View changes

Cokemonkey11 · Dec 6, 2017

wurst/network/Network.wurst

+			if localPlayer == sender
+				// keep in mind that before you start sending data,
+				// network.getData() is locked in a WRITE-ONLY MODE
+				val stream = network.getData()


Thanks! Habit from writing Kotlin code ;)

Cokemonkey11 · Dec 6, 2017

Cokemonkey11 reviewed Dec 6, 2017

View changes

Cokemonkey11 · Dec 6, 2017

wurst/network/Network.wurst

+			network.start()			
+			// this will start the transmission
+			// keep in mind, that after you call this, you MUST NOT write/read to network.getData()
+			// as it is locked while the data is syncing


How is the stream processed at the receiver? Is there an Rx Queue?

"MUST NOT" - what happens if I try?

Frotty and I agreed that 'Stream' is a misnomer. Network is ultimately datagram based and sends the payload in smaller datagrams of gcKeysCount size.

If you try with SAFETY_CHECKS_ENABLED == true, you will simply receive an error. If you try without them, then it is undefined behaviour. You will probably just screw up the transmission.

Stream: why does that make it a misnomer? are you imagining that UDP streams offer no guarantee but datagram ones do, or are you saying that the presence of a sequence of frames does not fit the definition of stream?

In both cases I disagree. Stream is pretty abstract. See https://tokio.rs/docs/getting-started/streams-and-sinks/

About safety checks: consider rephrasing the sentence so "MUST NOT" doesn't exist.

The API should do its best to not let someone do something wrong

MUST NOT is an angry warning that will, on its own, fall upon deaf ears.

Cokemonkey11 · Dec 6, 2017

Cokemonkey11 reviewed Dec 6, 2017

View changes

Cokemonkey11 · Dec 6, 2017

wurst/network/Network.wurst

+			// as it is locked while the data is syncing
+
+		4. To receive the data, specify the callback and read the data inside it:
+			network.onFinish((stream) -> begin


Is this documentation for accepting Tx stream from another host, or a callback function for Tx finished its async operation?

'Stream' is a misnomore. This is a callback function for when the datagram has been fully received.

"datagram" you mean when full set (stream) of datagrams have all been received?

This needs serious clarification imo.

"4. Register a callback function to be run on all game clients when the Network sync is complete."

Cokemonkey11 · Dec 6, 2017

Cokemonkey11 reviewed Dec 6, 2017

View changes

Cokemonkey11 · Dec 6, 2017

wurst/network/Network.wurst

+				...
+			end)
+
+			WARNING: Network is a one-time use class, and is automatically


You mean: start() performs a destroy this?

destroy this is performed after the callback specified in .onFinish(cb) is called.

I'm wondering if this warning is necessary. Will wurst compiler prevent you from use-after-free? @Frotty

Cokemonkey11 · Dec 6, 2017

Cokemonkey11 reviewed Dec 6, 2017

View changes

Cokemonkey11 · Dec 6, 2017

wurst/network/Network.wurst

+				string field4 = ...
+
+				// optional stream-constructor
+				construct (DataStream stream)


is the space between construct and ( standard practice?

Whoops, my bad

Cokemonkey11 · Dec 6, 2017

Cokemonkey11 reviewed Dec 6, 2017

View changes

Cokemonkey11 · Dec 6, 2017

wurst/network/Network.wurst

+
+		And then use it like so:
+			// writing
+			val myClass = ...


Not clear. I guess you mean let myClass = new MyClass(...) ?

Yes. Will update the example to be more clear, thanks.

Cokemonkey11 · Dec 6, 2017

Cokemonkey11 reviewed Dec 6, 2017

View changes

Cokemonkey11 · Dec 6, 2017

wurst/network/Network.wurst

+		DataStream is a hashtable-based container with stream semantics for writing
+		integers, reals, booleans, strings and serializables.
+		Because we want to keep gamecache keys below a certain size, we can't store
+		everything immediately in the gamecache without risking exhausting available keys.


You mean that gamecache might have a collision? Source?

Network performance degrades significantly with longer gamecache keys, since keys (as well as gamecache names themselves) also get sent over the network. This has been demonstrated by TriggerHappy previously in his findings for the Sync library.

https://www.hiveworkshop.com/pastebin/3a4f7861cb884e21312168bd654330585801/

Is this documented in JassDoc? @lep ?

Code needs a direct reference if you're going to both (1) make claims about perf, and (2) they're actually relevant

Cokemonkey11 · Dec 6, 2017

Cokemonkey11 reviewed Dec 6, 2017

View changes

Overall really great code quality. Thanks, this is by far the biggest standard library PR I've seen in a long time - maybe ever.

Cokemonkey11 · Dec 6, 2017

wurst/network/Network.wurst

+		everything immediately in the gamecache without risking exhausting available keys.
+		We also need to know the size of data being sent prior to starting the transmission,
+		so we have to store all of it in an intermediate buffer, which is DataStream.
+		Prior to sending, all strings in the DataStream are 'exploded' into a stream


exploded -> encoded, packed, or unpacked

Cokemonkey11 · Dec 6, 2017

wurst/network/Network.wurst

+		We also need to know the size of data being sent prior to starting the transmission,
+		so we have to store all of it in an intermediate buffer, which is DataStream.
+		Prior to sending, all strings in the DataStream are 'exploded' into a stream
+		of integers, because SyncStoredString doesn't work.


The native is broken? Source?

Very old news. Source is TH's Sync library.

I tried this myself hoping it'd work on the newer patches somehow, but it simply doesn't sync strings correctly.
Might test this more in the future, but I have no reason to assume that it works.

yes, the stdlib's packages are broken as well with 1.28.3+ or so.

Code comment needs a source more than I do. Is this in jassdoc? @lep

Cokemonkey11 · Dec 6, 2017

wurst/network/Network.wurst

+		so we have to store all of it in an intermediate buffer, which is DataStream.
+		Prior to sending, all strings in the DataStream are 'exploded' into a stream
+		of integers, because SyncStoredString doesn't work.
+		After sending, they are 'compacted' back into strings and written to the DataStream.


compacted -> decode, packed, or unpacked

Cokemonkey11 · Dec 6, 2017

wurst/network/Network.wurst

+
+		GamecacheStream is a gamecache-based container with stream semantics for writing
+		integers, reals and booleans. There is a GamecacheStream for each primitive type,
+		integer, boolean, real and stringInts.


stringInt -> asciiInt

Cokemonkey11 · Dec 6, 2017

wurst/network/Network.wurst

+		all the heavy lifting.
+
+		Before starting the transmission, the DataStream is locked into a non-writable, non-readable
+		mode to prevent anyone from trying to mess with it while the transmission is going.


What happens when someone tries?

They will receive an error with safety checks enabled.

"the DataStream is locked into an immutable state, in which incorrect mutation attempts will print warnings, so long as safety checks are not disabled."

Cokemonkey11 · Dec 6, 2017

wurst/network/SyncSimple.wurst

+		synchronously for all players at the same time, allowing us to know for
+		certain when other players have acknowledged our unit selection, as well
+		as all network events that have been fired before it.
+		This is due to the fact that the WC3 game protocol is built on top of TCP,


Could be inferred, but background knowledge probably more noisy than value here

What do you mean?

This is due to the fact that the WC3 game protocol is built on top of TCP,

What's the actual value of explaining why [mathematical deterministic thing] works in [mathematically deterministic way]?

This is good background knowledge but not so much documentation.

Cokemonkey11 · Dec 6, 2017

wurst/network/SyncSimple.wurst

+		.sync() after a series of Sync natives, we ensure that the .onSynced()
+		callback will only be called after all players have received the data.
+
+		This way, we can send local data from one player to the rest, such as


Probably needs a concrete demo.

What about the usage section? It provides a valid usage case, which is also the operating principle behind Network.

By Concrete Demo I mean actual testmap. Here's an example of PR where testmap was warranted: wurstscript/WurstScript#472

Cokemonkey11 · Dec 6, 2017

wurst/network/SyncSimple.wurst

+
+// briefly selects the specified unit by the player
+function player.onceSelect(unit what)
+	let temp = CreateGroup()


https://wurstscript.github.io/manual.html#loops

Cokemonkey11 · Dec 6, 2017

wurst/network/SyncSimple.wurst

+
+	private unit dummy
+	private bitset syncedPlayers = emptyBitset()
+	private static bitset allPlayers = bitset(4095)


why 2^12 - 1 ?

2^12 - 1 is 1111 1111 1111 in binary, one bit for each player. neutrals don't count.

Needs a comment. Also would be nice if wurst had 0b1111_1111_1111 notation for ints @Frotty

Cokemonkey11 · Dec 6, 2017

wurst/network/SyncSimple.wurst

+	private SynchronizationCallback callback = null
+
+	construct()
+		dummy = CreateUnit(DUMMY_PLAYER, DUMMY_ID, 0, 0, 0)


https://wurstscript.github.io/manual.html#cascade-operator-dot-dot-operator

SamuelMoriarty · Dec 16, 2017

I've updated the PR, implementing most of the requests outlined above.
I've also done some refactoring in Network. Here's a short summary (off the top of my head)

Added Force API
Moved Buffer classes into wurst/data, added different implementations like HashBuffer and StringBuffer (to be used later for file I/O), also added unit tests for them
Split apart Network into multiple sub-packages: GamecacheBuffer, GamecacheKeys, Metadata, Network and StringEncoder
Metadata functionality has been externalized
String-encoding has been externalized
Network now sends first payload together with metadata, to avoid one redundant round-trip

Frotty · Dec 16, 2017

Frotty reviewed Dec 16, 2017

View changes

I'M still kinda asking myself what is the highest lvl api here and how will most users use this? Really nice work so far though :) A bit hefty to review though..

Frotty · Dec 16, 2017

wurst/_handles/Trigger.wurst

@@ -1,5 +1,6 @@
 package Trigger
 import NoWurst
+import Player


import can be removed?

Frotty · Dec 16, 2017

wurst/data/Buffer.wurst

+	private static constant MAX_BUFFER_SIZE = 1024
+
+	// list of "chunks" that can grow/shrink as required
+	private var chunks = new LinkedList<ChunkElement>()


this list doesn't get destroyed?

Frotty · Dec 16, 2017

wurst/data/Buffer.wurst

+
+	// loads the next chunk into the buffer
+	private function nextChunk()
+		if chunks.size() == 0


Frotty · Dec 16, 2017

wurst/data/Buffer.wurst

+			fail(BufferFailureMode.EOF, "reached EOF")
+			return
+
+		let chunk = chunks.dequeue()


using push and then dequeue kinda doesn't make sense. Please stick to sensible naming conventions of the intended datatype.

should I use .add()/.dequeue() pair instead?

Frotty · Dec 16, 2017

wurst/data/Buffer.wurst

+	override function writeBoolean(bool value)
+		checkWrite()
+		pushTypeIdentifier(ValueType.BOOLEAN)
+		if value


value.toString()

Frotty · Dec 16, 2017

wurst/network/Network.wurst

+	function getData() returns HashBuffer
+		return dataBuffer
+
+	// sends info about the amount of data to be expected to each player


Frotty · Dec 16, 2017

wurst/network/Network.wurst

+			end)
+
+			// encode all strings into ints
+			execute(() -> begin


doesn't need begin/end

Frotty · Dec 16, 2017

wurst/network/Network.wurst

+		let asciiIntCount = stringEncoder.getIntCount()
+
+		// calculate max length in each entity buffer
+		let maxLength = max(max(intCount, realCount), max(booleanCount, asciiIntCount))


should probably make a max(x,y,z) for maths package

Added min/max variants for up to 6 arguments in Maths package

Frotty · Dec 16, 2017

wurst/network/Network.wurst

+			destroy this
+
+	// this is the function to start sending all data in the intermediate dataBuffer buffer
+	function start()


can combine start and onFinish I think

Frotty · Dec 16, 2017

wurst/network/SyncSimple.wurst

+		2. Send some network events on one (or more) player using the Sync natives (or something else)
+			if sender == localPlayer
+				// value1 and value2 are some local values only known to localPlayer
+				SyncStoredInteger(gc, mkey, vkey1, value1)


use extension funcs

Frotty · Dec 18, 2017

Sweet 🌴

SamuelMoriarty force-pushed the SamuelMoriarty:master branch from 938205f to 56e49bb Dec 16, 2017

SamuelMoriarty force-pushed the SamuelMoriarty:master branch from 56e49bb to fc73756 Dec 16, 2017

Frotty merged commit 01a48ce into wurstscript:master Dec 18, 2017

wurstscript/WurstStdlib2

Join GitHub today

Introduce new Syncing libraries #19

Conversation

SamuelMoriarty commented Dec 5, 2017

Frotty reviewed Dec 6, 2017 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Frotty commented Dec 6, 2017 • edited

Cokemonkey11 reviewed Dec 6, 2017 View changes

This comment has been minimized.

This comment has been minimized.

Cokemonkey11 reviewed Dec 6, 2017 View changes

This comment has been minimized.

This comment has been minimized.

Cokemonkey11 reviewed Dec 6, 2017 View changes

This comment has been minimized.

This comment has been minimized.

Cokemonkey11 reviewed Dec 6, 2017 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Cokemonkey11 reviewed Dec 6, 2017 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Cokemonkey11 reviewed Dec 6, 2017 View changes

This comment has been minimized.

Cokemonkey11 reviewed Dec 6, 2017 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Cokemonkey11 reviewed Dec 6, 2017 View changes

This comment has been minimized.

Cokemonkey11 reviewed Dec 6, 2017 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Cokemonkey11 reviewed Dec 6, 2017 View changes

This comment has been minimized.

This comment has been minimized.

Cokemonkey11 reviewed Dec 6, 2017 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Cokemonkey11 reviewed Dec 6, 2017 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Cokemonkey11 reviewed Dec 6, 2017 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Cokemonkey11 reviewed Dec 6, 2017 View changes

This comment has been minimized.

This comment has been minimized.

Cokemonkey11 reviewed Dec 6, 2017 View changes

Frotty reviewed Dec 6, 2017

View changes

Frotty commented Dec 6, 2017 •

edited

Cokemonkey11 reviewed Dec 6, 2017

View changes

Cokemonkey11 reviewed Dec 6, 2017

View changes

Cokemonkey11 reviewed Dec 6, 2017

View changes

Cokemonkey11 reviewed Dec 6, 2017

View changes

Cokemonkey11 reviewed Dec 6, 2017

View changes

Cokemonkey11 reviewed Dec 6, 2017

View changes

Cokemonkey11 reviewed Dec 6, 2017

View changes

Cokemonkey11 reviewed Dec 6, 2017

View changes

Cokemonkey11 reviewed Dec 6, 2017

View changes

Cokemonkey11 reviewed Dec 6, 2017

View changes

Cokemonkey11 reviewed Dec 6, 2017

View changes

Cokemonkey11 reviewed Dec 6, 2017

View changes

Cokemonkey11 reviewed Dec 6, 2017

View changes

Cokemonkey11 reviewed Dec 6, 2017

View changes

Cokemonkey11 reviewed Dec 6, 2017

View changes

Cokemonkey11 reviewed Dec 6, 2017

View changes