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

Common - Improve PBO checking #9266

Merged
merged 39 commits into from
Jun 22, 2024
Merged

Conversation

johnb432
Copy link
Contributor

@johnb432 johnb432 commented Jul 15, 2023

When merged this pull request will:

  • Intended to close ACE PBO Check, when set to "Kick", can be bypassed using an Xbox Controller #9181.
    However, I have not tested if ACE PBO Check, when set to "Kick", can be bypassed using an Xbox Controller #9181 has been fixed with this, as I do not own a controller.
  • FUNC(errorMessage) has been modified to be a dedicated function for usage in PBO checking and has been adapted to be executable on clients without any mods.
  • Added events "ace_versioning_clientCheckDone" and "ace_versioning_serverCheckDone" for public usage. This would allow other mods to be notified when PBO checking has been completed and whether it has been completed successfully or not. Event names and arguments passed are up for debate: For now, "ace_versioning_clientCheckDone" passes a copy of ACE_Version_ClientErrors, "ace_versioning_serverCheckDone" passes a copy of ACE_Version_ServerVersions.
  • FUNC(checkPBOs) uses the event "ace_versioning_clientCheckDone" instead of a PFH.
  • checkVersionNumber.sqf now prints "and x more." at the end of an error message, to allow players to know how many PBOs they are missing.
  • PFHs and waitUntil have been replaced with addPublicVariableEventHandler.
  • checkVersionNumber.sqf is now a function.
  • Added checking for clients that don't have ACE 3.x.x loaded (version depending on if/when this is added). If a client has an older version, they will be kicked. I was not satisfied by any of the solutions that we came up with, so I dropped it.
    The way it checks might need some tweaking (changes are in XEH_postInit.sqf).
  • Code cleanup.
Old description - Intended to close #9181 by adding a new argument to `FUNC(errorMessage)`, which allows users to pass codes to both the "Ok" and "Cancel" buttons, without having said buttons being shown. However, I have not tested if #9181 has been fixed with this, as I do not own a controller. - `FUNC(errorMessage)`: "ok" and "cancel" codes are no longer global variables ~~, they are passed using `CBA_fnc_addBISEventHandler`~~. This prevents code from being overwritten, however I'm unsure if this is necessary at all. - ~~PFHs and `waitUntil` used for checking public variables have had their intervals reduced from 1 second to 0.5 seconds. This should make loading into mission be a little snappier.~~ - Added events `"ace_versioning_clientCheckDone"` and `"ace_versioning_serverCheckDone"` for public usage. This would allow other mods to be notified when PBO checking has been completed and whether it has been completed successfully or not. Event names and arguments passed are up for debate: For now, `"ace_versioning_clientCheckDone"` passes a copy of `ACE_Version_ClientErrors`, `"ace_versioning_serverCheckDone"` passes a copy of `ACE_Version_ServerVersions`. - `FUNC(checkPBOs)` uses the event `"ace_versioning_clientCheckDone"` instead of a PFH. I'm unsure about this change, as if the event is raised externally it could cause some issues - hence the `WIP` tag in the title. If the event is not considered safe, then I will restructure `FUNC(checkPBOs)`: First I'd execute `checkVersionNumber.sqf` and `waitUntil` it's done, then execute the rest of `FUNC(checkPBOs)`, so as to remove a PFH check - or something else, see in the first point of the questions below. - ~~`checkVersionNumber.sqf` no longer relies on the global variable `ACE_Version_Whitelist`, the whitelist is passed by `FUNC(checkPBOs)` directly. This is to avoid third party influence. `ACE_Version_ClientVersions` has also been changed for the same reason.~~ no longer valid - `checkVersionNumber.sqf` now prints `"and x more."` at the end of an error message, to allow players to know how many PBOs they are missing. - Code cleanup.

8386a3c includes:

  • PFHs and waitUntil have been replaced with addPublicVariableEventHandler.
  • checkVersionNumber.sqf is now a function.
  • Added checking for clients that don't have ACE 3.x.x loaded (version depending on if/when this is added). If a client has an older version, they will be kicked.
    The way it checks might need some tweaking (changes are in XEH_postInit.sqf).
  • FUNC(errorMessage) has been adapted to be executable on clients without any mods.
