[Fix] "when login" trigger before affect initialization

  • Konuyu açan Konuyu açan Mitachi
  • Açılış Tarihi Açılış Tarihi
  • Yanıt Yanıt 9
  • Gösterim Gösterim 506

Mitachi

Üye
Üye
Mesaj
15
Beğeni
26
Puan
440
Ticaret Puanı
0
Kolaylık olması açısından metni İngilizce bırakıyorum.

If you would like a full explanation:

I don't usually explain what I do, but I've decided to start doing so. It might be useful, who knows.

This is quite an old and well-known bug, that’s why quests have always been written in a way to avoid it.
The core issue is that quests are initialized before affects, so the when login trigger fires before affects are loaded.

Have you ever tried to do something on login that involved affects?

e.g.:
  • You want to check if the player has a mount when entering the OX map and dismount them.
  • You want to polymorph them into mob 101.
Well, that wouldn’t work.

The common workaround everyone uses is simply adding a timer to the login trigger, even 1 second is enough.
That’s why this bug has never been considered a real problem: the workaround is simple and effective. If it works, it works.

Lua (Quest):
Genişlet Daralt Kopyala
-- BUG AFFECTED
quest bug_affect_login begin
    state start begin
        when login with pc.get_map_index() == 41 begin
            pc.polymorph(101, 300); -- poly 101 for 5 minutes
        end
    end
end

-- WORKAROUND FIX WE'RE USED TO
quest bug_affect_login begin
    state start begin
        when login with pc.get_map_index() == 41 begin
            timer("login_workaround", 1) -- WORKAROUND
        end
   
        when login_workaround.timer begin -- WORKAROUND
            pc.polymorph(101, 300); -- poly 101 for 5 minutes
        end
    end
end

However, today I started wondering why this happens. At first I blamed the "slowness" of affect loading.
I then discovered that quests are simply loaded before affects.

Swapping the order does work
At first I thought this wasn’t guaranteed and considered it unsafe, but after reviewing the SQL layer it’s clear the order is deterministic.
Still, I preferred a more robust fix: always using the quest_login_event, and delaying the login trigger until both PHASE_GAME and IsLoadedAffect() are satisfied.

Eventually, I traced it back to the exact piece of code that calls the login trigger, inside QuestLoad, at the end:

C++:
Genişlet Daralt Kopyala
        if (ch->GetDesc()->IsPhase(PHASE_GAME))
        {
            sys_log(0, "QUEST_LOAD: Login pc %d", pQuestTable[0].dwPID);
            quest::CQuestManager::instance().Login(pQuestTable[0].dwPID);
        }
        else
        {
            quest_login_event_info* info = AllocEventInfo<quest_login_event_info>();
            info->dwPID = ch->GetPlayerID();

            event_create(quest_login_event, info, PASSES_PER_SEC(1));
        }

So, during loading it now checks if the character is in PHASE_GAME.
If not, it calls the quest_login_event timer, which will keep checking and delaying by one second until IsPhase is actually PHASE_GAME, at that point we are safely in-game.

Basically, those two lines of code, the sys_log with QUEST_LOAD and the trigger call, were just copy-pasted into the event if the conditions were met.
So I decided to keep the event and remove the direct login trigger.

C++:
Genişlet Daralt Kopyala
            quest_login_event_info* info = AllocEventInfo<quest_login_event_info>();
            info->dwPID = ch->GetPlayerID();

            event_create(quest_login_event, info, PASSES_PER_SEC(1));

Ok, now we will call the event everytime, let's see how it looks

