Hi!

I've been making a Breakout/Arkanoid clone game with a "simple" client-server arquitecture using C++, SDL, SDL_net and Visual Studio 2013 in Windows 8.1. Working through various articles and webs on network programming (e.g. beej's & Glenn Fiedler) I've created two classes: UDPSocket and Connection (virtual connection over UDP using a protocol ID) and they seemed to work "fine" in other simple projects. Now I want to send some struct between the server and the client with input or game state (like ball x position), but when the packet arrives at the other end the data inside it has changed (e.g 0x0072F868 vs 0x0072F6a4). I think I've pinned down the problem in the UDPSocket class, but I can't seem to solve it. The problem is in the functions in UDPSocket that copy an array with the data to the packet's data (Uint8*) and viceversa.

In order to send the struct I copy the struct to an unsigned char* and pass it to Connection, where the protocol ID is added. After that Connection passes the unsigned char* to the Send in UDPSocket, where the unsigned char* is copied to the packet. I feel the solution would be pretty obvious, but my actual knowledge don't let me see it.

As you can see in the code below, I get the game information with GetGameInfo(unsigned char* data) and pass it verbatim to Connection::SendPacket (const unsigned char data[]). In the client I try to do the opposite.

int UDPSocket::Receive(IPaddress& sender, void * data, int size)
{
	if (socket == nullptr)
	{
		return 0;
	}

	packet->data[0] = 0;

	if (SDLNet_UDP_Recv(socket, packet))
	{
		sender.host = packet->address.host;
		sender.port = packet->address.port;

		memcpy(data, (char*)packet->data, sizeof(data));

		return packet->len;
	}
	
	return 0;
}

bool UDPSocket::Send(const IPaddress &destination, const void * data, int size)
{
	packet->data[0] = 0;

	//Fill packet
	memcpy((char*)packet->data, data, sizeof(data));
	packet->address.host = destination.host;
	packet->address.port = destination.port;
	packet->len = size + 1;

	//Send packet. Return false if the packet cannot be sent;
	if (!SDLNet_UDP_Send(socket, -1, packet))
	{
		return false;
	}

	return true;
}


bool Connection::SendPacket(const unsigned char data[])
{

	if (!running)
	{
		std::cerr << "connection is not running" << std::endl;
		return false;
	}

	if (address.host == 0)
	{
		std::cerr << "address host is empty";
		return false;
	}
	
	//Insert the protocolId + data into the packet data
	unsigned char packet[PACKET_SIZE];
	int protocolIdAux = SDL_SwapBE32(protocolId);

	memcpy(packet, &protocolIdAux, sizeof(protocolIdAux));
	memcpy(packet + sizeof(protocolIdAux) + 1, data, sizeof(data));

	return socket.Send(address, packet, PACKET_SIZE);
}


int Connection::ReceivePacket(unsigned char data[])
{
	if (!running)
	{
		std::cerr << "connection is not running" << std::endl;
		return 0;
	}

	IPaddress sender;
	unsigned char packet[PACKET_SIZE];

	int bytes = socket.Receive(sender, packet, PACKET_SIZE);
	
	//If packet has no data or only the protocolId return 0
	if (bytes == 0 || bytes <= 4)
	{
		return 0;
	}

	int protocolIdAux = 0;
	memcpy(&protocolIdAux, packet, sizeof(protocolId));
	
	//If protocolID is not the connection's protocolId return 0
	
	if (protocolId != SDL_SwapBE32(protocolIdAux))
	{
		return 0;
	}

	//If packet comes from a client the server does not know anything about then server connects with the client (just one client at the moment)
	if (mode == Server && !IsConnected())
	{
		state = Connected;
		address = sender;
	}

	if (sender.host == address.host && sender.port == address.port)
	{
		if (mode == Client && state == Connecting)
		{
			std::cout << "client completes connection with server" << std::endl;
			state = Connected;
		}
		timeoutAccumulator = 0.0f;
		memcpy(data, packet + sizeof(protocolId) + 1, sizeof(data));
			

		return PACKET_SIZE - 4;
	}
	return 0;
}

struct NetworkGameState
{
int ballX;
int ballY;
int paddleX;
int idQuantity;
char bricksID[40];
int order;
};

void ServerGame::GetGameInfo(unsigned char* data)
{
NetworkGameState state;

state.ballX = SDL_SwapBE32((int)ball->GetPosX());
state.ballY = SDL_SwapBE32((int)ball->GetPosY());
state.paddleX = SDL_SwapBE32((int)paddle->GetPosX());
state.idQuantity = SDL_SwapBE32((int)bricksDestroyed.size());
state.order = SDL_SwapBE32((int)order);

int brickID;
int displacement;
int i = 0;

if (state.idQuantity)
{
displacement = 0;

std::vector<int>::iterator it;
for (it = bricksDestroyed.begin(); it != bricksDestroyed.end(); ++it)
{
brickID = *it;
memcpy(state.bricksID + displacement, &brickID, sizeof(int));
++i;
displacement = i*sizeof(int)+1;
}
}
else
{
state.bricksID[0] = 0;
}

memcpy(data, &state, sizeof(NetworkGameState));

}

