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

State serialization does not hold the player turn information #1277

Closed
waterhorse1 opened this issue Sep 10, 2024 · 3 comments
Closed

State serialization does not hold the player turn information #1277

waterhorse1 opened this issue Sep 10, 2024 · 3 comments
Labels
bug Something isn't working fixed This is fixed internally, and will be merged in the next github sync!

Comments

@waterhorse1
Copy link
Contributor

Thanks for this great repo and it is really helpful!

I am running experiments on the breakthrough environment. My experiments need to save some intermediate states and try to reload them so I can reset the environment state to any point I want. I find that state serialization can help me for most of my needs, except for the player turn information. The state serialization seems to miss the player turn information so I cannot fully retrieve the state from it.

Do I miss anything or is there any better way to implement it?

@lanctot
Copy link
Collaborator

lanctot commented Sep 11, 2024

Oh wow.. I think you're right. That's a bug. That is very, very old code.. I believe it was written before we open-sourced OpenSpiel, might be more than 5 years old.

Breakthrough does not really need custom serialization. It probably was coded before we introduced the general serialization. We could just use the default one (which stores each game's parameters and full action history). This works very well for all the simple games because it's game-independent and works directly from the general API.

So there are two options:

  1. Just remove the custom serialization methods (BreakthroughState::Serialize and BreakthroughGame::DeserializeState) which will adopt the default ones from State and Game

  2. Keep the custom serialization but fix it so that the current player is included.

I could go either way, but actually, I slightly prefer Option 2. Because then we could also easily implement NewInitialState(const std::string& str) described here:

virtual std::unique_ptr<State> NewInitialState(const std::string& str) const {
. We could refactor part of the the BreakthroughGame::DeserializeState function into a BreakthroughState::FromString function that would then also be called by NewInitialState(const std::string& str).

What do you think? Could you submit a PR fix? It'd be great to get credit for finding / fixing this but if you don't have time, we could also easily do it on our side.

@lanctot lanctot added the bug Something isn't working label Sep 11, 2024
@waterhorse1
Copy link
Contributor Author

waterhorse1 commented Sep 12, 2024

Just submit a PR!

#1278

It's been a long time since the last time I wrote C++, so not sure evetything is sound. Let me know if there is anything you want me to change!

@lanctot lanctot added the fixed This is fixed internally, and will be merged in the next github sync! label Sep 23, 2024
@lanctot
Copy link
Collaborator

lanctot commented Sep 23, 2024

Great, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working fixed This is fixed internally, and will be merged in the next github sync!
Projects
None yet
Development

No branches or pull requests

2 participants