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 Target Debug run with addon config #724

Merged
merged 2 commits into from
Nov 18, 2017

Conversation

Dystopian
Copy link
Contributor

@Dystopian Dystopian commented Jul 9, 2017

Fixes #712.
Addon config has a greater priority than mission one.
Also function description fixed.

@jonpas
Copy link
Member

jonpas commented Jul 9, 2017

What about the new array syntax: enableDebugConsole[] = {"SteamID1", "SteamID2", ...}?

Honestly I'd like an option to just have it the same as the vanilla debug console, so we don't have to write things twice all the time.

@Dystopian
Copy link
Contributor Author

Target Debugging is only available, when the Debug Console is available

So Debug console checks steam IDs if they exists. Why do you need to define different ways for both consoles? If you insist I can implement it.

@jonpas
Copy link
Member

jonpas commented Jul 9, 2017

Oh right, never mind! :P

@@ -18,7 +18,8 @@ GVAR(projectileTrackedUnits) = [];

ADDON = true;

if (getMissionConfigValue ["EnableTargetDebug", 0] isEqualTo 1) then {
private _cfg = configFile >> "EnableTargetDebug";
if (1 isEqualTo ([getMissionConfigValue ["EnableTargetDebug", 0], getNumber _cfg] select isNumber _cfg)) then {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be:

getMissionConfigValue ["EnableTargetDebug", getNumber (configFile >> "EnableTargetDebug")] isEqualTo 1

imo. There is no reason for an addon to overwrite the mission if the mission sets the value explicitly.

@@ -17,7 +17,8 @@ Author:

#define COUNT_WATCH_BOXES 8

if (!((getMissionConfigValue ["EnableTargetDebug", 0]) isEqualTo 1)) exitWith {};
private _cfg = configFile >> "EnableTargetDebug";
if !(1 isEqualTo ([getMissionConfigValue ["EnableTargetDebug", 0], getNumber _cfg] select isNumber _cfg)) exitWith {};
Copy link
Contributor

Choose a reason for hiding this comment

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

same

@commy2
Copy link
Contributor

commy2 commented Jul 11, 2017

Addon config has a greater priority than mission one.

Why? Nowhere else is that the case.

@Dystopian
Copy link
Contributor Author

Dystopian commented Jul 11, 2017

Target Debug is server admin/developer tool disabled by default. Security questions are solved with vanilla enableDebugConsole. There are no any reasons to disable it in mission config. Addon priority is for those times when you play some mission where you need to debug something right now (multiplayer missions are difficult to debug, so sometimes you have to debug in playtime) but for some reason the author disabled it (by chance or purposely) (like BI disabled enableDebugConsole in its coop missions).

@Dorbedo
Copy link
Contributor

Dorbedo commented Jul 11, 2017

I personally don't agree with this.
If the missiondesigner has disabled the feature on purpose, his decision should be respected and not be overwritten by an addon.
If the missiondesigner didn't use the value, the Addon specified value should be used instead.
Just like commys suggestion.

@Dystopian
Copy link
Contributor Author

Dystopian commented Jul 12, 2017

What purpose can it be? As server admin and addon tester/developer I can't agree.

You should understand that Target Debug is not user feature like ACE Cargo e.g., which can be disabled in favor of some mission features. This is an admin feature and mission designer is not that man who finally decides.

@commy2
Copy link
Contributor

commy2 commented Jul 12, 2017

It should work the same way as vanilla does described here: #712
I don't know which way that is.

@Dorbedo
Copy link
Contributor

Dorbedo commented Jul 12, 2017

BIS-Wiki Vanilla order

1. Mission param is checked. If is not defined then...
2. description.ext param enableDebugConsole is checked. If is not defined then...
3. Eden attribute option is checked. If not defined then...
4. Global/mod param enableDebugConsole is checked.

@commy2
Copy link
Contributor

commy2 commented Jul 12, 2017

So mission config beats addon config. Just like everything else. It's a good rule to go by. It seems to me like this PR does it the opposite way though.

@kymckay
Copy link
Contributor

kymckay commented Jul 12, 2017

I sort of don't see the point in an addon being able to enable the debug console if the mission can just override it anyway 🤔 (More of a comment on the vanilla behaviour than anything else)

@commy2
Copy link
Contributor

commy2 commented Jul 12, 2017

I guess it's more convenient to be able to change the default behavior by addon.
I'd say that almost all missions that "disable" the debug console, actually just didn't bother adding the entry. And if it's disabled explicitly by the mission, then that should overrule the addon.

@PabstMirror
Copy link
Contributor

Yeah, I'd say if either one explicitly disables, then it should be disabled

private _cfg = configFile >> "EnableTargetDebug";
private _blocked = ((isNumber _cfg) && {(getNumber _cfg) isEqualTo 0}) || {(getMissionConfigValue ["EnableTargetDebug", -999]) isEqualTo 0};
if ((!_blocked) && {1 isEqualTo ([getMissionConfigValue ["EnableTargetDebug", 0], getNumber _cfg] select isNumber _cfg)}) then {
    ...

@Dystopian
Copy link
Contributor Author

So nobody answered my question about mission maker purpose to disable Target Debug. I gave 2 arguments for addon priority and nobody said anything against.

About enableDebugConsole analogue: as I saw in dev version src a week ago after T124835, it looks like mission value WILL NOT beat addon value. I don't know if it's changed in release version - we'll see in 1.74.

@Dorbedo
Copy link
Contributor

Dorbedo commented Jul 14, 2017

I think there is no good reason for overruling the missionmakers decision. If you have the permission of the missioncreator, you could modify the missionfile very easily. If not, you should respect the decision.

I don't see a reason to make the behavior different from the vanilla behavior. This makes things much more complicated.
Until the vanilla behavior is not changed, CBA should stick with mission priority over addon.

I must add, that we are talking about a very few cases. I think, there will almost never be someone allowing the debug console, but disallowing the target debugging.

@Dystopian
Copy link
Contributor Author

We have this in vanilla 1.76:

_chk = getMissionConfigValue ["enableDebugConsole", 0]; 
if !(_chk isEqualTo 0) exitWith {_chk};
...
_cfg = configFile >> "enableDebugConsole"; 
if (!isNull _cfg) exitWith {_cfg call _fnc_getConfigValue};

Mission value has priority only if > 0.

@commy2
Copy link
Contributor

commy2 commented Sep 9, 2017

BIS_fnc_isDebugConsoleAllowed is a mess now.

@commy2 commy2 self-assigned this Sep 9, 2017
@Dystopian
Copy link
Contributor Author

updates on this?

@commy2 commy2 added this to the 3.5 milestone Nov 18, 2017
@commy2 commy2 merged commit 519c811 into CBATeam:master Nov 18, 2017
@commy2 commy2 added the Feature label Nov 18, 2017
@Dystopian Dystopian deleted the addTargetDebugAddonCfg branch November 18, 2017 22:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add addon activation of extended MP debug console
6 participants