Questions I had: - ~~Is there any specific reason why `waitUntil` is used in `checkVersionNumber.sqf` instead of a PFH? `FUNC(checkPBOs)` runs when the event `"CBA_settingsInitialized"` has been raised, so `CBA_fnc_addPerFrameHandler` ought to be available. Even more elegant imo would be to use a `addPublicVariableEventHandler` on `ACE_Version_ServerVersions` to check when it has been received by the client. Would there be any disadvantages to this?~~ answered - ~~In `FUNC(checkPBOs)`: What does the `@todo` refer to? The point above?~~ answered - ~~Currently, max. 11 PBOs are listed in the log if they are missing, wrong version etc. I added the `"and x more."` at the end of an error message. Should this be changed to something that reflects the total amount instead (e.g. `"(_x total)"`)? Or should ACE simply list all of the PBOs in the logs (probably not `systemChat` though)? Other ideas? I feel like the current info could be supplemented to help out people figure out what they need to load/unload.~~ - ~~For when 2.14 rolls out: Should `ACE_Version_ServerVersions` and `ACE_Version_ClientVersions` switch to hashmaps, for the sole purpose of hashmaps being able to be made final using [compileFinal](https://community.bistudio.com/wiki/compileFinal)? Unless - can arrays be made final? The disadvantage of hashmaps is the alphabetical order would be lost, which in this case would be less than ideal imo, so I don't really want to change to them. But at the same time, finalising the variables might outweigh that drawback.~~ answered
Looking forward to any input.

IMPORTANT

  • If the contribution affects the documentation, please include your changes in this pull request so the documentation will appear on the website.
  • Development Guidelines are read, understood and applied.
  • Title of this PR uses our standard template Component - Add|Fix|Improve|Change|Make|Remove {changes}.

@commy2
Copy link
Contributor

commy2 commented Jul 20, 2023

Is there any specific reason why waitUntil is used in checkVersionNumber.sqf instead of a PFH?

Legacy code / used to run when remote executed without CBA and ACE installed.

In FUNC(checkPBOs): What does the @todo refer to? The point above?

Yes. Todo: make it a function, not a script.

Should ACE_Version_ServerVersions and ACE_Version_ClientVersions switch to hashmaps, for the sole purpose of hashmaps being able to be made final using compileFinal?

Don't see the point. Also, whack that Dedmen didn't make them insertion ordered.

@johnb432
Copy link
Contributor Author

Legacy code / used to run when remote executed without CBA and ACE installed.

What does this mean ultimately? Can this be updated or not?

@commy2
Copy link
Contributor

commy2 commented Jul 25, 2023

What does this mean ultimately?

Nothing, it's already broken.

Can this be updated or not?

Sure, but keep in mind that it has to be backwards compatible, meaning, it makes no sense if someone connects with an older version and then nothing shows up because the interface of the PBO checking keeps changing between updates.

@PabstMirror
Copy link
Contributor

btw 2.14 will add mod hash to https://community.bistudio.com/wiki/allAddonsInfo
this might be simpler, faster and have the ability to check mods that don't version
and also handle checking mods that re-sign with the same key

@johnb432
Copy link
Contributor Author

johnb432 commented Aug 1, 2023

btw 2.14 will add mod hash to https://community.bistudio.com/wiki/allAddonsInfo this might be simpler, faster and have the ability to check mods that don't version and also handle checking mods that re-sign with the same key

Would this new system replace the current one or compliment it?

@PabstMirror
Copy link
Contributor

don't need to do anything about it in this PR
just posting info incase it could be useful

@LinkIsGrim LinkIsGrim added the kind/enhancement Release Notes: **IMPROVED:** label Aug 28, 2023
@BrettMayson BrettMayson changed the title WIP - Common - Improve PBO checking Common - Improve PBO checking Oct 16, 2023
@BrettMayson BrettMayson marked this pull request as draft October 16, 2023 06:49
@LinkIsGrim
Copy link
Contributor

Currently, max. 11 PBOs are listed in the log if they are missing, wrong version etc. I added the "and x more." at the end of an error message. Should this be changed to something that reflects the total amount instead (e.g. "(_x total)")? Or should ACE simply list all of the PBOs in the logs (probably not systemChat though)? Other ideas? I feel like the current info could be supplemented to help out people figure out what they need to load/unload.

All mismatched PBOs should be listed in logs. User error message (and that functionality) was changed in #9560, should update this PR after to see what overlaps/conflicts.

addons/common/XEH_postInit.sqf Outdated Show resolved Hide resolved
Copy link
Contributor

@LinkIsGrim LinkIsGrim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not mean to approve that.

@johnb432
Copy link
Contributor Author

All mismatched PBOs should be listed in logs. User error message (and that functionality) was changed in #9560, should update this PR after to see what overlaps/conflicts.

I have only addressed the merge conflicts so far, I haven't looked at compatibility between #9560 and this.

@johnb432 johnb432 marked this pull request as ready for review October 25, 2023 17:19
@LinkIsGrim
Copy link
Contributor

Haven't tested yet, but request after https://discord.com/channels/976165959041679380/1166914354051027024: addon mismatch message should show server's configSourceMod for common or main

@johnb432
Copy link
Contributor Author

Haven't tested yet, but request after https://discord.com/channels/976165959041679380/1166914354051027024: addon mismatch message should show server's configSourceMod for common or main

Can you elaborate further on your suggestion? Do you mean only for ACE (so only in FUNC(checkFiles)) or for all mods (FUNC(checkPBOs))? Do you want the server's configSourceMod for main to show up on the client's error message?

@LinkIsGrim
Copy link
Contributor

LinkIsGrim commented Oct 26, 2023

ACE only. Server's source for main displayed on client (should be good enough to just setVariable?)

To clarify, there shouldn't be any errors on a source mismatch. But the source should be displayed if there is a version/addon mismatch.

@johnb432 johnb432 requested a review from LinkIsGrim April 4, 2024 17:00
@johnb432
Copy link
Contributor Author

johnb432 commented Apr 4, 2024

I've tested in self-hosted, I have not been able to test on a dedicated server.

@johnb432
Copy link
Contributor Author

I was able to test this on a dedicated server:

9:23:50 [ACE] johnb43: ERROR client missing addon(s): AR_AdvancedRappelling, AUR_AdvancedUrbanRappelling, SA_AdvancedSlingLoading, OCAP_main, OCAP_extension, OCAP_recorder
9:23:50 [ACE] johnb43: ERROR client additional addon(s): jsrs_soundmod_boats, jsrs_soundmod_bullethits, L_ES_ES_main, L_ES_ES_sounds, L_ES_ES_sys, jsrs_soundmod_cfg_rhs_afrf_weapons, jsrs_soundmod_cfg_rhs_afrf_vehicles, jsrs_soundmod_cfg_rhs_afrf_air_vehicles, jsrs_soundmod_cfg_rhs_usf_weapons, jsrs_soundmod_cfg_rhs_usf_vehicles, jsrs_soundmod_cfg_rhs_usf_air_vehicles, no_actions_main, tao_rewrite_main, turret_lock_main, Revo_EP, AIMEE_main, AIMEE_uav_terminal, AIMEE_vehicle_controls, AIMEE_vehicle_seats, bettinv_main, bettinv_main_ace, tfar_ace_extended_main, AIMEE_change_ammo, AIMEE_group, AIMEE_inventory, zeus_additions_main

It works as intended.

@johnb432 johnb432 added this to the 3.18.0 milestone Apr 19, 2024
Copy link
Contributor

@LinkIsGrim LinkIsGrim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still not completely happy with how we give feedback on this (we could add custom properties in config for the compats that'd override the error message to indicate client/server has extra CDLC, for example, instead of just saying there's extra/missing ACE addons), but this PR is good.

@johnb432 johnb432 merged commit 7ea2aab into acemod:master Jun 22, 2024
5 checks passed
@johnb432 johnb432 deleted the pbo-check-update branch June 22, 2024 13:53
blake8090 pushed a commit to blake8090/ACE3 that referenced this pull request Aug 18, 2024
* Update PBO checking

* Added kicking of clients without ACE loaded

* Update fnc_errorMessage.sqf

* Update fnc_checkVersionNumber.sqf

* More compatibility for acemod#9568

* Cleanup

* Minor cleanup + added server source

* update outdated/not present error message

* check version number fixes

* Update fnc_errorMessage.sqf

* Changed error names

Server is always right, client has either older or newer versions, or missing or additional addons

* Improved ACE detection method

* Tweaks and fixes

* Try another approach

* Update events-framework.md

* Update XEH_postInit.sqf

* Update fnc_checkVersionNumber.sqf

* Removed check for non-ACE clients

* Update XEH_postInit.sqf

* Cleanup

* Remove rogue change

* Improved message display in systemChat

* Update fnc_checkPBOs.sqf

* Removed loop variable initialisers

* Fixed header

* Updated headers

---------

Co-authored-by: Grim <69561145+LinkIsGrim@users.noreply.github.com>
Co-authored-by: LinkIsGrim <salluci.lovi@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement Release Notes: **IMPROVED:**
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ACE PBO Check, when set to "Kick", can be bypassed using an Xbox Controller
4 participants