Fix setElementSyncer's persist argument

[HeresHavi]

Summary

Fixed the persist argument of setElementSyncer, which was applied to the wrong check and stored in the wrong place.

The server picks one player to simulate each driverless vehicle and each ped, known as that element's syncer, and reassigns the job automatically as players come and go. setElementSyncer(element, player, true) asks the server to keep a specific player in that role instead. Two separate things stopped that from working:

• A dimension change took the syncer away regardless, and usually left the vehicle with no syncer at all, so it stopped moving for everyone.
• The setting was stored once per sync manager rather than per element, so it applied to every unoccupied vehicle, or every ped, at the same time. It also outlived the syncer it was granted for.

Servers that do not pass the persist argument are unaffected. This is server side only, so there is no protocol or client change.

Motivation

The condition was grouped wrongly

CUnoccupiedVehicleSync::UpdateVehicle decides when to take a vehicle's syncer away. It should only do that when the syncer is not persistent, and then only if that player is too far away or in a different dimension. Missing parentheses left the dimension test outside the persistence check, because && binds tighter than ||:

(!persistent && !closeEnough) || dimensionMismatch

A dimension mismatch therefore took the syncer away on its own, whether or not persist was set. The indentation in the original code grouped the two tests together, but the code did not.

Two further details made the result worse than simply losing the setting. StopSync clears the persistence flag while it runs, so the setting was gone afterwards as well. And FindSyncer only considers players in the vehicle's own dimension, so there was usually nobody available to take over. The vehicle then stayed unsynced until a player entered that dimension.

For example, a resource sets a player as the persistent syncer of an unoccupied vehicle, then moves that vehicle into another dimension for an interior or an event area. The setting was discarded on the next update, about half a second later.

CPedSync::UpdateSyncer already implements the same rule correctly, as persistent || (sameDimension && closeEnough), so the intended grouping is not in question.

The setting was stored in the wrong place

Because the flag lived on the sync manager, using it on one element changed how the server treated all of the others.

For a vehicle, FindPlayerCloseToVehicle rejected every candidate player while the flag was set, so no other driverless vehicle could be given a syncer at all. Those vehicles stopped being simulated. For a ped, CPedSync::UpdateSyncer returned early for every ped, so peds held on to syncers that were too far away or in another dimension.

The setting also outlived the syncer it described. StopSync cleared it, but CPlayer's destructor drops a syncer by calling CVehicle::SetSyncer directly and never goes through StopSync. A player who was a persistent syncer and then disconnected therefore left the flag set with nobody holding it, and from that point no driverless vehicle could be given a syncer until an unrelated StopSync happened to clear it.

The flag now lives on CVehicle and CPed next to m_pSyncer, and SetSyncer clears it when the syncer is removed. Every route that drops a syncer goes through SetSyncer, including the disconnect path, so the rule is enforced in one place instead of at each call site. With that invariant in place the persistence check inside FindPlayerCloseToVehicle can no longer be reached, so it is removed.

OverrideSyncer also now applies the persistence setting when the given player is already the element's syncer. It previously returned early and only ever cleared the setting, which made setElementSyncer(element, player, true) a no-op whenever the engine had already picked that player.

The four call sites that consulted the old manager-wide flag now read the flag of the element in front of them. Three of those are the enter and exit paths in CGame, which are a straight substitution. The fourth is the trailer attach, which also passes the persistence on to the trailer, so a pinned towing vehicle does not keep its chosen syncer while the trailer is handed to somebody else on the next update. Worth knowing for review: trailer coupling has four independent server side paths and the attach handler in CGame is the only one that consults this flag, so that part only affects attaches arriving through the trailer packet rather than through the driver's own sync stream.

Runtime evidence

Server log from the test resource, run against the same tree built from master and with these two commits. Only the deathmatch module differed between runs.

Before:

case A | persist, dimension mismatch -> keep
  case A => *** FAIL ***  (kept=false expected=true)
case G | one vehicle persistent -> another still gets a syncer
  case G => *** FAIL ***  (other vehicle syncer=nil)
case I | engine picked the syncer, then persist=true -> must take effect
  case I => *** FAIL ***  (kept=false expected=true)
case P | one ped persistent -> another ped still loses its syncer
  case P => *** FAIL ***  (pedA=SyncTest pedB=SyncTest)
case H | after the persistent syncer disconnected, a new vehicle must get a syncer
  case H => *** FAIL ***  (new vehicle syncer=nil)
case Q | and a new ped must still lose its syncer when it should
  case Q => *** FAIL ***  (ped syncer=SyncTest)
