PlayerNetID helper

[Brett208]

I'm not great at bit manipulation, so probably bears a fair amount of scrutiny.

[DanRStevens]

The code changes look good. I'm tempted to approve this, though I probably need to take another look over it first to see how the new exception may impact behaviour. It's possible an uncaught exception could have pretty bad consequences, depending on where it may occur.

[DanRStevens]

The question is, how does the code behave when an exception is thrown.

[Brett208]

If you think it is safer, we can pull the out of bounds check from the code and merge. It means we probably cannot close issue #96 though. I'm not in a particular hurry to merge though if you want to spend time reviewing in depth.

[DanRStevens]

Hmm, that might not be a bad idea. While looking over the code, it occurs to me there are uses where bounds checking matters, and where it doesn't matter. In particular, the logging code should just output whatever the value is, even if it's wrong. Especially if it's wrong. Currently the error check would prevent that output.

---

It may be easier to change the max number of players from 6 to 8. At least in terms of internal buffers, even if the UI still limits things to 6.

const int MaxRemotePlayers = 8;

There isn't a whole lot of wasted memory from that, since the array of player info is the following relatively small sized struct, which only needs an extra 2 copies.

struct PeerInfo
{
	int playerNetID;
	int status;
	sockaddr_in address;
	bool bReturnJoinPacket;
};

In small cases like this, the extra data memory could end up being less than extra code memory needed to implement the check and error message.

[Brett208]

I'm not too concerned with memory usage as I suspect either solution would be imperceptible to the end user. It might make sense to focus on readability.

Setting MaxRemotePlayer to 8 would give anyone reading the code an impression that you could literally have 8 remote players. They would have to check comments or dig through the code itself to understand that is not the case.

In several places, the code iterates through the entire PeerInfo container. In the code's current form, this could attempt to send packets to players 7 and 8 as well as display debug messages containing player 7 and 8. We could add a FakeMaxRemotePlayer = 8 constant and then use the old MaxRemotePlayer constant when iterating. It would also require forbidding range based iterations on PeerInfo. It seems like too many negatives to me.

We could add a new function called PlayerNetID::CheckPlayerIndex(int playerNetID) and remove the check from the function PlayerNetID::GetPlayerIndex. This would allow conditionally checking based on circumstance. It would increase the code footprint in OPUNetTransportLayer some. And we would still have to chose when it is appropriate to check and when not to check.

We could add a PlayerNetID::GetCheckedPlayerIndex function as well, which could improve readability in OPUNetTransportLayer.

If this isn't easy to solve here, I'd rather just remove the check for now and merge if the rest looks good. We can leave the issue open and resolve later if it is an impasse.

[DanRStevens]

I had also thought about having two get functions, for either checks or unchecked reading. Of course you'd still need to sort the calls into the correct form, and determine if proper exception handling is being done.

I'm kind of thinking remove the check for now so all the other changes can be merged in. There was some good stuff in the other code, and I don't want a large change set sitting open to accumulate merge commits with other stuff.

---

In terms of setting players to 8, the underlying network code is perfectly capable of handling that. It's only the game engine and UI that limit it. Those can both potentially be updated.

As for the iteration, I believe the send functions are checked so it doesn't send to players that don't exist. There may be some slight inefficiency in iterating over the entries, but I doubt it would ever be noticeable.

Changing to std::vector could reduce the fixed iteration, particularly as players drop from a game, though I don't think that's really worth it. Particularly since we can't exceed 8 players due to bitfield width. Trying to use std::vector might require adding checks to ensure the vector doesn't grow too large.

[Brett208]

OK, this should have restored original behaviour in handling invalid packets within the Receive function. The merge commit was a bear.

[DanRStevens]

I'll bet it was.

Changes look good. I expect this will function the same as it did before.