Fix zipline+water desync

[SamuZad]

When a beaver rides a zipline toward a flooded station, MovementAnimator.Update() fires a group ID change every frame. Since different machines run at different frame rates, ZiplineVisitor.IsOnZipline can flip at different times relative to game ticks. This makes ZiplineWaterPenaltyModifier return 0.5 on one machine and 1.0 on the other — a 2x movement speed difference that snowballs into a desync

This PR addressed the problem stated above

[thomaswp]

Thanks for this - sounds like this was a tough one to track down and fix. I want to take a deeper dive before I release it, just to double-check for unintended consequences, but it looks good at first readthrough. I'll aim for including it in the next release.

[thomaswp]

I've also got a persistent desync on a zipline save that I could test this against. It could very well be the cause.

[thomaswp]

@SamuZad So my map isn't reproducing this like I expected (I actually think your earlier name sync PR fixed that :D). It sounds like flooding is an important part of it? Do you have a map that reliably reproduces this desync to test the fix? In the meantime I'll make sure there's no unintended consequences, and if not, I suppose I can just merge it.

[thomaswp]

Ok, so I think this makes sense, but I wanted to confirm a few things before merging. I see the problem, and that's a subtle one, so thanks for tracking it down!

First, can you explain your take on how the various groupIDs are being used (MovementAnimator, AnimatedPathCorner, AnimatedPathFollower). I've seen a few use cases that I agree would cause desyncs if they got out of sync, e.g. deciding if a character is on a zipline or in some situations how fast they turn. But I'm guessing there's another use case and I just haven't tracked down where. Is it used to batch updates? Do the group IDs on all 3 classes refer to the same group, or are there different kinds of groups?

Second, from what I understand, you fix doesn't prevent the group from being updated outside of a tick (e.g. during Update()), it just deterministically resets the group before each tick, in the same way I deterministically set the position.

My only concern with the reset approach is that MovementAnimator.NotifyGroupIdUpdated() still gets called during Update(), and then called again at the start of a tick. This event will also trigger ZiplineVisitor.OnGroupIdUpdated() both times as well, and I imagine there may be (now or someday) other listeners. I wonder if that might still deysnc the state.

Would it make sense to also prevent group updates during MovementAnimator.Update()? I've already rewritten that whole method in AnimationFixes, so it would be easy enough. We could ensure it updates deterministically just like the current movement animations do, and then fix it at the start of a tick. But I don't know for sure if this is necessary, or if there'd be unintended consequences, so that's why I'm trying to better understand how the group is used in the first place.

[thomaswp]

FWIW, I think either way the content of the PR would be part of the fix, so I could go ahead and merge it and we could see if there are still issues. I don't think that resetting the group would create any new desyncs, since it only updates if needed. So if you think that's a longer conversation and just want to get it out, I can file an issue for what I discussed here and merge this.

[SamuZad]

@thomaswp I still have my save that I used to make this video. The caveat is that it's a save with other mods, so you'd have to install about 20 or so of them to have it work.. Not because the issue is mod-related (it is not), but you wouldn't be able to load the map otherwise

[SamuZad]

racked down where. Is it used to batch updates? Do the group IDs on all 3 classes refer to the same group, or are there different kinds of groups?

I since implementing the fix I have not seen this particular desync despite the occasional flooding of zipline stations around the map - so I think it would be worthwhile to merge as-is

[SamuZad]

Ok, so I think this makes sense, but I wanted to confirm a few things before merging. I see the problem, and that's a subtle one, so thanks for tracking it down!

First, can you explain your take on how the various groupIDs are being used (MovementAnimator, AnimatedPathCorner, AnimatedPathFollower). I've seen a few use cases that I agree would cause desyncs if they got out of sync, e.g. deciding if a character is on a zipline or in some situations how fast they turn. But I'm guessing there's another use case and I just haven't tracked down where. Is it used to batch updates? Do the group IDs on all 3 classes refer to the same group, or are there different kinds of groups?

Second, from what I understand, you fix doesn't prevent the group from being updated outside of a tick (e.g. during Update()), it just deterministically resets the group before each tick, in the same way I deterministically set the position.

My only concern with the reset approach is that MovementAnimator.NotifyGroupIdUpdated() still gets called during Update(), and then called again at the start of a tick. This event will also trigger ZiplineVisitor.OnGroupIdUpdated() both times as well, and I imagine there may be (now or someday) other listeners. I wonder if that might still deysnc the state.

Would it make sense to also prevent group updates during MovementAnimator.Update()? I've already rewritten that whole method in AnimationFixes, so it would be easy enough. We could ensure it updates deterministically just like the current movement animations do, and then fix it at the start of a tick. But I don't know for sure if this is necessary, or if there'd be unintended consequences, so that's why I'm trying to better understand how the group is used in the first place.

From my understanding: Same group concept across all three classes — one shared int propagated unchanged.

PathCorner.GroupId
   → AnimatedPathCorner.GroupId
   → AnimatedPathFollower.CurrentGroupId
   → MovementAnimator._groupId

Is it used for batch updates? Not that I could see (though of course I could be wrong).

And yes, I essentially "reset" the position - I have not seen any desyncs and we've been playing with this "enabled" for quite some time - but of course it's possible that our playstyle is just not bringing out other edge cases

With regards to the zipline though, I'm sure this is safe because it only happens on "transition" (ie when beavers get on and off the zipline)

And yeah, I thought about touching MovementAnimator.Update() but I don't think it's strictly necessary for this specific desync. However it's probably worthwhile for future-proofing