Thanks in advance.

In the upd class send and receive you copy only the sizeof(data) but data is a pointer. So you copy only 4 bytes on a 32bit system or 8 bytes on 64bit.
I think you want copy the data that data points to? And in Receive you dont use the size parameter.

Thank you very much Tribad, that seems to be one of the problems, I use strlen now and the size parameter, but, unfortunately, the data send to the client is completely different to the data send by the server, e.g. ball x position on server = 223 ball x position on client = 3435973836. I tried to cout the content of the packet and the char* sent and received:

bool UDPSocket::Send(const IPaddress &destination, const void * data, int size)
{
	packet->data[0] = 0;

	//Fill packet
	memcpy((char*)packet->data, data, strlen((char*)data));
	packet->address.host = destination.host;
	packet->address.port = destination.port;
	packet->len = size;

	
	//Send packet. Return false if the packet cannot be sent;
	if (!SDLNet_UDP_Send(socket, -1, packet))
	{
		return false;
	}

	return true;
}

int UDPSocket::Receive(IPaddress& sender, void * data, int size)
{
	if (socket == nullptr)
	{
		return 0;
	}

	packet->data[0] = 0;

	if (SDLNet_UDP_Recv(socket, packet))
	{
		sender.host = packet->address.host;
		sender.port = packet->address.port;

		memcpy(data, (char*)packet->data, size);

		return packet->len;
	}
	
	return 0;
}

bool Connection::SendPacket(const unsigned char data[])
{

	if (!running)
	{
		std::cerr << "connection is not running" << std::endl;
		return false;
	}

	if (address.host == 0)
	{
		std::cerr << "address host is empty";
		return false;
	}
	
	//Insert the protocolId + data into the packet data
	unsigned char packet[PACKET_SIZE];
	int protocolIdAux = SDL_SwapBE32(protocolId);

	memcpy(packet, &protocolIdAux, sizeof(protocolIdAux));
	memcpy(packet + sizeof(protocolIdAux) + 1, data, strlen((char*)data));
	

	return socket.Send(address, packet, PACKET_SIZE);
}

int Connection::ReceivePacket(unsigned char data[])
{
	if (!running)
	{
		std::cerr << "connection is not running" << std::endl;
		return 0;
	}

	IPaddress sender;
	unsigned char packet[PACKET_SIZE];

	int bytes = socket.Receive(sender, packet, PACKET_SIZE);
	
	//If packet has no data or only the protocolId return 0
	if (bytes == 0 || bytes <= 4)
	{
		return 0;
	}

	int protocolIdAux = 0;
	memcpy(&protocolIdAux, packet, sizeof(protocolId));
	
	if (protocolId != SDL_SwapBE32(protocolIdAux))
	{
		return 0;
	}

	//If packet comes from a client the server does not know anything about then server connects with the client (just one client at the moment)
	if (mode == Server && !IsConnected())
	{
		state = Connected;
		address = sender;
	}

	if (sender.host == address.host && sender.port == address.port)
	{
		if (mode == Client && state == Connecting)
		{
			std::cout << "client completes connection with server" << std::endl;
			state = Connected;
		}
		timeoutAccumulator = 0.0f;
		memcpy(data, packet + sizeof(protocolId)+1, strlen((char*)packet + sizeof(protocolId)+1));
		
		
		return PACKET_SIZE - 4;
	}
	return 0;
}

void Server::SendGameInfoToClient()
{
	unsigned char data[512];
	
	serverGame->GetGameInfo(data);

	connection->SendPacket(data);
}

 void ClientGame::GameInfoToObjects(unsigned char* data)
 {
	 NetworkGameState state;

	 memcpy(&state, data, sizeof(NetworkGameState));

	 ball->SetPosX(SDL_SwapBE32(state.ballX));
	 ball->SetPosY(SDL_SwapBE32(state.ballY));
	 
	 paddle->SetPosX(SDL_SwapBE32(state.paddleX));

	 points += state.idQuantity*POINTS_MULTIPIER;

	 if (state.idQuantity)
	 {
		 int aux;
		 int i = 0;
		 int displacement = 0;

		 for (int j = 0; j < state.idQuantity; ++j)
		 {
			 
			 memcpy(&aux, state.bricksID + displacement, sizeof(int));
			 board->destroyBrick((aux));

			 ++i;
			 displacement = i*sizeof(int)+1;
		 }
	 }
 }

