Zone Sleep time changed to 15min

[DrDoakHead]

I affirm:

• I understand that if I do not agree to the following points by completing the checkboxes my PR will be ignored.
• I understand I should leave resolving conversations to the LandSandBoat team so that reviewers won't miss what was said.
• I have read and understood the Contributing Guide and the Code of Conduct.
• I have tested my code and the things my code has changed since the last commit in the PR and will test after any later commits.

What does this pull request do?

This is an attempt to fix #5910.

Steps to test these changes

It builds and runs locally. I have not hooked it up to a server.

PLEASE DO NOT MERGE THIS. I SIMPLY WANT SOME FEEDBACK FROM A DEVELOPER

[github-actions]

✨ Thanks for the PR! ✨

This is a friendly automated reminder that the maintainers won't look at your PR until you've properly completed all of the checkboxes in the pre-filled template.

[zach2good]

Hello and welcome 👋

I'm the original author for #5910 and as I was typing up a response saying I didn't remember anything about it - a bunch of things came back to me. So here's my mind dump:

• Number one priority/warning/issue: In terms of basic things the server can do, keeping a zone alive is the most expensive thing possible. All the other expensive things I normally complain about like Navmesh access, SQL queries, etc. all rely on a zone actually being awake. Forcing a zone awake is forcing every mob in that zone to tick, and then the server pays the cost. If a player isn't in the zone, then you're potentially paying this heavy cost for no reason.
• The abuse vector I want to avoid here is if you're a small-time operator running the server on your old laptop or a raspberry pi etc. and the keepalive timer is set to 15 mins, a bad actor can just go to as many zones as possible (or get their friends to help) and jack up your server workload. We're not at a point where a single process can handle 100+ active zones, so server performance would start to degrade.
• Years ago we settled on 5s as the time limit to allow up to 2 logical ticks to fire off before the zone goes back to sleep after the last player leaves. This was to help with my confrontation system, which needed that buffer time to clean itself up.
• There is an actual retail use case here though, so the whole idea doesn't need to be thrown out: I think it would be harder to "simulate" zones that have events that happen in them when players aren't there than it would be to just spin up the zone and have it do things. When mobs travel through zones before besieged, when mobs travel for campaign, etc. it would be easier to just make the zone wake up and do the logic as if the player was there. This lends itself more to having a global ask the zone to wake up, and then put it back to sleep when it's done though, rather than keeping a zone awake for 15 mins when the last player leaves.

As a general question though, aside from trying to close out that ticket, is there something you're trying to solve here?

[WinterSolstice8]

As a general question though, aside from trying to close out that ticket, is there something you're trying to solve here?

There is a legitimate problem here, if you train mobs (NMs) to the zoneline and leave, on retail they will path back as if they moved normally. Right now if you do that on LSB and you're the only player in the zone you left they won't path back at all, which is incorrect. I think this is a legitimate change. In the future we may want to tick zones that don't have players in them recently at all (such as during campaign, which will be event driven to activate zones in that case)

[WinterSolstice8]

Thinking further, maybe what we could do is keep the zones alive until all mobs have pathed back to "home" and finished despawning? Most mobs despawn on zone so most of the time zones will remain active for seconds. I can't think of other cases at this exact moment however

[DrDoakHead]

@zach2good This is my first go at fixing something on LSB. I basically just wanted to fix your ticket, which in there you suggested, "zone:setKeepalive(utils.minutes(15)) or similar"

It makes sense that keeping the zone alive with no one in it would drastically increase the server load. I come from safety critical land where you need to account for WCET type things, as I think even in horizon, there is rarely a zone empty. With a few exceptions.

Anyways, again, I was taking your ticket as gold, and trying to do what you asked. I didn't put much thought into the "why".

[DrDoakHead]

@WinterSolstice8 if that is what we would like to solve, there would probably be a better way than just waiting and assuming after x amount of time that they pathed back? I'm not familiar enough with the code, but if the mob class had some sort of flag signifying it was back "home", we could check those flags for all mobs at some sort of periodic rate, then when true, sleep the zone?

[cocosolos]

Something to consider too is mobs that despawn from various conditions while idle. Consider King Vinegarroon. If you zone in during his weather and he pops, then you leave the zone, he will stay there until someone enters the zone again and his roam tick will happen and he'll despawn, setting his ToD respawn time to that point instead of when his weather actually ended and he was supposed to despawn. Or if he spawns and you leave and come back a few hours later after he should have despawned and reset his timer, if it's earth weather again he'll still be up.

[DrDoakHead]

@zach2good and @WinterSolstice8 does my code look correct? Will it do the intended functionality?

• I was a little confused on the singleton timer class. Do I need to pass it "this"?
• I noticed the removeTask will print a warning if the item is not in the queue.
• Are there any things that stick out to you as wrong or just against your general coding guidelines?

[jmcmorris]

I happened to see this and wanted to share my thoughts on how this should likely be fixed ultimately but not in this PR. I imagine updating a zone is only expensive due to updating monsters. Instead of putting the entire zone to sleep we should have monsters sleep if they are within their home range and no players in the zone. This could easily be a check at the start of the zone update. With this logic it'd allow monsters to roam back to home from a zone line and also handle logic like KV despawning. Perhaps we'd need a "no sleep" flag for mobs like this.

