Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BIG Serialization Update #102

Merged
merged 19 commits into from
Dec 5, 2023

Conversation

EthanRylko
Copy link
Contributor

-Added serialization schemes for agents and items, and world types
-Added manager classes to manage agent movement on client and server
-ControlledAgents, client-side agents that mimic agents/interfaces on the server
-Demo modes: Server chooses world type by program arguments, then client gets world data from server
This has been done for a bit but I want to break up our feature PR a bit. Actual multiplayer coming later.

Added serialization/deserialization for type_options
Serialization/Deserialization for WorldBase
Server serializes world and sends string to client
Client deserializes world with new MazeWorld constructor
-Added constructors in each world type for serialized string
-Added property serialization for type options
-Edited client and server main to support all worlds
-ControlledAgents, agents that only appear on the client side and mimic actions of agents on the server side
-ServerManager and ClientManager to facilitate this
-Synchronization bug to be fixed
-Removed deserialization constructors in derived classes
-Made action maps in client and server manager private
-Removed type_options serialization, redundant with having to have specific worlds in server
-PropertyType enum class in Data
-Pure virtual Entity serialization
-Item Serialize and Deserialize
Run (for local games) is now separate from running a server, which requires ServerManager involvement
Server can select between four world types with different agents and items. Client automatically chooses corresponding world.
Copy link
Contributor

@FergusonAJ FergusonAJ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this looks really solid!
I've flagged a few style things, but functionally this looks really good!

I think there's an issue with how this interacts with CMake, but I'll change that after this is merged in.

Misc. Notes:

  • Running things locally, I get an issue where I optional isn't defined in a few files. It looks like a #include <optional> in NetworkInterface.hpp fixes this. I'll also fix this after the merge.

@@ -0,0 +1,99 @@
/**
* This file is part of the Fall 2023, CSE 491 course project.
* @brief A networking interface that allows information to be sent across a network
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably want to update this, looks like it's the same as ClientInterface

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, this and client interface both match NetworkingInterface. It's worth taking a pass to clean these up. I know keeping up with them is a pain, but it really does help orient folks who are new to the code.

* @param id Agent ID
* @return true if ID is present
*/
bool IdPresent(size_t id) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm being nitpicky here, but it would be nice if ID was consistent between this and GetActionID.
I prefer ID over Id, but consistency is the important part.

public:
const static constexpr unsigned short m_initConnectionPort = 55000; ///Port for initial client connection

unsigned short m_maxClientPort = 55000; ///Port that is incremented for client thread handoff
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like these aren't used elsewhere. I'm assuming they come into play in multiplayer? If so that's totally fine.

/**
* Deserialize agents and add to world
* @param is istream
* @param world world that is being added to
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like world is no longer a parameter and can be cut from the doxygen comment

* @param world world that is being added to
* @param manager pointer to ClientManager for agents
*/
void DeserializeAgentSet(std::istream &is, netWorth::ClientManager *manager) {\
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a dangling \ here.
It doesn't seem to break anything, but it makes me uncomfortable...

std::cerr << "Could not find end of agent_set serialization" << std::endl;
return;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indentation got a little wonky in this function.

os << ":::END item_set\n";
}

void DeserializeItemSet(std::istream &is) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indentation


// grab action ID from MainInterface
std::uint32_t action_id = i_2D::MainInterface::SelectAction(grid, type_options,
item_set, agent_set);
std::cout << action_id << std::endl;
//std::cout << action_id << std::endl;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is super minor, but as we move to the final deadline, feel free to cut lines like this instead of commenting. Old versions are always available on GitHub anyway!
(I don't think we'd take points off for something like this, unless it's particularly egregious.)

@FergusonAJ FergusonAJ merged commit 0cc9410 into MSU-CSE491:development Dec 5, 2023
1 check failed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants