Desync plight continues

LoneGazebo commented 8 years ago

@ilteroi,

The modpacks for 7-27 that I released last night still have desyncs related to barbarians. Not sure where the desyncs are coming from, but my guess is that it is related to the tactical AI (i.e. units are ending up on different tiles on client/host and the desync is occurring). Maybe you and I should do a multiplayer game, hook into the DLL and try to debug it? Can we even do that?

ilteroi commented 8 years ago

never tried multiplayer ... don't know what will happen if one side suddenly stops answering because it hit a breakpoint. but it's worth a try, maybe next wednesday?

LoneGazebo commented 8 years ago

Yeah I could do that. Not sure what it'll do either. But what's the worst that could happen? :)

LoneGazebo commented 8 years ago

Theoretically, though, what is it about barbs that could be desyncing? Are there elements of barb data that we aren't accounting for when they're turned off?

LoneGazebo commented 8 years ago

Is it possible that the barbarian_player isn't being properly treated as an independent player in terms of memory allocation for things (as it sits right at the top of the max_player pool)?

ilteroi commented 8 years ago

i've checked the barbs again and again, found nothing ... until 5 minutes ago.

there are two things:

in GET_PLAYER() there is a failsafe, if you call it with -1 as argument we would normally crash, but i made it so that the barbarian player is returned instead. shouldn't be happening, but never know. maybe time to remove that crutch.
a funny bug. i tried to avoid duplicate pathfinding for unit missions, but i didn't consider that players can call TurnsToReach() et al on other players' units! so it would happen that the cached path changed between turns. especially the barbarian player would do that, which explains the unintended attacks on barb camps. maybe because of timing differences, it could also create desyncs. then again, the desyncs have been there forever, and shouldn't be related to the caching problem.

LoneGazebo commented 8 years ago

@ilteroi.

Top one could be the issue, however the fact that the desyncs stop when you disable barbarians via an option (something that only seems to affect things in the barbarians.cpp file, and two other places in MinorCivAI and Player) makes me wonder if it is also (or largely) an issue with camp spawning.

Second one might explain why users are seeing scouts move across barb camps and attack them. Glad you found it!

I've got one more small AI anomaly that I've been trying to pin down - it seems that ranged units (especially for barbs) never use their attacks if they have an extra movement point after their main move. Perhaps we should add an 'opportunity attack' to the end of move orders in the tactical AI if there are still move points left?

LoneGazebo commented 8 years ago

Re barbs, do a search for this: GAMEOPTION_NO_BARBARIANS

The parts affected by that may be the culprit. On the other hand, it may be that desyncs are gone because the barbarian player has nothing to do...

Iamblichos commented 8 years ago

I'll do some digging into this as well.

LoneGazebo commented 8 years ago

Thanks @Iamblichos

Iamblichos commented 8 years ago

I'm going to venture that it is in ::DoCamps or ::DoUnits

LoneGazebo commented 8 years ago

Possible. We'd need to look at changes versus base code to really understand why.

Iamblichos commented 8 years ago

Yeah but here's the deal about that, I've played a lot of Civ multiplayer (vanilla) and one of the things that is known as common knowledge is that barbs cause desyncs, only way to guarantee no desyncs in vanilla multiplayer is to get to use the option No Barbarians.

LoneGazebo commented 8 years ago

Hmm...interesting. Did anyone do any sleuthing back then?

LoneGazebo commented 8 years ago

I think the issue is with the RNG, myself. The fact that barbarian tactical moves, for example, are the only ones that use randomization in the tactical AI makes me feel that this is the root of the problem.

LoneGazebo commented 8 years ago

@ilteroi, what if we hijacked getjonrandnumber and used your pseudo-random number generator in all places instead? Stability at the cost of reduced randomness seems like a fair tradeoff.

Iamblichos commented 8 years ago

I don't know if anyone ever did any tinkering with the base DLL to fix the desyncs.

Iamblichos commented 8 years ago

RNG in respect to their moves or in respect to their spawns? Why don't we just get rid of the RNG in respect to their moves have them do some simple real Tactical AI moves, to protect camps or assault nearest units?

LoneGazebo commented 8 years ago

That's what they do now, but there's a small RNG added to priority that could cause desyncs if the RNG seed isn't being managed properly. I'm more worried about the fact that the RNG seed does not seem to be synchronous like it should be.

ilteroi commented 8 years ago

@LoneGazebo : both RNGs are pseudo-RNGs ... the difference is that mine is stateless. that's why it's not very random ;) anyway the problem was that sometimes the DLL RNG (JonRand) was called from the GUI thread via diplo AI calls, which changed the seed (state) for one player, leading to a desync. so i invented the stateless RNG.

another way would be to use the GUI RNG (AsyncRand) instead of DLL RNG whenever we're in a GUI thread, which can be checked with IsGameCoreThread(). so simply write a wrapper. except that's not completely foolproof either, because all LUA is executed in the GUI thread, no matter if it's a true GUI call or a hook from the DLL.

ilteroi commented 8 years ago

anyway, there's already a define for "REDUCE_RANDOMNESS" which can be extended. for the barbarians most randomness is already faked, except plot selection for new camps and unit type selection. so we could experimentally fake those as well and see if it helps.

in other news, there was also a report about ruins causing desyncs because the received goody type is different on both ends. don't know how you would even see what another player is getting. anyway another candidate for eliminating RNG calls (and popups and lua hooks because they are evil).

LoneGazebo commented 8 years ago

@ilteroi, I switched to your stateless RNG for the goodyhut check, and also changed the RNG in the tactical AI for barbarians. Where in the GUI is jonrand used? Is there a way to make it so that when the seed state changes, a forced update of the seed is made for all players?

ilteroi commented 8 years ago

i'm home now, but let's postpone the game until tomorrow maybe, if it's still needed ...

anyway, i currently don't know of any place where JonRand is called from the GUI. but the problem is it's hard to rule out what can be called from lua and what can not.

the re-sync is the forced update ... anyway i figure having a wrapper around any random call wouldn't hurt.

LoneGazebo commented 8 years ago

Sounds good. New modpack is out, hopefully someone will test it soon.

ilteroi commented 8 years ago

why did you remove GetBarbCampPlots()?

also, the potentially critical rand calls

            int iPlotIndex = kGame.getJonRandNum( bWantsCoastal ? vCoastalPlots.size() : vAllPlots.size(), "Barb Camp Plot-Finding Roll");

and

            iValue = (1 + kGame.getJonRandNum(1000, "Barb Unit Selection"));

are still there ...

LoneGazebo commented 8 years ago

GetBarbCampPlots CTD'd for me a few times and the lack of storage in the host side made me think that it might be a source of desync.

Ah, missed those rand calls, I'll edit that and seed a new DLL for MP testing.

ilteroi commented 8 years ago

so now i implemented workarounds for the two problems above, might not be truly random but should be good enough.

maybe i'll do the wrapper later on, let's see if this has an effect first - it's pushed

LoneGazebo commented 8 years ago

Cool. I'll merge, compile and seed the proper Modpacks. Thanks!

LoneGazebo / Community-Patch-DLL

Desync plight continues #2237