Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 6 commits into from
Oct 3, 2024

Conversation

Dystopian
Copy link
Contributor

When merged this pull request will:

  • title.

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
Copy link
Contributor

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.

addons/repair/XEH_postInit.sqf Outdated Show resolved Hide resolved
@LinkIsGrim
Copy link
Contributor

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
Copy link
Contributor

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?).

Co-authored-by: Jouni Järvinen <[email protected]>
@johnb432
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor Author

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 Dystopian marked this pull request as draft September 26, 2024 22:29
@Dystopian
Copy link
Contributor Author

Dystopian commented Sep 26, 2024

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 Dystopian marked this pull request as ready for review September 28, 2024 00:40
@Dystopian
Copy link
Contributor Author

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.

@LinkIsGrim LinkIsGrim added the kind/bug-fix Release Notes: **FIXED:** label Oct 3, 2024
@LinkIsGrim LinkIsGrim added this to the 3.18.0 milestone Oct 3, 2024
@LinkIsGrim LinkIsGrim added the status/added-to-RC Added after RC, need to manually add to changelog label Oct 3, 2024
@LinkIsGrim LinkIsGrim changed the title Repair - Fix unit being vanilla engineer when ACE Repair is enabled Repair - Fix unit being vanilla engineer with Repair enabled Oct 3, 2024
@LinkIsGrim LinkIsGrim merged commit 4114631 into acemod:master Oct 3, 2024
4 checks passed
@Dystopian Dystopian deleted the fix-engineer-trait branch October 4, 2024 05:04
@PabstMirror PabstMirror removed the status/added-to-RC Added after RC, need to manually add to changelog label Oct 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug-fix Release Notes: **FIXED:**
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants