The International Simutrans Forum

 

Author Topic: Fix for desync related to trees  (Read 803 times)

0 Members and 1 Guest are viewing this topic.

Offline ceeac

  • Devotee
  • *
  • Posts: 267
Fix for desync related to trees
« on: July 20, 2021, 07:51:30 PM »
This patch fixes a desync which may occur when processing a tile that contains a tree that is about to die and a tree that is about to spawn new trees. The order in which both trees are processed is essentially random so it is possible for one tree to be deleted on the server while it is still present on the client. To reproduce the desync:
  • set max_no_of_trees_on_square=5 in simuconf.tab
  • Start a paused server with the yoshi test map
  • Connect to server with a client
If the desync does not occur after a few seconds, restart the server and reconnect the client to the server.

I think the only way to fix this is by storing tree offsets in the save again, even though this makes server saves significantly larger.
I tried to counter this increase by pooling desc names of trees - the names of tree descs are stored separately instead of with each tree. However, server saves with this patch are still ~20% larger on average compared to before.

The patch also increases the save game version.

Offline Dwachs

  • DevTeam, Coder/patcher
  • Administrator
  • *
  • Posts: 4897
  • Languages: EN, DE, AT
Re: Fix for desync related to trees
« Reply #1 on: July 21, 2021, 07:25:08 AM »
Good catch!

Two questions:

1) Why did you move tree loading to baum.cc? Leaving it in boden.cc would save one byte per tree for obj-id for saving/loading in obj_list. If because of private member variables, then a new method rwdr_lean in baum.cc could do the trick.
2) Also why these computations with xoff/yoff? It think saving these two bytes and restoring them after call to constructor would be enough.

Offline prissi

  • Developer
  • Administrator
  • *
  • Posts: 10818
  • Languages: De,EN,JP
Re: Fix for desync related to trees
« Reply #2 on: July 21, 2021, 01:50:12 PM »
Server and client always have the same pak, otherwise they desync anyway. So the id to names exercie is not needed; in the worst case just another type of tree would appear in loading an old server game in a newer pak server or an offline client. For a server with "legal" client, the tree ids should be identical.

Since the only problem is the offset, how about using the id and the current age instead of the pointer? Those two are deterministic and should always give similar offsets. Furthermore, the age is already random. So replace Line 264 in baum.cc by
Code: [Select]
const sint16 random = (sint16)( get_pos().x + get_pos().y + get_pos().z + slope + geburt + tree_id );
With this all desync seems gone. Now identical trees born in the same month gets similar offsets; but that probably rather happens very rarely.

EDIT:
To be precise: Out of 310384 trees in the yoshi map it concerns only 95 trees. SO one can ignore this entirely and keep the savegame size as it is now.

EDIT 2:
I commited the above line as r9954. It would appreaciate, if you could test that I was not just lucky for my 15 tries.
« Last Edit: July 21, 2021, 02:35:03 PM by prissi »

Offline Freahk

  • Devotee
  • *
  • Posts: 1585
  • Languages: DE, EN
Re: Fix for desync related to trees
« Reply #3 on: July 21, 2021, 05:26:15 PM »
How does this affect the tool_plant_tree_t?
In that case there will be many trees generated in the same mmonth.

Offline prissi

  • Developer
  • Administrator
  • *
  • Posts: 10818
  • Languages: De,EN,JP
Re: Fix for desync related to trees
« Reply #4 on: July 22, 2021, 01:17:57 AM »
Unless there is a default parameter, yes, indeed. Although random trees are spawned, and thus these will has different offets. But I think there should be a random age for this tool anyway without a default parameter..

Offline Freahk

  • Devotee
  • *
  • Posts: 1585
  • Languages: DE, EN
Re: Fix for desync related to trees
« Reply #5 on: July 22, 2021, 09:18:34 AM »
There are some paksets out there with a rather low variety of trees (per climate).
Although not exactly correct, slightly randomising the age seems fair to me.

Another consideration: Why is the hang/slope involved in this? Shouldn't the coords be enough to have different patterns on different tiles?
« Last Edit: July 22, 2021, 11:34:04 AM by Freahk »

Offline prissi

  • Developer
  • Administrator
  • *
  • Posts: 10818
  • Languages: De,EN,JP
Re: Fix for desync related to trees
« Reply #6 on: July 22, 2021, 03:03:09 PM »
The slope does not hurt, but is not really needed. (Ok, it is needed to calculate the actual offset down in the function.) Actually one could have a better pseudorandom function which could involve the slope.

Offline ceeac

  • Devotee
  • *
  • Posts: 267
Re: Fix for desync related to trees
« Reply #7 on: July 25, 2021, 05:08:28 AM »
Hm, if tree offsets can be recalculated from the remaining tree properties, I think we can avoid having to save the offsets in the save game also for clients. In addition to reducing the save size this would be one step closer to byte-equal client and server saves which would help with checking for desyncs.

Offline prissi

  • Developer
  • Administrator
  • *
  • Posts: 10818
  • Languages: De,EN,JP
Re: Fix for desync related to trees
« Reply #8 on: July 25, 2021, 12:08:32 PM »
Indeed, this is possible.

Offline ceeac

  • Devotee
  • *
  • Posts: 267
Re: Fix for desync related to trees
« Reply #9 on: September 04, 2021, 12:16:26 PM »
Here is a patch with the changes for r10078. I have included the id <-> desc name mapping because it is required if you want to open the save with a new pakset version. For the same reason this is also enabled for servers (i.e. from now on you can load old server games with new paksets and trees will be migrated automatically via compat.tab).

Results for the yoshi save (in kB):
trunknewchange
binary2311218240-21%
xml202892157948-22%
zipped46083344-27%
bzip231202604-16%
zstd42443116-27%

Offline prissi

  • Developer
  • Administrator
  • *
  • Posts: 10818
  • Languages: De,EN,JP
Re: Fix for desync related to trees
« Reply #10 on: September 04, 2021, 01:03:11 PM »
Thank you very much. I think you can submit yourself. But please defer to bumping the actual savegame version yet, this is usually done shortly before a release only. (You can bump the server savegame version though, since this is only used on autosave and servers.)

Offline ceeac

  • Devotee
  • *
  • Posts: 267
Re: Fix for desync related to trees
« Reply #11 on: September 06, 2021, 06:25:56 AM »
Okay, submitted as r10080.