by the server, e.g. ball x position on server = 223 ball x position on client = 3435973836.

When doing stuff like this, it's much more useful to look at numbers in hex.

3435973836 -> CCCCCCCC

CCCCCCCC is often used as a marker for uninitialized memory in debug builds. So that should help you narrow this down.

Making a review now.

Pass the size to "bool Connection::SendPacket(const unsigned char data[]) " because it is best for a sending operation not to make any assumptions about the content.

As I said before: This probably will not work. because strlen stops counting at the first null-byte. This even means it stops earlier if any data contains a single byte=='\0' and it will not stop before any '\0' has been found. You could get here sizes much greater than the data are.

int protocolIdAux = SDL_SwapBE32(protocolId);

use int32_t because these type is 32-bit on any platform, even if you change to 64-Bit or the compiler has other type sizes. This is usefull because SDL_SwapBE32 is limited to 32-Bit.

memcpy(packet + sizeof(protocolIdAux) + 1, data, strlen((char*)data));

The next is another problem you should avoid:

memcpy(packet + sizeof(protocolIdAux) + 1, data, strlen((char*)data));

Because indexing in C/C++ starts with zero packet+sizeof(protocolIdAux already points behind the protocol id.

Connection::ReceivePacket:

Receiving an UDP Packet is always complete. If the transmission of the UDP-Packet fails you got nothing. If you got something it is always complete.

memcpy(data, packet + sizeof(protocolId)+1, strlen((char*)packet + sizeof(protocolId)+1));

Same as above strlen probably will give you wrong length of packet.

In UDPSocket::Receive:

You return the size of the received packet. Use it to copy your data later. in Connection::ReceivePacket

Maybe there is more wrong but these are the things I could find in a hurry.

Thank you very much guys!, your replies has been very helpful. Changing the types of int to Uint32, passing and returning the size explicitly and fixing the "indexing" solved the problem. I post the fixed code below.

bool UDPSocket::Send(const IPaddress &destination, const void * data, size_t size)
{
	packet->data[0] = 0;

	//Fill packet
	memcpy((char*)packet->data, data, size);
	packet->address.host = destination.host;
	packet->address.port = destination.port;
	packet->len = size+1;

	//Send packet. Return false if the packet cannot be sent;
	if (!SDLNet_UDP_Send(socket, -1, packet))
	{
		return false;
	}

	return true;
}

size_t UDPSocket::Receive(IPaddress& sender, void * data)
{
	if (socket == nullptr)
	{
		return 0;
	}

	packet->data[0] = 0;

	if (SDLNet_UDP_Recv(socket, packet))
	{
		sender.host = packet->address.host;
		sender.port = packet->address.port;

		memcpy(data, (char*)packet->data, packet->len);

		return packet->len;
	}
	
	return 0;
}


bool Connection::SendPacket(const unsigned char data[], size_t size)
{

	if (!running)
	{
		std::cerr << "connection is not running" << std::endl;
		return false;
	}

	if (address.host == 0)
	{
		std::cerr << "address host is empty";
		return false;
	}
	
	//Insert the protocolId + data into the packet data
	unsigned char packet[PACKET_SIZE];
	Uint32 protocolIdAux = SDL_SwapBE32(protocolId);

	memcpy(packet, &protocolIdAux, sizeof(Uint32));
	memcpy(packet + sizeof(Uint32), data, size);

	return socket.Send(address, packet, sizeof(Uint32)+size);
}


int Connection::ReceivePacket(unsigned char data[])
{
	if (!running)
	{
		std::cerr << "connection is not running" << std::endl;
		return 0;
	}

	IPaddress sender;
	unsigned char packet[PACKET_SIZE];

	size_t bytes = socket.Receive(sender, packet);
	
	//If packet has no data or only the protocolId return 0
	if (bytes <= sizeof(Uint32))
	{
		return 0;
	}

	Uint32 protocolIdAux = 0;
	memcpy(&protocolIdAux, packet, sizeof(Uint32));
	
	if (protocolId != SDL_SwapBE32(protocolIdAux))
	{
		return 0;
	}

	//If packet comes from a client the server does not know anything about then server connects with the client (just one client at the moment)
	if (mode == Server && !IsConnected())
	{
		state = Connected;
		address = sender;
	}

	if (sender.host == address.host && sender.port == address.port)
	{
		if (mode == Client && state == Connecting)
		{
			std::cout << "client completes connection with server" << std::endl;
			state = Connected;
		}
		timeoutAccumulator = 0.0f;
		memcpy(data, packet + sizeof(Uint32), bytes - sizeof(Uint32));

		return bytes - sizeof(Uint32);
	}

	return 0;
}