SINGLE-JOIN RESULT: 10 passed, 6 failed, 0 inconclusive
TWO-JOIN RESULT: 0 passed, 2 failed, 0 inconclusive

After:

case A => PASS  (kept=true expected=true)
case G => PASS  (other vehicle syncer=SyncTest)
case I => PASS  (kept=true expected=true)
case P => PASS  (pedA=SyncTest pedB=nil)
case H => PASS  (new vehicle syncer=SyncTest)
case Q => PASS  (ped syncer=nil)
SINGLE-JOIN RESULT: 16 passed, 0 failed, 0 inconclusive
TWO-JOIN RESULT: 2 passed, 0 failed, 0 inconclusive

Test plan

Runtime

Eighteen cases in a server side test resource, run against the same tree built from master and with these two commits. Eight fail on master and all eighteen pass here. The other ten pass in both runs and exist to catch regressions.

• Dimension mismatch: a persistent syncer is kept; a non-persistent one is still dropped.
• Distance: a persistent syncer is kept with the vehicle about 2100 units away; a non-persistent one is dropped.
• Combined: persistent, far away and in another dimension, still kept.
• Other vehicles: with one vehicle persistent, a second untouched vehicle is still assigned a syncer. Fails on master.
• Re-applying persist: when the engine had already chosen that player, setElementSyncer(v, p, true) now takes effect. Fails on master.
• Clearing persist: setElementSyncer(v, p, false) on the current syncer clears it.
• Syncable toggling: turning unoccupied syncing off drops the syncer, turning it back on re-assigns.
• Destruction: destroying a persistent vehicle does not stop a new one being assigned a syncer. Fails on master.
• Peds: the same dimension and distance rules, plus one ped being persistent must not stop another ped losing its syncer. The last one fails on master.
• Disconnect: after a persistent syncer disconnects and reconnects, a new vehicle is still assigned a syncer, and a new ped still loses its syncer when it should. Both fail on master.

Steps to re-check this later

Run as a server script with one player connected.

-- The persist argument must survive a dimension change.
addCommandHandler("pintest", function(player)
    local x, y, z = getElementPosition(player)
    local dim = getElementDimension(player)

    local veh = createVehicle(411, x + 4, y, z)
    setElementDimension(veh, dim)
    setElementSyncer(veh, player, true)
    setElementDimension(veh, dim + 7)

    -- The syncer is re-evaluated every 500ms, so allow a few updates.
    setTimer(function()
        local s = getElementSyncer(veh)
        outputServerLog("syncer: " .. (isElement(s) and getPlayerName(s) or "none"))
        destroyElement(veh)
    end, 1600, 1)
end)

-- Persistence on one vehicle must not stop another from getting a syncer.
addCommandHandler("isolated", function(player)
    local x, y, z = getElementPosition(player)
    local dim = getElementDimension(player)

    local carA = createVehicle(411, x + 4, y, z)
    local carB = createVehicle(411, x + 8, y, z)
    setElementDimension(carA, dim)
    setElementDimension(carB, dim)
    setElementSyncer(carA, player, true)

    setTimer(function()
        local s = getElementSyncer(carB)
        outputServerLog("carB syncer: " .. (isElement(s) and getPlayerName(s) or "none"))
        destroyElement(carA)
        destroyElement(carB)
    end, 1600, 1)
end)

-- Persistence must not survive the syncer disconnecting. Run /leak, reconnect,
-- then run /leak again: the second run must report a syncer.
local pinned
addCommandHandler("leak", function(player)
    local x, y, z = getElementPosition(player)
    local dim = getElementDimension(player)

    if not pinned or not isElement(pinned) then
        pinned = createVehicle(411, x + 4, y, z)
        setElementDimension(pinned, dim)
        setElementSyncer(pinned, player, true)
        outputServerLog("armed; now reconnect and run /leak again")
        return
    end

    local fresh = createVehicle(411, x + 12, y, z)
    setElementDimension(fresh, dim)
    setTimer(function()
        local s = getElementSyncer(fresh)
        outputServerLog("new vehicle syncer: " .. (isElement(s) and getPlayerName(s) or "none"))
        destroyElement(fresh)
    end, 1600, 1)
end)

Expected in all three: the player's name. Before these changes each reports none.

Builds and Tests

• Debug | Win32: Full build passed, 304 client tests passed.
• Release | Win32: Full build passed.
• Debug | x64: Full build passed.
• Release | x64: Full build passed.
• Ran clang-format.

Checklist

• Your code should follow the coding guidelines.
• Smaller pull requests are easier to review. If your pull request is beefy, your pull request should be reviewable commit-by-commit.