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

Refactor private ARRAY to private keyword #5598

Merged
merged 6 commits into from
Oct 10, 2017
Merged

Conversation

dedmen
Copy link
Contributor

@dedmen dedmen commented Oct 6, 2017

#5566 (review)
#5566 (comment)
This PR does what the titel says.
Also it fixed a typo _atached to _attached
and fixes a minor bug from #5566

@jonpas jonpas added the kind/cleanup Release Notes: **CHANGED:** label Oct 6, 2017
@jonpas jonpas added this to the 3.11.0 milestone Oct 6, 2017

_cloudType = "";
private private _cloudType = "";
Copy link
Contributor

Choose a reason for hiding this comment

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

double private

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I got a double private there it means I forgot a private below.. Sometimes I would click a pixel to far left and hit the line number instead of the text... Atom is somewhat crap I guess.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Jup. _surface on line 51 was missing


_number = _this select 0;
_coordinate = _this select 1;
params ["_number", "_coordinate"]
Copy link
Contributor

Choose a reason for hiding this comment

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

missing end semi-colon ;

@@ -19,7 +19,7 @@

#define EMP_RF_ACC 5 // Rangefinder Accuracy

PARAMS_3(_slopeDistance,_azimuth,_inclination);
params ["_slopeDistance", "_azimuth", "_inclination"]
Copy link
Contributor

Choose a reason for hiding this comment

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

missing end semi-colon ;

@commy2
Copy link
Contributor

commy2 commented Oct 7, 2017

Did you make sure this doesn't break anything? Sometimes you need a variable in main scope and setting it to private in a then-block or something would make it nil there later.

@dedmen
Copy link
Contributor Author

dedmen commented Oct 7, 2017

@commy2 I payed attention to that. That's why I initialized some variables in main scope with objNull or controlNull or even scriptNull in one case.
I already noticed some missing semicolons.. But then it was already too late.. My regex skills weren't enough to find them for me. SQF validator from build script found one so I thought the rest was also correct...

I'll do a full build and deploy it to my group. We'll test it on todays mission.

@dedmen
Copy link
Contributor Author

dedmen commented Oct 7, 2017

Turns out project file regex search in Atom is horribly broken.. Notepad++ found the missing semicolons just fine

@bux
Copy link
Member

bux commented Oct 7, 2017

Atom

https://code.visualstudio.com/

@dedmen
Copy link
Contributor Author

dedmen commented Oct 7, 2017

No compilation errors. Running around a bit and taking some AB shots no errors anywhere.
Will have to wait till after the mission this evening.

@dedmen
Copy link
Contributor Author

dedmen commented Oct 7, 2017

Wow. isNull NUMBER -> script error... GG Arma.

@@ -103,7 +103,7 @@ if (GVAR(bulletTraceEnabled) && cameraView == "GUNNER") then {

private _stabilityFactor = 1.5;
if (_caliber > 0 && _bulletLength > 0 && _bulletMass > 0 && _barrelTwist > 0) then {
if (isNull _temperature) then {
if (_temperature isEqualTo scriptNull) then {//isNull doesn't work with Numbers
Copy link
Contributor

@commy2 commy2 Oct 7, 2017

Choose a reason for hiding this comment

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

Elaborate? SCRIPT type is different from NUMBER. Although isNull does indeed not work with NUMBER, it should work with SCRIPT just fine.
https://community.bistudio.com/wiki/isNull
Therefore, _temperature isEqualTo scriptNull should be isNull _temperature, if _temperature actually is a SCRIPT that is. If it's a NUMBER, then it will never be a null script 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.

I expect to have isNull check if variable contains a null type. And return false if it's not null. But instead of returning false on a number it just errors.
_temperature is by default set to scriptNull. So here I just check if it's still default(undefined)

if (GVAR(barrelLengthInfluenceEnabled)) then {
_barrelVelocityShift = [_barrelLength, _muzzleVelocityTable, _barrelLengthTable, _muzzleVelocity] call FUNC(calculateBarrelLengthVelocityShift);
};

_ammoTemperatureVelocityShift = 0;
private _ammoTemperatureVelocityShift = 0;
private _temperature = scriptNull; //Need the variable in this scope. So we need to init it here.
Copy link
Contributor

@commy2 commy2 Oct 7, 2017

Choose a reason for hiding this comment

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

I get it now. Can you just use:

private "_temperature";

or

private _temperature = nil;

Instead? This is really ugly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was told _temperature = nil; works. But I wasn't sure. And didn't have time to test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If it works then sure.
private "_temperature" would be as bad as private ARRAY

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just tested. It works. Atleast in debug console.. Might break in scheduled but handleFired is unscheduled anyway

if (_caliber > 0 && _bulletLength > 0 && _bulletMass > 0 && _barrelTwist > 0) then {
if (isNil "_temperature") then {
if (_temperature isEqualTo scriptNull) then {//isNull doesn't work with Numbers
Copy link
Contributor

Choose a reason for hiding this comment

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

And revert this change of course.

@dedmen
Copy link
Contributor Author

dedmen commented Oct 7, 2017

Just played 3 hours no errors.
Though without the new change for _temperature.

@commy2
Copy link
Contributor

commy2 commented Oct 7, 2017

I can vouch for those two lines^^

@@ -80,7 +80,7 @@ class RscACE_SelectTimeUI {
y = 0.06;
w = 0.49;
h = 0.025;
onSliderPosChanged = "private ['_mins', '_secs'];_mins = floor((_this select 1)/60);_secs=floor((_this select 1) - (_mins*60));ctrlSetText [8870, format[localize 'STR_ACE_Explosives_TimerMenu',_mins, _secs]];";
onSliderPosChanged = "_mins = floor((_this select 1)/60);_secs=floor((_this select 1) - (_mins*60));ctrlSetText [8870, format[localize 'STR_ACE_Explosives_TimerMenu',_mins, _secs]];";
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess these aren't private, but I don't think it matters for BIS event handlers as there shouldn't be any upper scope variables (I hope)

Copy link
Member

Choose a reason for hiding this comment

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

They should still be privatized.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly why I removed that. There is no upper scope. So private does nothing in this case. Besides making the string longer. Thus increasing compileTime. And as this is a string eventhandler that get's compiled before every usage...

@@ -24,7 +24,7 @@ class CfgMagazines {
count = 1;
mass = 85;
pylonWeapon = QGVAR(launcher);
hardpoints[] = {"B_MISSILE_PYLON", "SCALPEL_1RND_EJECTOR", "B_ASRRAM_EJECTOR", "UNI_SCALPEL", "CUP_NATO_HELO_SMALL", "CUP_NATO_HELO_LARGE", "RHS_HP_MELB"};
Copy link
Contributor

Choose a reason for hiding this comment

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

Whats going on here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wat? wtf

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. And I just skimmed over all changes again and it looks like only my changes are left.

Copy link
Contributor

Choose a reason for hiding this comment

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

make sure to push the changes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

XD I did push... But I didn't commit before
Pushed now.. I will go to bed now..

Copy link
Contributor

@PabstMirror PabstMirror left a comment

Choose a reason for hiding this comment

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

looked over all changes,
fix the hellfire magazines (probably was just a merge conflcit)
and this should be ok to merge

@@ -16,11 +16,9 @@

#include "script_component.hpp"

private ["_settingIndex", "_inputText", "_setting", "_settingName", "_convertedValue"];
private _inputText = ctrlText 414; //Index of right drop down
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sometimes the diffing algo is confused....

@PabstMirror PabstMirror merged commit 81e02a7 into acemod:master Oct 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/cleanup Release Notes: **CHANGED:**
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants