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

Arsenal - Improvements to 3DEN attribute #6849

Merged
merged 3 commits into from
Mar 12, 2019

Conversation

mharis001
Copy link
Member

When merged this pull request will:

  • Add various QOL improvements to 3DEN attribute
  • Ability to automatically add compatible attachments or magazines to list for all currently added weapons
    • Useful when playing with a lot of mods resulting in hundreds of magazines
    • Button is only visible for applicable categories
  • Ability to import an exported list
  • Last selected category is automatically selected on load
  • Right clicking search bar will clear it and set focus on it
  • Clicking the search button will set focus on the search bar
  • Preview:
    arsenal_attribute_5

@mharis001 mharis001 added the kind/enhancement Release Notes: **IMPROVED:** label Mar 9, 2019
Copy link
Contributor

@alganthe alganthe left a comment

Choose a reason for hiding this comment

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

All good, nice work 👍

private _muzzleConfig = if (_x == "this") then {_weaponConfig} else {_weaponConfig >> _x};

// Only add existent magazines and ensure correct classname case
private _magazines = getArray (_muzzleConfig >> "magazines") select {isClass (_cfgMagazines >> _x)};
Copy link
Contributor

Choose a reason for hiding this comment

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

CBA_fnc_compatibleMagazines?

Copy link
Contributor

@dedmen dedmen Mar 11, 2019

Choose a reason for hiding this comment

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

We have to. For magazineWells.

Edit: oh I see magazinewells are handles manually...
Might be better perf wise. As we can just reference the magazineGroups that arsenal already has. Which is also done here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Can change to avoid duplicate code. Will need to test the performance difference for a large weapons list.

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 this does match code in fillRightPanel
cba func does cache results, but on first call it probably will be more expensive

Copy link
Member Author

Choose a reason for hiding this comment

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

Did a quick test with ~400 weapons (RHS) and diag_tickTime, here are the results for the first and second clicks.
Current:

14:08:41 [ACE] (arsenal) INFO: Added compatible magazines (current) - Time: 36.9873 ms
14:09:05 [ACE] (arsenal) INFO: Added compatible magazines (current) - Time: 37.9944 ms

CBA_fnc_compatibleMagazines:

14:10:59 [ACE] (arsenal) INFO: Added compatible magazines (cba func) - Time: 67.0166 ms
14:11:13 [ACE] (arsenal) INFO: Added compatible magazines (cba func) - Time: 14.0076 ms

So not too big of a difference and this is also the extreme case of all weapons. Thoughts?

Copy link
Contributor

@dedmen dedmen Mar 12, 2019

Choose a reason for hiding this comment

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

Perf of both is fine for that.
CBA func uses additional memory for it's cache. But reduces code duplication.
So don't really know.
CBA func also filters out non existent magazines (which can occur in magazinewells) which you don't really do (configName will probably return "" so you'll have a bunch of empty strings)

Thinking about it. Does cache make so much sense for this? Do you really click that button that often? Most mission makers will probably make one arsenal object and then just copy it.

Copy link
Member Author

Choose a reason for hiding this comment

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

The arsenal magazine wells cache also filters out non-existent magazines aswell: https://github.com/acemod/ACE3/blob/master/addons/arsenal/functions/fnc_scanConfig.sqf#L180

Yeah, the performance is fine for both (and I can't see the button being clicked very often). The slowest part is L72 which in the above situation takes 900-1000 ms.

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 really need to worry about 35ms if you have a 1000ms laying around a couple lines lower.

It really doesn't matter. Whatever you prefer. Less code duplication is the only reason that I have left as a reason to change this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I think in that case it is fine as is.

@Drofseh
Copy link
Contributor

Drofseh commented Mar 11, 2019

Does the add compatible items button always set it to infinite?

@PabstMirror PabstMirror added this to the 3.12.6 milestone Mar 12, 2019
@mharis001
Copy link
Member Author

@Drofseh It just adds the items to the list, the mode controls whether the items added are whitelisted or blacklisted.

@PabstMirror PabstMirror merged commit 695416d into acemod:master Mar 12, 2019
@mharis001 mharis001 deleted the arsenal-attribute-qol branch March 12, 2019 16:43
BaerMitUmlaut pushed a commit that referenced this pull request Aug 5, 2019
* Improvements to arsenal 3DEN attribute

* Better alignment of button

* Remove magazineGroups copy
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.

5 participants