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

OOS (out of sync) when trading with indians #228

Closed
CharlySmitn opened this issue Dec 18, 2018 · 12 comments
Closed

OOS (out of sync) when trading with indians #228

CharlySmitn opened this issue Dec 18, 2018 · 12 comments
Labels
bug Something isn't working Network Desyncs and other network communication
Projects
Milestone

Comments

@CharlySmitn
Copy link

Frequent OOS (out of sync in multiplayer) when trading with indians. Probably caused by bargaining option.

@Nightinggale Nightinggale added bug Something isn't working Network Desyncs and other network communication labels Dec 18, 2018
@Nightinggale
Copy link
Contributor

I remember this was fixed ages ago, like before RaR 2.5. Then it broke and was fixed, also before 2.5. Now it seems to be broken again.... something tells me that we might want to look into a code redesign. The current implementation seems rather fragile and prone to OOS.

@orlanth
Copy link

orlanth commented Dec 20, 2018

Maybe adopt the trade/diplo code from DoaNE if it’s more stable and full featured?

@Nightinggale
Copy link
Contributor

I don't think it's a good idea to import network code from another mod. If a fix should depend on anything, then make it depend on #201.

@abmpicoli
Copy link
Member

Hi, everyone. I have witnessed that when I bargain prices with the indians with the option "new random seed on reload" turned off, the indians will offer a different price: this suggests that the OOS could really be related on how the bargaining prices is calculated... (Or that to turn off the "new random seed" option doesn't work anymore).

@devolution79
Copy link
Member

devolution79 commented Jan 2, 2019

Just had a look @ the code and I see that the random rolls in CvPlayer::tryGetNewBargainPriceSell() and CvPlayer::tryGetNewBargainPriceBuy() uses rand() directly (no getSorenRandNum() )
However, the real issue may be that this is an "async" context and we should thus use GC.getASyncRand().
When the bargaining has completed, we have to find a way to sync it to the rest of the players.

@Nightinggale Nightinggale added this to the 2.8 milestone Sep 26, 2019
@devolution79
Copy link
Member

Due to a recent complaint about this (see https://steamcommunity.com/app/16810/discussions/0/1694969274389393160/
) and considering that this is a long-standing bug, I propose that we fix it in 2.7.x

@devolution79
Copy link
Member

Here's a quick idea.

Let's create a hash that samples some "semi-random" variables from the player (that's already in sync) and then use it to determine the result. If we can get away with that we don't have to deal with MP sync issues at all

@Nightinggale
Copy link
Contributor

I wonder if it would be possible to transmit haggle package in the diplo screen and then when receiving all computers will run a synced random check to get the outcome. I think that would be the best solution, also for testing purposes where we run the same savegame multiple times with the same random seed in order to test function or performance differences.

I still want a feature where we can freeze the random seed as in load one from a savegame and never replace it with a randomly generated one as this will be useful for various types of testing for consistency.

@raystuttgart
Copy link
Collaborator

@ErikSandbraaten:
Ah I saw that you already thought about that "semi-random" number idea. :)
My post in slack was unnecessary then.

@Nightinggale:
To be honest I have no idea how your idea could technically work.
But you are a much better programmer than myself so I trust that you might find a solution.
But until then the idea of Erik could be a workaround.

@Nightinggale Nightinggale added this to TO Do in 2.7.2 Jan 11, 2020
@devolution79
Copy link
Member

I propose that we disable bargaining for MP games until we figure out a fix ?

@raystuttgart
Copy link
Collaborator

Disabling it for MP games (only) is perfectly fine to me.
Once we know how to solve this, we could enable it again in MP games.

But in SP games the feature should of course still be available.

@raystuttgart
Copy link
Collaborator

@devolution79
I think I read that you had fixed this.
Thus I think we can close the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Network Desyncs and other network communication
Projects
2.7.2
  
TO Do
Development

No branches or pull requests

6 participants