C++:
Genişlet Daralt Kopyala
EVENTFUNC(quest_login_event)
{
    quest_login_event_info* info = dynamic_cast<quest_login_event_info*>(event->info);

    if (!info)
    {
        sys_err("quest_login_event> <Factor> Null pointer");
        return 0;
    }

    DWORD dwPID = info->dwPID;

    LPCHARACTER ch = CHARACTER_MANAGER::instance().FindByPID(dwPID);

    if (!ch)
        return 0;

    LPDESC d = ch->GetDesc();
    if (!d)
        return 0;

    if (d->IsPhase(PHASE_HANDSHAKE) ||
        d->IsPhase(PHASE_LOGIN) ||
        d->IsPhase(PHASE_SELECT) ||
        d->IsPhase(PHASE_DEAD) ||
        d->IsPhase(PHASE_LOADING))
    {
        return PASSES_PER_SEC(1);
    }
    else if (d->IsPhase(PHASE_CLOSE))
    {
        return 0;
    }
    else if (d->IsPhase(PHASE_GAME))
    {
        sys_log(0, "QUEST_LOAD: Login pc %d by event", ch->GetPlayerID());
        quest::CQuestManager::instance().Login(ch->GetPlayerID());
        return 0;
    }
    else
    {
        sys_err(0, "input_db.cpp:quest_login_event INVALID PHASE pid %d", ch->GetPlayerID());
        return 0;
    }
}

It delays by one second until IsPhase is PHASE_GAME.
So let’s add an extra check: make it delay by one second until IsLoadedAffect() is true as well.

C++:
Genişlet Daralt Kopyala
...
    else if (d->IsPhase(PHASE_GAME))
    {
        if (!ch->IsLoadedAffect()) // fix login event too early than affect load
        {
            return PASSES_PER_SEC(1);
        }

        sys_log(0, "QUEST_LOAD: Login pc %d by event", ch->GetPlayerID());
        quest::CQuestManager::instance().Login(ch->GetPlayerID());
        return 0;
    }
...

That's all.

If you want just the simple fix:

C++:
Genişlet Daralt Kopyala
/// 1.) Search in void CInputDB::QuestLoad(LPDESC d, const char * c_pData), at the end:

        if (ch->GetDesc()->IsPhase(PHASE_GAME))
        {
            sys_log(0, "QUEST_LOAD: Login pc %d", pQuestTable[0].dwPID);
            quest::CQuestManager::instance().Login(pQuestTable[0].dwPID);
        }
        else
        {
            quest_login_event_info* info = AllocEventInfo<quest_login_event_info>();
            info->dwPID = ch->GetPlayerID();

            event_create(quest_login_event, info, PASSES_PER_SEC(1));
        }

// and replace with:

        quest_login_event_info* info = AllocEventInfo<quest_login_event_info>();
        info->dwPID = ch->GetPlayerID();

        event_create(quest_login_event, info, PASSES_PER_SEC(1));

/// 2.) Search in EVENTFUNC(quest_login_event):

    else if (d->IsPhase(PHASE_GAME))
    {
        sys_log(0, "QUEST_LOAD: Login pc %d by event", ch->GetPlayerID());
        quest::CQuestManager::instance().Login(ch->GetPlayerID());
        return 0;
    }

// and replace with:

    else if (d->IsPhase(PHASE_GAME))
    {
        if (!ch->IsLoadedAffect()) // fix login event too early than affect load
        {
            return PASSES_PER_SEC(1);
        }

        sys_log(0, "QUEST_LOAD: Login pc %d by event", ch->GetPlayerID());
        quest::CQuestManager::instance().Login(ch->GetPlayerID());
        return 0;
    }

If you are using marty files you don't need this additional fix, he's already doing that.

IsLoadedAffect is only set if you actually have an Affect or LoadAffect won't load, which is a problem.
Not only for this fix, but in general, IsLoadedAffect is used in anti-exploit contexts.

It is necessary that they are loaded even if you do not have affect, so, you got another fix there

C++:
Genişlet Daralt Kopyala
        case QID_AFFECT:
            sys_log(0, "QID_AFFECT %u", info->dwHandle);

            // if there are no affects, make an empty one to send the packet
            if (!mysql_num_rows(pSQLResult))
            {
                TPacketAffectElement pAffElem{};
                DWORD dwCount = 0;

                peer->EncodeHeader(HEADER_DG_AFFECT_LOAD, info->dwHandle, sizeof(DWORD) + sizeof(DWORD) + sizeof(TPacketAffectElement) * dwCount);
                peer->Encode(&info->player_id, sizeof(DWORD));
                peer->Encode(&dwCount, sizeof(DWORD));
                peer->Encode(&pAffElem, sizeof(TPacketAffectElement) * dwCount);
                break;
            }

            RESULT_AFFECT_LOAD(peer, pSQLResult, info->dwHandle);
            break;
 
