Issues in piratespawner (some issues, mostly cleanups)

I apologize if this is a bit too big, I don't know if it's worth making separate reports for code cleanup.

The following lines are unused and could be commented out (mainly due to a lot of these being constants for that file, no need to keep them around):

local SourceModifierList = require("util/sourcemodifierlist")

-------

local LIFESPAN = {	base = TUNING.TOTAL_DAY_TIME *3,
					varriance = TUNING.TOTAL_DAY_TIME }

-------

local _scheduledtask = nil
._scheduledtask = GetTaskRemaining(_scheduledtask) -- this one's inside the OnSave function

-------

local _minspawndelay = TUNING.PIRATE_SPAWN_DELAY.min
local _maxspawndelay = TUNING.PIRATE_SPAWN_DELAY.max

-------

local _maxpirates = 1
local _timescale = 1

There's also these which could be removed or made to be used, mainly _map, though _worldstate could be used if winter checks were added to prevent raids:

local _worldstate = TheWorld.state
local _map = TheWorld.Map

local _updating = false

--------------------------------------------------------------------------------------------------------

The "onremove" listener here:

self.inst:DoTaskInTime(0,function()
    for k,v in pairs(Ents) do
        if v.prefab == "monkeyqueen" then
            self.queen = v
            self.inst:ListenForEvent("onremove", function() self.queen = nil end, self.queen)
            break
        end
    end

    if TUNING.PIRATE_RAIDS_ENABLED and self.queen then
        self.inst:StartUpdatingComponent(self)
    end
end)

Should also be stopping updating of the piratespawner component:

self.inst:ListenForEvent("onremove", function()
    self.queen = nil

    if TUNING.PIRATE_RAIDS_ENABLED then
        self.inst:StopUpdatingComponent(self)
    end
end, self.queen)

(also wouldn't it make more sense to register the queen from her prefab file, when spawning her? could account for multiple queens that way, or to like, remove extra ones if they're spawned in, to avoid issues)

Also, If winter checks are added, then a more general function to check for updating should be added (and used with a "iswinter" WorldWatcher), like this:

local function OnWinterChange(inst, iswinter)
	if iswinter or (not self.queen or self.queen.right_of_passage) then
		if _updating then
			_updating = false

			inst:StopUpdatingComponent(self)
		end
	elseif not _updating then
		_updating = true

		-- reset player searching
		_lasttick_players = {}
		_lasttick_boats = {}
		_nextpiratechance = getnextmonkeytime()

		inst:StartUpdatingComponent(self)
	end
end

This if from a modified version of the file where winter and the right of passage stop raids completely, raids are evaluated by boats occupied by players instead of each player individually, and the _updating variable is also used, but otherwise you get the idea.

--------------------------------------------------------------------------------------------------------

The following bits would be better if most of the values used in them were defined in TUNING instead of being hardcoded into the functions:

local function getnextmonkeytime()
    local days = GetAveragePlayerAgeInDays()
    local mult = 1
    if days < 10 then
        mult = 0.6
    elseif days < 20 then
        mult = 0.5
    elseif days < 40 then
        mult = 0.4
    elseif days < 80 then
        mult = 0.3
    else
        mult = 0.2
    end
    local time = (TUNING.PIRATESPAWNER_BASEPIRATECHANCE*mult) + (math.random() *  (1-mult) * TUNING.PIRATESPAWNER_BASEPIRATECHANCE )

    return time
end

    local day = GetAveragePlayerAgeInDays()
    local monkeys = 1

    if day < 15 then
        monkeys = 2+ (math.random() < 0.7 and 1 or 0)
    elseif day < 30 then
        monkeys = 2+ (math.random() < 0.7 and 1 or 0) + (math.random() < 0.3 and 1 or 0)
    elseif day < 60 then
        monkeys = 3+ (math.random() < 0.7 and 1 or 0) + (math.random() < 0.3 and 1 or 0)
    else
        monkeys = 4+ (math.random() < 0.7 and 1 or 0)    
    end

Namely the day thresholds, the multipliers, and amount of monkeys for each threshold.

--------------------------------------------------------------------------------------------------------

Megaflare detonations don't check if the queen exists, not sure if intentional. If winter checks are added, that would need to be included there too.

--------------------------------------------------------------------------------------------------------

This listener function should clean up _lasttic_players to not keep stale data stored until server restart:

local function OnPlayerLeft(src, player)
    for i, v in ipairs(_activeplayers) do
        if v == player then
            table.remove(_activeplayers, i)
            return
        end
    end
end

--------------------------------------------------------------------------------------------------------

local function GetAveragePlayerAgeInDays()
    local sum = 0
    for i, v in ipairs(_activeplayers) do
        sum = sum + v.components.age:GetAgeInDays()
    end
    return sum > 0 and sum / #_activeplayers or 0
end

This function is fine in terms of functionality, but it's a bit questionable that all players, even players outside the raiding zones, or not even on a boat, can influence these mechanics. Wouldn't it make more sense if the amount of monkeys (and time between raids) scaled based on amount of players in a certain radius or in one boat, and/or be based on how close the potential target players are to the island?

--------------------------------------------------------------------------------------------------------

Only nextpiratechance is used from the following variables that are saved:

function self:OnSave()
    local data =
    {
        maxpirates = _maxpirates,
        minspawndelay = _minspawndelay,
        maxspawndelay = _maxspawndelay,
        nextpiratechance = _nextpiratechance,
    }

    data._scheduledtask = GetTaskRemaining(_scheduledtask) -- also mentioned above

As well as:

function self:OnLoad(data)
    _maxpirates = data.maxpirates or TUNING.PIRATE_SPAWN_MAX
    _nextpiratechance = data.nextpiratechance or getnextmonkeytime()
end

_maxpirates isn't used, as mentioned at the start of the report.

--------------------------------------------------------------------------------------------------------

Lastly, it feels like the OnUpdate function of this component is a bit unnecessarily expensive?

function self:OnUpdate(dt)

    local mindist = math.huge

    if not self.queen then
        return
    end
    
    for i, v in ipairs(_activeplayers) do
        if not v.components.health:IsDead() and not TheWorld.Map:IsVisualGroundAtPoint(v.Transform:GetWorldPosition()) then
            if not _lasttic_players[v] then
                _lasttic_players[v] = {time=0, dist=math.huge} 
            else
                if _lasttic_players[v].time < GRACETIME then
                    _lasttic_players[v].time = _lasttic_players[v].time + dt
                end
                if _lasttic_players[v].time > GRACETIME then
                    _lasttic_players[v].dist = self.queen:GetDistanceSqToInst(v)
                    if _lasttic_players[v].dist < mindist then
                        mindist = _lasttic_players[v].dist
                    end
                end
            end
        else
            if _lasttic_players[v] then
                _lasttic_players[v] = nil
            end
        end
    end

mindist could be a local variable defined outside of the function, with a value that's bigger than the maximum zone (square) distance, instead of having to retrieve math.huge on each update, though I'm not sure which is cheaper to be completely honest so it's just a suggestion.

Onto a more obvious suggestion though, instead of using TheWorld.Map:IsVisualGroundAtPoint() calls, wouldn't it make more sense to check if the player is on a platform instead of being on water (on a boat or not), or use a listener for "done_embark_movement"?

I'll update this report if needed, as well as include my version of the file if it's easier to use as reference.