ServerGameState + SharedGameState Redesign Initial Implementation
#45
Conversation
…e Debugger can only be used to debug a ServerGameState instance), and updated serialization test code to use ServerGameState and SharedGameState.
gilkeidar
added
enhancement
gilkeidar
requested review from
Tyler-Lentz,
dmin12,
atar13,
EddieJ03 and
jhparkt
| // single template class - otherwise, each type-specific object vector will | ||
| // come with an extra free list and this will get hairy to use. | ||
|
|
||
| SmartVector<Object *> objects; |
There was a problem hiding this comment.
I like this approach but I wanted to clarify the need for the SmartVectorclass. Is it to hide the implementation details of managing the freelist in SmartVector?
There was a problem hiding this comment.
Yes, exactly - currently, each object has two IDs, a global ID and a type-specific ID; the idea here is that the ServerGameState class would have vectors for each object type (e.g., Player, Creature, etc.) and objects of that type would be stored in these vectors. The type-specific ID of the object would be its index in this type-specific vector, and its global ID would be its index in a global vector<Object &> objects vector. To avoid wasting space, when objects get removed and then new objects get created, I'd like for the freed object IDs to get reassigned. This means that every vector will need a free list to keep track of available IDs, and since we may have as many of these vectors as there will be object types, I think it makes sense to encapsulate the vectors with their free lists in a new class, which here is SmartVector.
| * @return ShareGameState instance that represents this ServerGameState | ||
| * instance. | ||
| */ | ||
| SharedGameState generateSharedGameState(); |
There was a problem hiding this comment.
I like this approach of generating the SharedGameState from the version the server has.
src/shared/game/gamestate.cpp
Outdated
There was a problem hiding this comment.
I'm not a huge fan of leaving many lines or entire files commented out. It's not that big of a deal but we do have Git tracking all our changes in case we want to revert to a previous version.
There was a problem hiding this comment.
You're right, I will remove the old GameState files (so gamestate.cpp and gamestate.hpp).
src/shared/game/servergamestate.cpp
Outdated
| Object** ptrToPtr = this->objects.get(i); | ||
|
|
||
| if (ptrToPtr == nullptr) { | ||
| // Push empty object to SharedObject SmartVector | ||
| shared.objects.pushEmpty(); | ||
| } | ||
| else { | ||
| Object* object = *ptrToPtr; | ||
|
|
||
| // Create a SharedObject from this object | ||
| shared.objects.push(object->generateSharedObject()); | ||
| } |
There was a problem hiding this comment.
Handling the double pointer to an object here feels kinda clunky.
Maybe we can have SmartVector::get return a std::shared_ptr to the underlying Object (this would mean that we'd have a SmartVector of Object not Object*).
Also, this isn't that big of a deal and it's totally fine if we keep this.
There was a problem hiding this comment.
Overall I think this is a solid foundation that summarizes all of the ideas that we have. I just have a couple of high level comments:
-
Like Anthony said I think it would be best to get rid of all of the commented out code so that it's easier to parse and there is less noise.
-
In addition, I think it would be better if whoever is iterating through a SmartVector doesn't have to worry about checking if the Objects they are retrieving are nullptr. That seems like it might get annoying to do, and I feel like someone could forget. To solve this, I think we could define a custom iterator for SmartVector that automatically skips over nullptr objects. I found this article talking about how to define custom iterators. I think this would be good to do anyways because it would be nice to get the good foreach syntax wherever we use a SmartVector. My idea would be to define an iterator on SmartVector whose ++ operation will keep iterating until it finds a valid object, and whose begin() makes sure to find the first object in the vector that is valid.
-
Also, I think we can get around the restrictions of the vector resizing by making the vectors contain smart pointers so that we don't have to worry about their addresses changing, and we can safely return smart pointers to the underlying objects. I think this might be worth looking into later on. However, I think the SmartVector class would still be important because it abstracts away the free list logic.
I think it would be best to do 1. before merging in, but I think we can do 2. and 3. later on. Approving because 1 is a nitpick, and this is a good change and the sooner we merge in the better.
Also I think there might be some merge conflicts that have to be resolved before merging.
And also, I am a little bit worried about the amount of repeated data between the shared data structures and server data structures. I hope it doesn't end up being too much. If it does there might be a better way to represent that relationship. One idea might be to make the server side data structures just contain the shared data structures inside of them as data members so there isn't repeated definitions of the same values that have to be explicitly copied over.
…ference to `gamestate.hpp`
…4/group3 into refactor/ServerGameState
…lementation (i.e., `SmartVector` won't cause the program to crash upon the wrapped vector reallocating)
Initial implementation of the
ServerGameState+SharedGameStateredesign.The key idea here is that the server maintains an instance of the
ServerGameStateclass which maintains all abstract game state data and logic, and after handling input events and updating theServerGameState, the server generates an instance of theSharedGameStatewhich contains all abstract game state data required by the client.SharedGameStateis serializable and is what is sent to the client withLoadGameStateEvents.Closes #42.