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

Add optional mod to skip missing Mod check #866

Merged
merged 5 commits into from
Apr 29, 2018

Conversation

dedmen
Copy link
Contributor

@dedmen dedmen commented Dec 31, 2017

Provides the option to disable CBA versioning's missing Mod check which would alert everyone on clientside if client is missing a Mod that uses CBA versioning.

Allows Servers to use CBA (PFH, local events, stuff) in serverside only scripts without annoying each client with a useless message.

@@ -20,6 +20,9 @@ FUNC(paranoid) = {

QGVAR(versions_serv) addPublicVariableEventHandler { (_this select 1) call FUNC(paranoid) };

// Skip missing mod check if it is disabled.
if (isNumber (configFile >> "CBA_skipMissingModCheck") && {getNumber (configFile >> "CBA_skipMissingModCheck") == 1}) exitWith {};
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the isNumber check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if the getNumber could cause errors. But as I am writing this right now I remember the talk from a couple days ago. Fix incoming

jonpas
jonpas previously requested changes Dec 31, 2017
Copy link
Member

@jonpas jonpas left a comment

Choose a reason for hiding this comment

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

Better name for optional would be missing_mod_check_disable or disable_missing_mod_check to go along with other optionals naming.

units[] = {};
weapons[] = {};
requiredVersion = REQUIRED_VERSION;
requiredAddons[] = {"CBA_Extended_EventHandlers", "CBA_Versioning"};
Copy link
Member

Choose a reason for hiding this comment

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

Should be lower-case really.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copied from cache disable

Copy link
Member

Choose a reason for hiding this comment

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

I figured, still wrong, components are all lower-case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Uppercase CBA looks good to me tho. But the uppercase Versioning already looked weird when I wrote it.

Copy link
Member

@jonpas jonpas Dec 31, 2017

Choose a reason for hiding this comment

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

But it's still wrong, actual component is cba_versioning. ¯\_(ツ)_/¯
Doesn't matter with Arma though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

I guess, bet there is a component that doesn't respect it. 😆

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#endif

#undef REQUIRED_VERSION
#define REQUIRED_VERSION 1.00
Copy link
Member

Choose a reason for hiding this comment

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

Pointless, this optional requires 2 other components which have required version set from main\script_mod.hpp anyways.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same for cache disable then

#include "\x\cba\addons\main\script_mod.hpp"


#ifdef DEBUG_ENABLED_CACHE_DISABLE
Copy link
Member

Choose a reason for hiding this comment

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

Should be #ifdef DEBUG_ENABLED_NO_MISSING_MOD_CHECK.

#define DEBUG_MODE_FULL
#endif

#ifdef DEBUG_SETTINGS_CACHE_DISABLE
Copy link
Member

Choose a reason for hiding this comment

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

Should be #ifdef DEBUG_ENABLED_NO_MISSING_MOD_CHECK.

#endif

#ifdef DEBUG_SETTINGS_CACHE_DISABLE
#define DEBUG_SETTINGS DEBUG_SETTINGS_CACHE_DISABLE
Copy link
Member

Choose a reason for hiding this comment

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

Should be #define DEBUG_SETTINGS DEBUG_SETTINGS_NO_MISSING_MOD_CHECK.

@commy2
Copy link
Contributor

commy2 commented Dec 31, 2017

I think the name missing_mod_check is weird. It's atm called "versioning".
Disabling it completely is a bit odd. Someone could still want it, but only ignore one certain addon.
Btw. which addon are we talking about? Because unlike ACE_checkPBO, this only works for addons that have it enabled via config. Shouldn't that optional addon be disabled instead?

@commy2 commy2 added this to the 3.7 milestone Dec 31, 2017
@dedmen
Copy link
Contributor Author

dedmen commented Dec 31, 2017

I don't think I disable versioning completly (https://github.com/CBATeam/CBA_A3/blob/master/addons/versioning/XEH_postInitClient.sqf <- That is version comparison). I only disable the missing mod check.
I basically want CBA on the Server without telling clients they are missing it.
So I guess this is about disabling the check for all of CBA.

@commy2 commy2 self-assigned this Feb 13, 2018
@commy2
Copy link
Contributor

commy2 commented Feb 13, 2018

Related: #871

commy2
commy2 previously approved these changes Feb 13, 2018
@commy2 commy2 dismissed jonpas’s stale review February 13, 2018 17:44

name was changed

@jonpas
Copy link
Member

jonpas commented Feb 14, 2018

I still don't see why it has to be an optional PBO, when it could be just a config entry which people can define in description.ext as well. Or a setting itself as @commy2 suggested on Slack.

In short it should literally be identical to enableTargetDebug (which could also be a setting).

@commy2 commy2 dismissed their stale review February 14, 2018 19:22

see jonpa's comment

@dedmen
Copy link
Contributor Author

dedmen commented Feb 15, 2018

So what now? CBA setting or config? Or both?
I'd say CBA setting is enough. But it would be a serverside only setting. And would that be the first CBA setting inside CBA itself? :D

@commy2 commy2 modified the milestones: 3.7, 3.8 Apr 14, 2018
@dedmen
Copy link
Contributor Author

dedmen commented Apr 20, 2018

A CBA setting for this would be "confusing" because it's only relevant on serverside. Meaning it shouldn't show up on client/mission settings which afaik isn't supported in CBA settings currently.

Also to enable the setting you would then either need a serverside userconfig for the settings or you'd have to connect with CBA enabled on clientside and then configure it. Which is a chore on a vanilla server with bisigning enabled.

Jonpas to do it the same way as enableTargetDebug which is a mission specific attribute.
I don't think a thing that is hidden on the server-backend should be a mission attribute that you have to set in every mission.
My concern is server-side only mods that require CBA. You shouldn't have to edit every of your missions just to run a server-side mode that might have nothing to do with the mission.

I'm still for optional-pbo. In my case I want this for server-side Intercept. And as all users already need a PBO with config to register their plugin they could also add the config entry into their pbo instead of loading the CBA optional.
The optional PBO also shows everyone that, that feature exists. And people might also want to use that outside of just serverside intercept and without having to build their own pbo.

I guess it wouldn't hurt to make it configFile or missionConfigFile. Just to let all options open.

@commy2 commy2 modified the milestones: 3.8, 3.7 Apr 29, 2018
@Killswitch00 Killswitch00 merged commit 920dc5f into CBATeam:master Apr 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants