New DLC hook ModifyGeneratedUnitAppearance

[Iridar]

Fixes #783

Initially I wanted to address this with an Event, but ran into the issue mentioned in #297; some soldiers (new rookies added to the HQ roster on the campaign start) had their appearance generated before the Event Listener had a chance to register.

So I added a DLC hook instead, which is functionally the same, because the Event Listeners would have to use ELD_Immediate in order to make any changes to the generated appearance anyway.

The hook code runs at the very end of XGCharacterGenerator::CreateTSoldier().

The Resistance Faction soldiers (Reaper, Skirmisher, Templar) in their own CreateTSoldier() do a super.CreateTSoldier(), and then modify the country and generate the nickname for the generated soldier, which would mean any changes to these aspects of the soldier appearance in the DLC hook would be disregarded.

Example of the original code:

function TSoldier CreateTSoldier(optional name CharacterTemplateName, optional EGender eForceGender, optional name nmCountry = '', optional int iRace = -1, optional name ArmorName)
{
	local X2SoldierClassTemplateManager ClassMgr;
	local X2SoldierClassTemplate ClassTemplate;

	kSoldier = super.CreateTSoldier('ReaperSoldier', eForceGender, nmCountry, iRace, ArmorName);
	SetCountry('Country_Reaper');
	ClassMgr = class'X2SoldierClassTemplateManager'.static.GetSoldierClassTemplateManager();
	ClassTemplate = ClassMgr.FindSoldierClassTemplate('Reaper');
	kSoldier.strNickName = GenerateNickname(ClassTemplate, kSoldier.kAppearance.iGender);

	return kSoldier;
}

So in order to facilitate better compatibility with the DLC hook, I have added their character generator classes to the Highlander and refactored them along these lines:

function TSoldier CreateTSoldier(optional name CharacterTemplateName, optional EGender eForceGender, optional name nmCountry = '', optional int iRace = -1, optional name ArmorName)
{
	kSoldier = super.CreateTSoldier('ReaperSoldier', eForceGender, nmCountry, iRace, ArmorName);
	return kSoldier;
}

function SetCountry(name nmCountry)
{
	kSoldier.nmCountry = 'Country_Reaper';
	kSoldier.kAppearance.nmFlag = kSoldier.nmCountry; // needs to be copied here for pawns -- jboswell
}

function GenerateName( int iGender, name CountryName, out string strFirst, out string strLast, optional int iRace = -1 )
{
	local X2SoldierClassTemplateManager ClassMgr;
	local X2SoldierClassTemplate ClassTemplate;

	super.GenerateName( kSoldier.kAppearance.iGender, kSoldier.nmCountry, kSoldier.strFirstName, kSoldier.strLastName, kSoldier.kAppearance.iRace );

	ClassMgr = class'X2SoldierClassTemplateManager'.static.GetSoldierClassTemplateManager();
	ClassTemplate = ClassMgr.FindSoldierClassTemplate('Reaper');
	kSoldier.strNickName = GenerateNickname(ClassTemplate, kSoldier.kAppearance.iGender);
}

The SetCountry() and GenerateName() functions will be called by the super.CreateTSoldier(). This approach is already used by the SPARK character generator class; it is weird the Firaxis didn't use the same approach for the resistance faction soldiers.

The Skirmisher character generator in particular contained a seemingly completely redundant call for GenerateName() which would have already been called in the exact same way by the super.CreateTSoldier() called a few moments before; it appears not a lot of care was given to this particular part of the code. There is a lot of potential for further refactoring, but I don't wanna fix what ain't broke without a good reason.

[Iridar]

I've confirmed that my additions function properly with this simple example:

static function ModifyGeneratedUnitAppearance(XGCharacterGenerator CharGen, optional name CharacterTemplateName, optional EGender eForceGender, optional name nmCountry = '', optional int iRace = -1, optional name ArmorName, optional XComGameState_Unit UnitState, optional XComGameState UseGameState)
{
	if (CharacterTemplateName == 'Soldier')
	{
		CharGen.kSoldier.kAppearance.nmLeftArmDeco = '';
		CharGen.kSoldier.kAppearance.nmRightArmDeco = '';
		CharGen.kSoldier.kAppearance.nmLeftForearm = '';
		CharGen.kSoldier.kAppearance.nmRightForearm = '';
		CharGen.kSoldier.kAppearance.nmThighs = '';
		CharGen.kSoldier.kAppearance.nmTorsoDeco = '';
		CharGen.kSoldier.kAppearance.nmShins = '';
	}
}

