Destroy cosmetic pawns when items are unequipped #1244
Conversation
Added to XCGS_Unit to remove cosmetic pawns when items are removed from a unit's loadout.
Tedster59
added
bug-basegame
ready-to-review
There was a problem hiding this comment.
@Tedster59 I've made some adjustments to the PR, see the comments for details. I tested and it still works fine. Please take a look to see if you agree with my changes, and if all is good, I'll squash merge this.
| @@ -259,7 +259,8 @@ simulated function XComUnitPawn GetCosmeticPawnInternal(out array<PawnInfo> Pawn | |||
| return PawnStore[StoreIdx].Cosmetics[CosmeticSlot].Pawn; | |||
| } | |||
|
|
|||
| /// Start issue #1108 - new function to destroy cosmetic pawns. INTERNAL CHL API, not covered by BC policy. | |||
| // Start issue #1108 | |||
| // Destroy a cosmetic pawn associated with a particular inventory slot on a particular unit, if there is one. | |||
There was a problem hiding this comment.
It's preferable to keep comments to the point, just explaining what the function does, giving its history unless it's relevant is unnecessary.
I for one got mislead by "use the new function", which can be interpreted as there's an old version of DestroyCosmeticPawn().
I also decided to remove the mention of "internal CHL API", because that would imply we don't want mods using this new function. I don't see any harm in keeping it open, though I confess I can't come up with a potential use case on the spot.
| @@ -290,8 +291,7 @@ simulated final function bool DestroyCosmeticPawn_CH(int CosmeticSlot, int UnitR | |||
| return false; | |||
| } | |||
|
|
|||
| // Helper function used by DestroyCosmeticPawn_CH. Also internal only, and not covered by BC policy. | |||
| simulated final function bool DestroyCosmeticPawnInternal_CH(out array<PawnInfo> PawnStore, int CosmeticSlot, int StoreIdx) | |||
| simulated private function bool DestroyCosmeticPawnInternal_CH(out array<PawnInfo> PawnStore, int CosmeticSlot, int StoreIdx) | |||
There was a problem hiding this comment.
Now that's an actual internal CHL API function, but in this case just adding private is enough to imply that, per Xym's guidelines.
| @@ -8485,9 +8485,6 @@ simulated function bool RemoveItemFromInventory(XComGameState_Item Item, optiona | |||
| local X2ArmorTemplate ArmorTemplate; | |||
| local int RemoveIndex; | |||
|
|
|||
| // Variable for Issue #1108 | |||
| local UIPawnMgr PawnMgr; | |||
There was a problem hiding this comment.
Since you're not gonna be none-checking the UIPawnMgr - and the user has bigger problems then accessed-none log warnings if UIPawnManager is missing - I decided to get rid of the local var for code brevity.
| } | ||
| } | ||
|
|
||
| /// HL-Docs: ref:Bugfixes; issue:1108 |
There was a problem hiding this comment.
In general we do want to ship most submissions with a HL-Docs entry, one was missing from this PR.
|
|
||
| /// HL-Docs: ref:Bugfixes; issue:1108 | ||
| /// If there is a cosmetic pawn associated with the unequipped item item, remove it. | ||
| `PRESBASE.GetUIPawnMgr().DestroyCosmeticPawn_CH(Item.InventorySlot, self.ObjectID); |
There was a problem hiding this comment.
As I understand it, there are two main presentation layers, HQPRES for strategy and PRES for tactical. Both of them extend PRESBASE. Using PRESBASE macro will yield you either HQPRES or PRES depending on where you are.
Since GetUIPawnMgr() lives in PRESBASE, and it is technically possible to unequip items in tactical, I think it's more appropriate to use PRESBASE here.
Iridar
added
ready-for-merge
|
Approved by Tedster, merging. |
Fixes Issue #1108 by creating a new function
DestroyCosmeticPawn_CHthat is modeled on the existingGetCosmeticPawnfunction, but destroys the requested cosmetic pawn instead of returning it. This function is now called byRemoveItemFromInventoryonXComGameState_UnitNew PR to use proper rebased branch with cleaned up commits.