CanSaveUnit is flawed - Introducing CanSaveUnitGood

[Chrisy15]

GameEvents.CanSaveUnit is a very flawed GameEvent for a number of reasons, and I'm sorry to say that I can't do anything about those. Instead my interest has been spurned over a new issue, the GameEvent's nature as a Test All event.

Test events allow Lua functions to weigh in on whether a certain game process should happen as normal or whether an exceptional behaviour should occur. An example of a standard Test All event would be PlayerCanConstruct, which allows a building to be constructed if all functions return true but blocks construction if any function returns false. This and gate works well here because a true return to PlayerCanConstruct does not serve to override any obstruction to construction imposed by the database, such as tech prereqs; a return of true is merely a function's way of not blocking construction, of pretending it isn't there.

The issue is that CanSaveUnit does not work as a Test All event because the boolean polarity is reversed; doing the exceptional thing with CanSaveUnit requires a true return, and subsequently a return of false should mean that the function is ignored. Unfortunately, since CanSaveUnit is a Test All event, all functions are required to return true for a unit to be saved; if any function decides it does not want to save a unit, the and gate fails and the unit is forced to die regardless of how many other functions wanted to save it. This renders CanSaveUnit logically flawed, by virtue of how a different outcome is derived from a function's logic failing to said failed function not being there in the first place.

