🐛 onMobRoam and onMobRoamAction are duplicated code

TeoTwawki commented 2 years ago

[x] I have paid attention to this example and will edit again if need be to not break the formatting, or I will be ignored
[x] I have searched existing issues to see if the issue has already been opened, and I have checked the commit log to see if the issue has been resolved since my server was last updated
[x] I have read and understood the Contributing Guide

Branch affected by issue

base

Steps to reproduce

LOOK AT THE CODEBASE AND DESPAIR! cough er I mean, you can see in luautils.cpp that these 2 functions ae identical except for a nullptr check + mob check to bail out on invalid entities and their name. Years ago all the "action" word were dropped off function names (by me!) but some server's code submission created this duplicate function instead of renaming their lua functions, and it was merged.

int32 OnMobRoam(CBaseEntity* PMob)
{
    TracyZoneScoped;

    sol::function onMobRoam = getEntityCachedFunction(PMob, "onMobRoam");
    if (!onMobRoam.valid())
    {
        return -1;
    }

    auto result = onMobRoam(CLuaBaseEntity(PMob));
    if (!result.valid())
    {
        sol::error err = result;
        ShowError("luautils::onMobRoam: %s", err.what());
        return -1;
    }

    return 0;
}

int32 OnMobRoamAction(CBaseEntity* PMob)
{
    TracyZoneScoped;

    if (PMob == nullptr || PMob->objtype != TYPE_MOB)
    {
        return -1;
    }

    sol::function onMobRoamAction = getEntityCachedFunction(PMob, "onMobRoamAction");
    if (!onMobRoamAction.valid())
    {
        return -1;
    }

    auto result = onMobRoamAction(CLuaBaseEntity(PMob));
    if (!result.valid())
    {
        sol::error err = result;
        ShowError("luautils::onMobRoonMobRoamActionam: %s", err.what());
        return -1;
    }

    return 0;
}

They have since evolved slightly differently:

if (m_Tick >= m_LastRoamScript + 3s)
{
    PMob->PAI->EventHandler.triggerListener("ROAM_TICK", CLuaBaseEntity(PMob));
    luautils::OnMobRoam(PMob);
    m_LastRoamScript = m_Tick;
}

https://github.com/LandSandBoat/server/blob/214165485ac9c6eb1a0ed409986419ff592764fa/src/map/ai/controllers/mob_controller.cpp#L915

and

else if (PMob->m_roamFlags & ROAMFLAG_EVENT)
{
    // allow custom event action
    luautils::OnMobRoamAction(PMob);
    m_LastActionTime = m_Tick;
}

https://github.com/LandSandBoat/server/blob/214165485ac9c6eb1a0ed409986419ff592764fa/src/map/ai/controllers/mob_controller.cpp#L875

Expected behavior

Not have 2 functions causing confusion.

If these cannot be merged, one of them should be renamed.

TeoTwawki commented 2 years ago

I believe right now the main diff is one of these executes always, and the other only when flagged. My vote here is:

onMobRoam -> onMobInactive (triggers when mob goes idle, which will then start roaming) onMobRoamAction -> onMobRoam (actual roaming is happening)

DiscipleOfEris commented 2 years ago

At the moment, onMobRoam triggers every 3 seconds a mob is not engaged in battle, whether it's idle or running about or w/e. onMobRoamAction triggers specifically when a mob with ROAMFLAG_SCRIPTED (used to be ROAMFLAG_EVENT) wants to find a new roam path, so it'd be roughly once every roam cooldown period (specified by MobMod ROAM_COOL). The onPath function is called whenever a mob (or NPC) reaches a point in its determined path while not in combat.

If you leave onMobRoam as it is now, a more directly descriptive name might be onMobRoamTick. The combat analog is onMobFight which is sent every combat tick, so the two current names somewhat match eachother.

I don't really see a problem with either of the rename suggestions you gave though, so long as you consider "Inactive" to include actively running a roam path.

TeoTwawki commented 2 years ago

I don't really see a problem with either of the rename suggestions you gave though, so long as you consider "Inactive" to include actively running a roam path.

I think in terms that separate random-roam from roaming with an actual set path, so to me onMobRoam is "it's idle - not actively engaged in chasing or attacking". Just to explain why did didn't seem to acknowledge the pathing aspect of "roaming".

If the "action" version is only fired on new path, perhaps it can just be named to specify that

The onPath function is called whenever a mob (or NPC) reaches a point in its determined path while not in combat.

sudden point of concern (oh hell did I miss something in that PR???)

~~last I knew, onPath for MOBS was a purely lua function - this was why we kick it off in onMobRoam on every pathing mob. If that behavior has changed, then a lot of scripts were made wrong by it.~~ Today I learn the core does read it, but only when they change to a new point for mobs where as NPCs get it read immediately after spawn. So its irritatingly a "hybrid function".

DiscipleOfEris commented 2 years ago

sudden point of concern (oh hell did I miss something in that PR???)

I didn't modify or add any new OnPath() calls with my PR. The only thing I added was a triggerListener("PATH") next to all the places OnPath() was already being called. The "PATH" event didn't even exist before so there's no possible conflict with previous scripts. Same with the "ROAM_ACTION" event actually. There are two mobs (Qiqirn Treasure Hunter and Race Runner) that were already using onMobRoamAction() and... They were working fine when I tested, but in retrospect they should probably be tested again. I didn't test them after the latest commit in the PR.

EDIT: And the reason for the "PATH" event was because I was thinking about making patrol() work like I've made follow() work. A call and forget, rather than having to call it each roam tick or path tick or w/e. I decided not to touch patrol() though cause that was entirely unnecessary scope creep of the PR.

TeoTwawki commented 2 years ago

rabbit hole induced addendum: things are almost exactly the opposite of how I thought when I opened my mouth. The function exists in lua for NPCs but the binding bails out on seeing an NPC, and they use initNpcAi() to create a pointer for CPathFind instead. Which hurts my soul.

LandSandBoat / server