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.

Sign up for GitHub

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

Jump to bottom

Destroy cosmetic pawns when items are unequipped #1244

Merged
merged 2 commits into from
Aug 19, 2023
Merged

Destroy cosmetic pawns when items are unequipped #1244

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1016b3d
New function in UIPawnMgr to destroy cosmetic pawns on request
Tedster59 Aug 14, 2023
e10fe7f
Adjusted comments, using PRESBASE
ps2guides Aug 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions X2WOTCCommunityHighlander/Src/XComGame/Classes/UIPawnMgr.uc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

simulated final function bool DestroyCosmeticPawn_CH(int CosmeticSlot, int UnitRef, bool bUsePhotoboothPawns = false)
{
local int PawnInfoIndex;
Expand Down Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

{
local bool bSuccess;

Expand Down
14 changes: 5 additions & 9 deletions X2WOTCCommunityHighlander/Src/XComGame/Classes/XComGameState_Unit.uc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

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

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.


if (CanRemoveItemFromInventory(Item, ModifyGameState))
{
RemoveIndex = InventoryItems.Find('ObjectID', Item.ObjectID);
Expand All @@ -8512,15 +8509,14 @@ simulated function bool RemoveItemFromInventory(XComGameState_Item Item, optiona
{
`RedScreen("Attempt to remove item" @ Item.GetMyTemplateName() @ "properly may have failed due to OnUnequippedFn -jbouscher @gameplay");
}
}
}

/// HL-Docs: ref:Bugfixes; issue:1108
Copy link
Contributor

Choose a reason for hiding this comment

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

In general we do want to ship most submissions with a HL-Docs entry, one was missing from this PR.

/// If there is a cosmetic pawn associated with the unequipped item item, remove it.
`PRESBASE.GetUIPawnMgr().DestroyCosmeticPawn_CH(Item.InventorySlot, self.ObjectID);
Copy link
Contributor

Choose a reason for hiding this comment

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

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.


if (RemoveIndex != INDEX_NONE)
{
// Start Issue #1108
// Remove Cosmetic Unit when the item is removed, using newly created function in UIPawnMgr.
PawnMgr = `HQPRES.GetUIPawnMgr();
PawnMgr.DestroyCosmeticPawn_CH(Item.InventorySlot, self.ObjectID);
// End Issue #1108
InventoryItems.Remove(RemoveIndex, 1);
}

Expand Down
Loading