The solution is for CanSaveUnit to instead be a Test Any event, an or gate. This way if any function returns true then the exception is made and the unit is saved, and any return of false does not override any exception generated by another function. Unfortunately, while this conclusion is easily drawn, it requires a DLL mod to implement - and I would implore that @whoward69 and I guess @Recursive (idk I'll make a github post later) implement it into their respective DLL patches because as far as I can conceive this is an objective improvement and necessary bugfix with no negative consequences or side effects.

In the absence of a DLL mod, I have instead devised a system which can be used to (effectively) convert CanSaveUnit into a Test All event. Although it is named in homage to Machiavelli's SerialEventUnitCreatedGood, the user-end implementation is quite different so modders should pay attention to the comments in my provided example. The most pressing drawback of this system is that it does not solve the problem being discussed here, as that can only be done by a DLL mod, but merely circumvents it; thus CanSaveUnitGood does not work if ANY other function is subscribed to CanSaveUnit, and without a DLL solution it is required that every mod which uses CanSaveUnit must be made to instead use this system. While this sounds like a very unattainable goal, I would hope that the infrequent use of CanSaveUnit and the easy deployability of the utility would mean that the limited number of relevant mods can be patched either by their creators or by their users.

How To Use It
This utility comes in the form of a Lua file which must be added through InGameUIAddin, NOT the VFS. If desired or required, the code can instead be ported into an existent InGameUIAddin Lua file so long as it is copied in its entirety. This utility creates two LuaEvents which your own InGameUIAddin Lua script will need to interact with, like so:

-- Standard normal function which you would have otherwise just subscribed to CanSaveUnit.
-- This solution should be applied to both traditional CanSaveUnit functions and to functions merely using the event to detect Trade Route pillaging.
function C15_CanSaveUnit(playerID, unitID)
    local pPlayer = Players[playerID]
    local pUnit = pPlayer:GetUnitByID(unitID)
    if pUnit:GetUnitType() == GameInfoTypes["UNIT_WARRIOR"] then
        return true
    else
        return false
    end
end

-- This function needs to be defined by the user to resolve load order issues. 
-- Names and methods don't matter, all that matters is that your normal function gets linearly inserted into the table provided by the function parameters.
-- Remember, no function arguments when inserting your function! You're inserting the function itself, not the returned value.
function C15_CanSaveUnit_Import(tTable)
    tTable[#tTable+1] = C15_CanSaveUnit
end

-- Make sure that you subscribe the import function to this LuaEvent, not your normal function!
-- The second LuaEvent then serves to call the first, "doing" that which you have just set up.
LuaEvents.C15_CanSaveUnitGood_Import.Add(C15_CanSaveUnit_Import)
LuaEvents.C15_CanSaveUnitGood_Refresh.Call()

The core utility code, for easy reference or easy copying:

Spoiler Nerd Code for nerds :

-- C15_CanSaveUnitGood
-- Author: Chrisy15
-- DateCreated: 4/8/2024 4:07:29 PM
--------------------------------------------------------------
local iVersionNumber = 2
local tStoredFunctions = {}

local bPrintSaveResult = false -- This will create A LOT of print statements, but can serve to identify if another mod's CanSaveUnit subscription is interfering with this system.

-- Refresh list of cached functions by telling user files to insert their CanSaveUnit functions into tStoredFunctions. Directly taken from MCIS lol
function C15_CanSaveUnitGood_RefreshList()
	tStoredFunctions = {}
	LuaEvents.C15_CanSaveUnitGood_Import.Call(tStoredFunctions)
end

LuaEvents.C15_CanSaveUnitGood_Refresh.Add(C15_CanSaveUnitGood_RefreshList)

-- Remove all content from this instance of the utility. Version check avoids self-destruction.
function C15_CanSaveUnitGood_Disable(iNewVersion)
	if iNewVersion > iVersionNumber then
		Events.SequenceGameInitComplete.Remove(C15_CanSaveUnitGood_Init)
		LuaEvents.C15_CanSaveUnitGood_Refresh.Remove(C15_CanSaveUnitGood_RefreshList)
		tStoredFunctions = {}
	end
end

LuaEvents.C15_CanSaveUnitGood_Disable.Add(C15_CanSaveUnitGood_Disable)

-- All version discrepancies should be resolved by the time SequenceGameInitComplete is called
function C15_CanSaveUnitGood_Init()
	LuaEvents.C15_CanSaveUnitGood_Refresh.Call()
	-- Additions to tStoredFunctions made after this point can still be recognised, so there's no impetus on mods to make their Imports before SequenceGameInitComplete.
	local function C15_CanSaveUnitGood_CanSaveUnit(playerID, unitID)
		local bSaveUnit = false -- Default state is that Unit should die
		for i = 1, #tStoredFunctions do
			if tStoredFunctions[i](playerID, unitID) then
				if bPrintSaveResult then
					print("CanSaveUnit: true!")
				end
				
				bSaveUnit = true -- One of the functions have concluded true, and thus the Unit should be saved
			end
		end
		
		if bPrintSaveResult then
			print("CanSaveUnit: ", bSaveUnit)
		end
		
		return bSaveUnit -- Don't return until all functions have been called, to ensure that all their code gets executed
	end
	
	GameEvents.CanSaveUnit.Add(C15_CanSaveUnitGood_CanSaveUnit)
	
	MapModData.C15_CanSaveUnitGood = nil
end

-- Prevent Duplicates - derived from MCIS
if MapModData.C15_CanSaveUnitGood then
	local iCurrentVersion = MapModData.C15_CanSaveUnitGood
	if iCurrentVersion >= iVersionNumber then
		print("Version check failed!", iVersionNumber)
	else
		LuaEvents.C15_CanSaveUnitGood_Disable.Call(iVersionNumber)
		LuaEvents.C15_CanSaveUnitGood_Refresh.Call()
		MapModData.C15_CanSaveUnitGood = iVersionNumber
		Events.SequenceGameInitComplete.Add(C15_CanSaveUnitGood_Init)
		print("Version check passed!", iVersionNumber)
	end
else
	MapModData.C15_CanSaveUnitGood = iVersionNumber
	Events.SequenceGameInitComplete.Add(C15_CanSaveUnitGood_Init)
	print("Version check passed!", iVersionNumber)
end

 

[whoward69]

I'm no longer modding Civ V and rarely play it, so don't hold your breath for anything from me.

Also, I'd be against changing the existing line of code as 1) you don't know if any mods are using this in an unexpected and clever way and 2) the name "CanSaveUnit" itself is flawed

CanSaveUnit implies a question like CanTrain, CanConstruct, et al. Imagine if hooking "CanTrain" and returning true actually caused a unit to appear in the city. It should be called "SaveUnit" as it does the saving and not just ask if a save is possible. "SaveUnit" is also confusing as it could imply that the unit data was being "saved" somehow. Something like "AvoidDeath" or "EscapeBeingKilled" would be better IMHO.

 

[Chrisy15]

Utility updated to ensure that all subscribed functions are executed before returning true or false. Situations where this could have been a problem are relatively niche, and so long as one enabled mod is using this version the fix will take effect for all subscribed functions.