-
Notifications
You must be signed in to change notification settings - Fork 149
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
update CBA_fnc_uniqueUnitItems #1157
Conversation
_allItems pushBack (_unit call CBA_fnc_binocularMagazine); | ||
}; | ||
|
||
_allItems = _allItems - [""]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be cheaper to do it at the end, after duplicate removal.
because -
iterates over everything and copies everything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or if we know where it comes from (I guess primary/secondary/handgunWeapon) we can do it right there.
Would be cheaper to do in L58 for just the 3 things, than do it for everything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It comes from p/s/hWeapon
as well as p/s/hWeaponItems
. The magazine commands report empty arrays if no mag is loaded. I put the array subtraction below the last line that may add empty strings. CBA_fnc_binocularMagazine
reports empty string too.
The placement is so that the array subtraction command is only executed when the _weaponItems
flag is set, as this is the only place where the commands may add empty strings. Note that while p/s/hWeaponItems
has the items in order muzzle/pointer/optics/bipod and fills the gaps with empty strings, assignedItems, while having an order, just changes in array size (and indices) if a specific slot is unoccupied.
If I were to add it to the last line after the arrayIntersect
command instead, I would slow down the function for every use even when this flag is set to false. I know that a smaller array would speed up the function slightly, but this would mean adding an additional binary command (-
) for all uses for a small improvement in C++ that itself wouldn't even be a problem in all other use cases. The priority is clear to me.
The only potential optimization here would be moving the weapon(items/magazines) block above the uniform/vest/backpack block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another potential optimization approach would be creating the initial array as [""]
and then using pushBackUnique
everywhere. And then using deleteAt 0
at the end. However, this would need appendUnique
, because one source of empty strings are arrays here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe instead store _weaponItems seperately, then remove "" from it.
And only then append it to _allItems. That way you don't have to iterate over uniform/vest/backpack items to remove ""
and then using pushBackUnique everywhere
Nah not with the number of appends we have.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make a new array here
https://github.com/CBATeam/CBA_A3/pull/1157/files#diff-5281788de7c6251ec13f252782a5e72fR54
And then _allItems append _newArray
after removing the ""
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would mean more commands in total. I don't think there will be any case where this actually would be faster.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but only in the weaponsItems case
which I'd say is rare.
When merged this pull request will:
_magazines
flag, default false. Unless set to true, no mags will be reported (even with_weaponItems
set to true)_magazines
flag will not report loaded mags unless_weaponItems
flag is also set to true.binocular
command is not needed (covered byassignedItems
)