Fix for weapon stacking behaviour

[BlackDog86]

Fixes #1429

This PR removes a check on number of upgrade slots from HasBeenModified. This was erroneously returning false for weapons where attachments are added by an onAcquired function andwith 0 upgrade slots on the template (e.g. TLE / chosen weapons), which would cause duplicates of those items to be stacked. This could cause downstream issues with attachments when equipping / removing them as the inventory functions expect all weapons with attachments to not be stacked.

[Larryturbo]

"items with identical attachments now stack together in the inventory"
As long as it can still see the weapon's colour and pattern as a difference. When multiple soldiers unequip their weapon for whatever reason, using the colour is how I can tell who dropped what.

[BlackDog86]

Will have to test that separately. There was no specific thought given to weapon customisation causing a weapon to become unique, so it's likely this would stack weapons with the same attachments but different customisations in the current state. I can fix this later but am away from pc for a week or so now so will take a look in the new year.

[faanlez]

HasBeenModified() does not check WeaponAppearance at all, it only cares about nickname and weapon upgrades. So with these changes identically upgraded weapons will stack and they will all get the same appearance. But, keep in mind that soldiers actually have their own weapon customization saved to them that they will use over the weapon's own customization unless HasBeenModified() returns true.

[Larryturbo]

Then upgraded and customized weapons will lose their appearance and stack. That should never be the case.

[BlackDog86]

Fair enough - It should be easy enough to revise to take account of customisation to be fair. If you force the behaviour to work 'correctly' & no-stack everything it makes a royal mess of the strategy debug start inventory and I do think having non unique items with the same attachments stacking makes sense (the original code in that function is downright awful) so I'd prefer to revise it that way than just blanket no-stack everything which isn't infinite but yeah - fixing the other two issues without playing with the item stacking behaviour is also perfectly possible.

[BlackDog86]

Updated PR now checks weapon appearances in the HasUnmodifiedItem function which is called when putting items back into the inventory so it ensures that upgraded weapons with custom appearances do not stack (non-upgraded weapons / infinite items continue to set the default weapon appearance on the soldier which is used if no custom appearance is already set on the weapon).

This means:
Two yellow magnetic rifles with a scope, = stack
1 x Blue and 1 x Yeloow magnetic rifle with a scope = no stack
Two yellow magnetic rifles, one with a scope and one with a stock = no stack

Does that seem reasonable? I've also added the chosen weapons to the 'OnEquipFix' array so their attachments don't get stripped (so if, for whatever reason mods decided to add multiple copies of the chosen weapons they wouldn't get stacked and have their attachments removed just like the TLE weapons).

[Iridar]

Haven't reviewed the PR yet, but I'm generally not a fan of any solution that involves maintaining config exclusion lists in the Highlander or else things break.

[BlackDog86]

Well the alternative is basically patching all of the weapons in X2Item_TLE_Weapons so that they fire the onAcquired function when you equip them, which is also a perfectly reasonable solution but I'm not sure if we're even allowed to modify that file since we don't have a 'TLE Highlander' as far as I know.

If I can't create a manual array of template names and I can't modify the templates, then I fail to see how the issue (1429) can be fixed - unless it can be done via OPTC and dumped out into an external fix mod.

[Iridar]

Why is simply not stacking weapons with any weapon upgrades attached is not an option?

[BlackDog86]

Of course, not stacking weapons with any upgrades is an option. Simply removing the check for number of slots in HasBeenModified() is sufficient to do that & implementing it that way also has the benefit of not needing to run the onacquired function when equipping the weapon (and thus no need for the config array), since the bit that strips the attachments in GetItemFromInventory can't happen if the weapons aren't stacked in the first place.

That said, when you just do the simple fix, what happens is HasBeenModified then correctly returns true for all items with attachments and it unstacks all the items created by strategy debug start (which is basically 10 x copies of every non-infinite item) so you get this:

I couldn't find the code that's responsible for doing this but if we can find it, perhaps we can use the event in HasBeenModified to over-ride the stacking behavior for those items.

Honestly, unstacking items in strat debug isn't a big deal for me & I'm perfectly happy to sacrifice a debug annoyance for better in-game control of the inventory so I can adjust the PR to just deal with the two explicit issues / bugs and not do anything regarding improving the item stacking behaviour, which I guess is probably the preferred approach.

[Iridar]

Yeah, that's better. I'll do a proper review later. To be clear, my main issue wasn't with configurable array itself, but more with filling it in Highlander. If you want, you can still add a configurable array of stackable items with upgrades, but it must be empty by default.

[Iridar]

And I'd probably prefer it done in a separate PR.

[BlackDog86]

Makes sense - it would be a lot easier, now I understand how these functions work, to make a configurable feature in a new PR, allowing items with identical attachments & appearance to stack. I agree it's better that way than trying to do too much at once.

[Iridar]

Good work, but needs hair-licking.