[DrDoakHead]

I'm not sure I'm following. This is my first PR so I am very new :)
Current - Zone sleeps 5 seconds after last person leaves
This PR - Zone sleeps 15 minutes after last person leaves
Purposed - Zone sleeps after last person leaves and mobs have pathed back to "home"

If "current" works, how would "purposed" break things like KV?

[cocosolos]

If "current" works, how would "purposed" break things like KV?

Sorry I wasn't really clear, KV currently behaves like that so there's nothing to break. I was just commenting on what a future solution should consider.

[zach2good]

I like this approach, and I'm glad we had the things available for it: GetMobList(), IsFarFromHome(), and SleepZone() 👍

Outside of the scope of this PR: I'd probably want to see a new zoneflag (ZONEMISC, maybe, if we have room?) that enables/disables this logic.

Also, we're passing around copies of these entity lists, someone else can fix that in another PR.

[WinterSolstice8]

Maybe we should also add in a check for mobs being fully healed too?

When all mobs path path it will call this which will heal them instantly. Normally this would be impossible if a mob is afflicted by a DoT, so if we wait out being healed to max hp/mp that call to m_zoneEntities->HealAllMobs(); isn't necessary. Buff timers will expire on their own naturally (they store timestamps iirc) so we don't need to wait for those. Just HP/MP

void CZone::SleepZone()
{
    if (ZoneTimer && m_zoneEntities->CharListEmpty())
    {
        ZoneTimer->m_type = CTaskMgr::TASK_REMOVE;
        ZoneTimer         = nullptr;
        ZoneTimerTriggerAreas->m_type = CTaskMgr::TASK_REMOVE;
        ZoneTimerTriggerAreas         = nullptr;
        m_zoneEntities->HealAllMobs();
    }
}

[DrDoakHead]

I was able to setup my client/server and verify this. Only to the point that i did damage to a mob, zoned, waited 10sec or so, and re-entered and the mob was pathing back. I can't verify from a black box perspective that the zone was slept.

[DrDoakHead]

do i need to make another PR with the little checkbox checked or is this ok?

[WinterSolstice8]

You can edit the checkbox back in on the post, as for verifying if it actually slept, you can just add a quick debug print temporarily ShowInfo("Zone is going to sleep.")

[DrDoakHead]

I am remembering times in garliage citadel and yuhtunga jungle where someone would train a bunch of mobs to the entrance and zone, then the mobs would path back instead of disappearing and wipe xp parties.

That behavior was induced not by a zone being empty though. This won't induce that right?

[WinterSolstice8]

Nope, only NMs will path back. Normal mobs will despawn. I guess it could be enabled to work again and this code would help it work properly, which is good

[WinterSolstice8]

CI is complaining about this:

Suggested change

	if (mob && !mob->isFullyHealed() || ((mob->CanRoamHome() && mob->IsFarFromHome()) || !mob->CanRoamHome()))
	if (mob && (!mob->isFullyHealed() || ((mob->CanRoamHome() && mob->IsFarFromHome()) || !mob->CanRoamHome())))

[DrDoakHead]

i see that ty. also with print statements im seeing other issues. looking into them.

[cocosolos]

Nope, only NMs will path back. Normal mobs will despawn. I guess it could be enabled to work again and this code would help it work properly, which is good

I think this is already supported with a global setting and some mob mods. This logic looks pretty similar to this in mob_controller. Maybe we could add a member var to mob entity, something like bool m_idle, and set it in mob_controller on the roam tick, then just check that here.

[DrDoakHead]

we want to check if the hp and mp has been restored.

which I am running into issues...for example, mobs that are fished up don't report hp==maxhp. And some random mobs in the zone aren't reporting being fully healed. Still looking into those.

[WinterSolstice8]

My guess: mobs that aren't spawned are mucking up your check. I bet things would work better if you filtered out the mobs that are not spawned

[DrDoakHead]

wow yea, that was it. thank you!

[cocosolos]

we want to check if the hp and mp has been restored.

The HP check is fine, it's this part I'm looking at:

((mob->CanRoamHome() && mob->IsFarFromHome()) || !mob->CanRoamHome())

This is basically a duplicate check of this in mob_controller (my comments added):

            // PMob->m_idle = true;
            if (!PMob->getMobMod(MOBMOD_DONT_ROAM_HOME) && PMob->IsFarFromHome())
            {
                if (PMob->CanRoamHome())
                {
                    // ...
                    // PMob->m_idle = false;
                }
                else if (!(PMob->getMobMod(MOBMOD_NO_DESPAWN) != 0) && !settings::get<bool>("map.MOB_NO_DESPAWN"))
                {
                    PMob->PAI->Despawn();
                    return;
                }
            }

If one of the conditions we're looking for to keep the zone alive is that they're roaming back home and we're already checking for that, we should leverage that instead of duplicating the check. That way you can just check:

if (mob && !mob->isFullyHealed() || !mob->m_idle)

I'm not sure the exact order this stuff is happening, but it also looks like if this roam check in zone fires before the one in mob_controller, the Despawn call will be delayed until the zone wakes back up.