Son düzenleme:
Great contribution from a talented guy, thanks. 🙏
 
1.webp

2.webp


The ReturnQuery already tells you what to read and when.
The ReturnQuery will always take precedence over QID_AFFECT.

AFTER:

3.webp


I've tried many times and affect always loads first.

This edit was made in ClientManagerPlayer.cpp.​
 
Son düzenleme:
25694 eklentisini görüntüle
25695 eklentisini görüntüle

The ReturnQuery already tells you what to read and when.
The ReturnQuery will always take precedence over QID_AFFECT.

AFTER:

25696 eklentisini görüntüle

I've tried many times and affect always loads first.

This edit was made in ClientManagerPlayer.cpp.​

There's no problem with them being loaded first; it's been that way for 20 years, and that's fine. I explained it in the post.
What matters is that the affects are fully initialized, not that the initialization call has been made.

Initialization calls are called in QUERY_PLAYER_LOAD

1757938207602affect_load.png

and there is another one below, if you invert these 3 calls, affect will initialize before quest, but that's not the point.

The number you give to the header has nothing to do with when you call it, you could call the 255 header before the 1 one

The way I did the fix is the safest one, it was already designed to be this way, in fact there was already an event that called the trigger, only it was not used except in phases other than the game.
 
There's no problem with them being loaded first; it's been that way for 20 years, and that's fine. I explained it in the post.
What matters is that the affects are fully initialized, not that the initialization call has been made.

Initialization calls are called in QUERY_PLAYER_LOAD

1757938207602affect_load.png

and there is another one below, if you invert these 3 calls, affect will initialize before quest, but that's not the point.

The number you give to the header has nothing to do with when you call it, you could call the 255 header before the 1 one

The way I did the fix is the safest one, it was already designed to be this way, in fact there was already an event that called the trigger, only it was not used except in phases other than the game.
I don't know exactly what the problem was, but from what you said, I just understood that the tasks came before the effects. That's why I commented.
 
I don't know exactly what the problem was, but from what you said, I just understood that the tasks came before the effects. That's why I commented.
Yes, I explained that the calls are "async", because they go from db and then go to game, even if you initialize affect before quest, you don't know which one will finish first, that's why it's worth making sure that affect are initialized before sending the quest trigger, I posted my whole thought process above, just read the spoiler
 
Yes, I explained that the calls are "async", because they go from db and then go to game, even if you initialize affect before quest, you don't know which one will finish first, that's why it's worth making sure that affect are initialized before sending the quest trigger, I posted my whole thought process above, just read the spoiler
So wouldn't it make sense to make a single callback instead of creating an event in memory each time?

Callback -> quest::QuestManager::instance().Login(id)
 
Yes, I explained that the calls are "async", because they go from db and then go to game, even if you initialize affect before quest, you don't know which one will finish first, that's why it's worth making sure that affect are initialized before sending the quest trigger, I posted my whole thought process above, just read the spoiler
Async != Concurrent, The ReturnQuery calls are handled by a another single(for each DB slot) SQL processor thread that is backed by a single queue. when you call ReturnQuery, the request is simply appended to the end of that queue. On the SQL processor thread, these queries are processed strictly in FIFO order, not in a scrambled or random sequence. So, if you initialize the 'affect' query before the quest query, the affect query will be processed first.
 
Async != Concurrent, The ReturnQuery calls are handled by a another single(for each DB slot) SQL processor thread that is backed by a single queue. when you call ReturnQuery, the request is simply appended to the end of that queue. On the SQL processor thread, these queries are processed strictly in FIFO order, not in a scrambled or random sequence. So, if you initialize the 'affect' query before the quest query, the affect query will be processed first.
I put it in quotes around async on purpose, it's not an concurrent async call, even though I thought it was when I fixed it a few days ago, but it didn't make sense to move the calls for me, this is still the most easy and safe fix you can do, the code was already all there.

So wouldn't it make sense to make a single callback instead of creating an event in memory each time?

Callback -> quest::QuestManager::instance().Login(id)
Modify it as you like, I fixed a bug that has been here for 20 years and also offered both solutions (move the call or wait for initialization) and specified that the safest thing would be to wait for loading, you can also use the one offered in the image above if you want so you save your 30 bytes of memory
 
Geri
Üst