Once (if) this PR goes live, I'm gonna refactor my Clean Soldier Generation mod so it can work without Mod Class Overrides and in a more predictable and streamlined manner.

[MrNiceUK]

#297 isn't directly applicable, any shell listeners would have already been unregistered as well, the issue was that starting rookies are generated before strategy listeners are registered.

[Iridar]

#297 isn't directly applicable, any shell listeners would have already been unregistered as well, the issue was that starting rookies are generated before strategy listeners are registered.

Didn't think they'd get unregistered. Then yeah.

[Iridar]

@Musashi1584 Please review this again when you have a moment.

[Iridar]

Sorry for the mess, deleted branch by accident.

[pledbrook]

This seems good to me, although I don't really understand the rationale for the faction soldier refactor. It seems particularly bizarre for SetCountry() to completely ignore its argument and just set a hard-coded country.

[Iridar]

The status shows "Changes requested", but they have already been applied. I have squashed most of the commits in this PR. If everything is in order, I'm ready to mark this "ready to merge".

[robojumper]

Approach LGTM.

[robojumper]

I still think these arguments should not be optional, considering this hook is always called with something in place of the arguments.

[Iridar]

I'm a bit unclear on the policy here. The X2DLCInfo::ModifyGeneratedUnitAppearance() is called by XGCharacterGenerator::ModifyGeneratedUnitAppearance, which in turn is called by XGCharacterGenerator::CreateTSoldier(), and all of its arguments are optional. I would assume that if a function is calling another function, passing its own arguments to it, the optional status should be inherited.

Nothing is stopping mods from calling CreateTSoldier() with arbitrary arguments to achieve arbitrary things, so I think marking the arguments in the X2DLCInfo::ModifyGeneratedUnitAppearance() as optional is a good way of communicating that there is a possibility of them being none.

[robojumper]

I see the documentation value. My comment was motivated by the fact that optional only allows omitting arguments on the caller side, and we as a caller want to ensure we always pass all the arguments we have -- even if their source might be optional (like they are in CreateTSoldier).

Additionally, not all arguments are passed verbatim. CreateTSoldier does this, for example:

if (nmCountry == '')
	nmCountry = PickOriginCountry();

so nmCountry is (hopefully) never empty.

[robojumper]

It would be great if the high-level idea behind these changes were described in a "compatibility" section. There are a lot of mods with custom CharacterGenerators and some info on how to make these compatible with the new hook in the same way that the existing hero ones were would be useful, especially since we might want to revisit this for DLC_3/Sparks.

[Iridar]

How would I do that? Add a compatibility tag to HL-Docs, and add some text about "blah blah now you don't have to MCO XGCharacterGenerator"?

[robojumper]

Not entirely. What I meant is that this hook only works for XGCharacterGenerator classes that explicitly received the Set* functions rewiring. Custom XGCharacterGenerator classes are in the same state as the Reaper/Skirmisher/Templar XGCharacterGenerator classes before this PR, which would overwrite a bunch of customization properties, overriding the DLC hook.

I'd like this PR to describe the problem and the steps needed to make an XGCharacterGenerator work with the DLC hook in a way where it doesn't potentially overwrite DLC hook changes, both so that modders can make their XGCharacterGenerators 100% compatible with the DLC hook and so that we have a reference point for the Spark XGCharacterGenerator if we choose to do a DLC_3 HL at some point.

[Iridar]

I have adjusted the documentation, removed optional from the arguments, and put const in there for most of them just in case.

[robojumper]

Thanks!

[pledbrook]

@Iridar Would you mind squashing and merging this? I would do it myself, but the commit messages aren't descriptive of the change and I have no idea what to write in the squashed commit.

[Iridar]

@Iridar Would you mind squashing and merging this? I would do it myself, but the commit messages aren't descriptive of the change and I have no idea what to write in the squashed commit.

I squashed and rebased, but I don't have the authoritah to merge.