acemod / ACE3

Open-source realism mod for Arma 3
https://ace3.acemod.org
Other
1k stars 736 forks source link

Repair - Fix unit being vanilla engineer with Repair enabled #10337

Closed Dystopian closed 4 weeks ago

Dystopian commented 1 month ago

When merged this pull request will:

I met this bug only once in multiplayer when made Repair enable setting. Since then I played hundred of times as engineer with DEBUG_MODE_FULL but bug disappeared. And when I tested 3.18.0 RC1 I met it again.

  1. getUnitTrait "engineer" can return nil (e.g. for ace_dragging_clone) so I added nil check.
  2. On unit InitPost event unit is (can be?) local for both server and client. In the next frame unit became local only for client in my tests. I used this code:
        INFO_4("setUnitTrait1 u=%1 t=%2 l=%3 e=%4",_unit,typeOf _unit,local _unit,_unit getUnitTrait "engineer");
        _unit setUnitTrait ["engineer", false];
        [{
            params ["_unit"];
            INFO_4("setUnitTrait3 u=%1 t=%2 l=%3 e=%4",_unit,typeOf _unit,local _unit,_unit getUnitTrait "engineer");
        }, _unit] call CBA_fnc_execNextFrame;

    client:

    19:10:42 [ACE] (repair) INFO: setUnitTrait1 u=B Alpha 1-1:1 (D) t=B_engineer_F l=true e=true
    19:10:42 [ACE] (repair) INFO: setUnitTrait3 u=B Alpha 1-1:1 (D) t=B_engineer_F l=true e=false

    server:

    19:12:34 [ACE] (repair) INFO: setUnitTrait1 u=B Alpha 1-1:1 (D) t=B_engineer_F l=true e=true
    19:12:34 [ACE] (repair) INFO: setUnitTrait3 u=B Alpha 1-1:1 (D) REMOTE t=B_engineer_F l=false e=false

The fix looks bad I know. Any suggestions are welcomed.

johnb432 commented 1 month ago

On unit InitPost event unit is (can be?) local for both server and client. In the next frame unit became local only for client in my tests.

I have a hard time believing that - not because it's impossible, but because a lot of code within ACE wouldn't be executing as expected. We definitely need to investigate that further.

LinkIsGrim commented 1 month ago

I have a hard time believing that - not because it's impossible, but because a lot of code within ACE wouldn't be executing as expected. We definitely need to investigate that further.

I can believe it, CSW used to duplicate mags because of similar behavior.

johnb432 commented 1 month ago

I have a hard time believing that - not because it's impossible, but because a lot of code within ACE wouldn't be executing as expected. We definitely need to investigate that further.

I can believe it, CSW used to duplicate mags because of similar behavior.

True. I just want to be sure, so we can implement mitigations properly (e.g. in CSW you added 1 second delay, whereas here it's only a frame later - is one frame enough? Is there a standard delay that we'd want to implement?).

johnb432 commented 1 month ago

The fix looks bad I know. Any suggestions are welcomed.

Wrap the original code in a CBA_fnc_execNextFrame - should do the trick, no?

johnb432 commented 1 month ago

I tried to replicate the issue, but I just don't have the setup to do so. Can anyone else please try to replicate?

Dystopian commented 1 month ago

Wrap the original code in a CBA_fnc_execNextFrame

You mean just do setUnitTrait in the next frame instead of current one? I can try testing this for some days but for now let it run twice just in case.

Dystopian commented 1 month ago

The fix doesn't work. I found reliable repro steps for me:

  1. dedicated server is just started before any missions loaded
  2. ACE Refuel is enabled
  3. mission with player as engineer on Mehland map (it is not cached in ACE Refuel positions)
  4. client was already loaded the map during game session
  5. start mission as soon as map is loaded after network lobby

Mission will start for some seconds. Player is not local neither in current frame nor in the next frame for InitPost.

Dystopian commented 1 month ago

Now it works. Local EH is enough.

For me in the worst case delay for locality pass is about 12 seconds. Just need to make server doing something long at postInit.

So it looks like JIP but it's not JIP. And now we know that player is not always local to client machine.