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 CBA_fnc_compatibleMagazines that supports mag wells #965

Merged
merged 4 commits into from
Aug 25, 2018
Merged

Conversation

PabstMirror
Copy link
Contributor

Because of mag wells it's no longer safe to do simple code like
private _weaponMagazines = getArray (configFile >> "CfgWeapons" >> _weapon >> "magazines");

The filtering gets expensive, and might not always be needed, so I added a param for it
rhs_weap_m27iar_grip with 57 filtered entries is 0.26 ms
fast way is 0.028 ms but produces 69 entries because of duplicates and no guarantees that in is safe to use

@commy2
Copy link
Contributor

commy2 commented Aug 21, 2018

For consistency with compatibleItems, I would change the name to compatibleMagazines (remove "get").

I think it would be worth it to implement caching:
https://github.com/CBATeam/CBA_A3/blob/master/addons/jr/fnc_compatibleItems.sqf#L30-L36
https://github.com/CBATeam/CBA_A3/blob/master/addons/jr/fnc_compatibleItems.sqf#L62

@commy2
Copy link
Contributor

commy2 commented Aug 21, 2018

The function should also report all entries lower case.
And with caching implemented, it should always filter duplicates. Not sure about non existing. Usually that is something that should error, although since that is a feature of magwells, it probably should just always filter them out if they're not present.

@commy2 commy2 added this to the 3.8.1 milestone Aug 21, 2018
@PabstMirror
Copy link
Contributor Author

PabstMirror commented Aug 21, 2018

I thought it would be harder to cache because I wanted to support sub-muzzles like
configFile >> "CfgWeapons" >> "m4" >> "M203", but it was pretty easy

I like config capitalization because you can use in but findIf {_x == z} is fine as well; I can change if needed.

edit, also should this go in addons/jr, next to fnc_compatibleItems? It's not really JR related.

@commy2
Copy link
Contributor

commy2 commented Aug 21, 2018

I guess the JAM component, but that doesn't exist yet. Common is fine for now imo.
Config case is good too and would be consistent with compatibleItems, but it seems that most of the time BI uses all lower case. E.g. how they changed getAllHitPointsDamage to all lower case.

@commy2
Copy link
Contributor

commy2 commented Aug 21, 2018

All lower case means toLower _mag in _mags is possible.
Config case this would look:
configName (configFile >> "CfgMagazines" >> _mag) in _mags and I don't think many people would use that, despite it being not all that bad.

@PabstMirror
Copy link
Contributor Author

@commy2
Copy link
Contributor

commy2 commented Aug 21, 2018

Whatever we go with, it should be clarified in the function header.

@dedmen
Copy link
Contributor

dedmen commented Aug 22, 2018

ACE Arsenal would prefer configCase I think.
People can always make it lowercase quite easily and quickly if they need it.
However turning it back to config case isn't that easy nor quick.
And why make this function slower for everyone (yes I know caching n stuff) if the user doesn't really want it.

Function: CBA_fnc_compatibleMagazines

Description:
Retrievs a list of magazines that are compatible with a weapon.
Copy link
Contributor

Choose a reason for hiding this comment

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

Retrieves


private _returnMags = GVAR(magNamespace) getVariable _cacheKey;

if (isNil "_returnMags") then {
Copy link
Contributor

@Dystopian Dystopian Aug 22, 2018

Choose a reason for hiding this comment

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

this could be replaced with early return
if (!isNil "_returnMags") exitWith {_returnMags};
if (!isNil "_returnMags") exitWith {+_returnMags};

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't forget to copy the returned array, otherwise someone will mess up the cache by editing it by reference.

@commy2 commy2 changed the title Add CBA_fnc_getCompatibleMagazines that supports mag wells Add CBA_fnc_compatibleMagazines that supports mag wells Aug 25, 2018
@commy2 commy2 merged commit a88fc7b into master Aug 25, 2018
@commy2 commy2 deleted the getMagsWell branch August 25, 2018 10:33
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