Network simulator settings are ignored

[dbechrd]

I've noticed a number of places in the tests, such as this:

yojimbo/test.cpp

Lines 1454 to 1464 in c868d55

	void CreateClients( int numClients, Client ** clients, const Address & address, const ClientServerConfig & config, Adapter & _adapter, double time )
	{
	for ( int i = 0; i < numClients; ++i )
	{
	clients[i] = YOJIMBO_NEW( GetDefaultAllocator(), Client, GetDefaultAllocator(), address, config, _adapter, time );
	clients[i]->SetLatency( 250 );
	clients[i]->SetJitter( 100 );
	clients[i]->SetPacketLoss( 25 );
	clients[i]->SetDuplicates( 25 );
	}
	}

Where we're calling client->SetLatency() etc. on a newly allocated client. If you look at the implementation for this setter, it doesn't do anything when networkSimulator is null:

yojimbo/source/yojimbo_base_client.cpp

Lines 91 to 97 in c868d55

	void BaseClient::SetLatency( float milliseconds )
	{
	if ( m_networkSimulator )
	{
	m_networkSimulator->SetLatency( milliseconds );
	}
	}

I also noticed this happening in the soak test:

yojimbo/soak.cpp

Lines 75 to 82 in c868d55

	Client client( GetDefaultAllocator(), Address("0.0.0.0"), config, adapter, time );
	client.SetLatency( 1000.0f );
	client.SetJitter( 100.0f );
	client.SetPacketLoss( 25.0f );
	client.SetDuplicates( 25.0f );
	client.InsecureConnect( privateKey, clientId, serverAddress );

These set calls should all be moved to after the InsecureConnect() call, because that's when the networkSimulator gets created (indirectly, via CreateInternal():

yojimbo/source/yojimbo_base_client.cpp

Lines 143 to 146 in c868d55

	if ( m_config.networkSimulator )
	{
	m_networkSimulator = YOJIMBO_NEW( *m_clientAllocator, NetworkSimulator, *m_clientAllocator, m_config.maxSimulatorPackets, m_time );
	}

This maybe happens with some server instances, too; I don't remember. I'm not going to submit a PR for this as I don't remember all the places it happens, and don't have the energy to audit it right now, but I wanted to document the problem in an issue.

This should be audited properly at some point by searching for all calls to those setters and ensuring they're happening on a valid networkSimulator instance. Alternatively, maybe the setters should just assert() when networkSimulator is null instead of silently ignore the parameters, so that it's not possible to call the setters when the networkSimulator doesn't exist, because who would ever want that anyways? The assert would make it very easy for users to notice when they messed up, vs. the network simulator not having the latency/jitter you expect, which is very difficult to notice since the tests still pass. 🤷

[gafferongames]

Add an assert at the top to check that the network simulator is non-NULL before the if check. Then fix all the tests so they pass, if necessary, move the network simulator allocation earlier -- thanks

[gafferongames]

I'm OK with any attempt to set things on the network simulator asserting out, because it's better than failing silently in debug....

[dbechrd]

I already fixed this in my local copy, but it's in a different programming language (I'm porting Yojimbo). I don't have the C++ version building locally, so all of my PRs have been done through Github UI, and it's not trivial for me to fix this from the web since it affects a lot of different files. I probably won't get around to it, but I completely agree with your assessment (adding the assert and re-ordering is what I did in my version).

[gafferongames]

What language are you porting it to?

[dbechrd]

What language are you porting it to?

I just finished (mostly) doing a native port of Yojimbo (and Serialize/Reliable/Netcode) to Jai.

I made a few changes as I went, but it's mostly on par:

• I removed the per-client memory allocators temporarily, as Jai has a completely different way to do allocators that I will probably try to use later.
• I tried to remove the reference counting from the Message class, as it's quite error-prone in Jai (which doesn't have destructors), but I had to add it back to get the tests to pass. I think it may be causing a memory leak in certain rare scenarios (e.g. when the sequence buffer overflows in the tests), but I haven't been able to find it, so I don't know if that bug exists in the C++ version or not. I will probably try to remove/improve it again later.
• I stubbed out libsodium for now, to avoid the dependency while porting, but I will probably do a native port of the few functions in libsodium that Netcode relies on (ChaCha20-Poly1305, its extended variant, and the secure random bytes code); not because it's necessarily a good idea, but for fun.
• All the MessageFactory macros were removed completely. My MessageFactory is just an array of create/serialize function pointer pairs. Jai's macro system is completely different from C/++, so it wasn't possible to port it directly. The new version still needs some improvements, but it works.
• I removed a bunch of redundant functionality (e.g. why are there two different Address implementations, and two different SequenceBuffer implementations, and two different Queue implementations, etc.?). I also rewrote the Address class entirely to remove all system dependencies, and improve error reporting when the parse fails, and published it as a standalone Jai module (https://github.com/dbechrd/jai-ip-address).

Overall, it was a joy to read your code. Your test coverage is phenomenal and the port would have failed catastrophically if it weren't for the tests. I fixed at least 50 bugs in my implementation while porting the tests. I will say that the uber-C++-ified Yojimbo was the least fun part of the library to work on, and was quite difficult to understand since it's spread across tens of files and uses tons of inheritance/virtual/macros. The C libraries were extremely trivial to port in comparison, and felt much more straightforward and consistent. It would be cool to see a C version of Yojimbo one day.

I mostly did this project to learn Jai, and to better understand the internals of Yojmibo et al (which I was already using in an unreleased game written in C++). I don't plan to keep my port up-to-date with the main branch, and even if I publish it, I would still recommend people just generate bindings instead of using a point-in-time native port. I honestly didn't realize when I started this how often you're still updating the libraries (which is awesome!). I'm also not sure if my protocol is compatible with the C++ version, because I haven't implemented encryption yet so I can't test it. My original plan was to port Yojimbo, then use what i learned to refactor it heavily, perhaps into something else entirely, but I dunno what will come of that. I may also try to make a network traffic visualizer to provide more insight into how it works in a visual/interactive way. I'll let you know if anything cool comes of it.

[gafferongames]

Hey that's awesome! Well done!

[gafferongames]

ps. I agree about the C version, the main reason is because of the serialize stuff using the C++ templates for unified serialize, and how that is used throughout the library in message serialization. I think if messages were reduced to byte buffers and sent that way instead, a C port would be a great idea. Planning on something like this in the future, as a separate library.