Trinitycore: [3.3.5] Missing "Quest Complete" Text for all Explore Quests

[Tauriella]

Description:
Example:
Use Quest 62 and go in the Deep Mines
Result:
Not a comple quest text is displayed

http://www.wowhead.com/quest=62/the-fargodeep-mine

Branch(es):

CHANGEME 3.3.5

TC rev. hash/commit:
TrinityCore rev. 6fe4d37f8d8b 2017-05-02 14:57:10 +0200 (335 branch)

TDB version: TDB_full_world_335.63_2017_04_18 + Updates

Operating system: Win7 64

[STARRHELD]

@Tauriella

Would appear as Scout through the Fargodeep Mine (Complete) right?

Now it appears as Objective Complete

[MrSmite]

Works fine for me with TDB_full_world_335.61_2016_04_11.sql + updates to 2db9269ec1c06b5fccc45dc0a5b4f714f9b48804

quest_template.AreaDescription = Scout through the Fargodeep Mine so when you hit the POI, it says "Objective Complete - Scout through the Fargodeep Mine" or something to that effect.

Also,

SELECT * FROM quest_template WHERE (`Flags` & 4) = 4

produces 60 rows with only quest 1149 having no data in AreaDescription

[Tauriella]

@TW1NKLET yes :+1:

[ghost]

@Tauriella : does this mean that the quest works as intended now, that I don't have to test it too?

[Tauriella]

The Quest itself works fine, but no complete Message displayed

"Scout through the Fargodeep Mine (Complete)"

[ghost]

TrinityCore rev. 011b8847d1bb 2017-05-03 17:48:42 +0200 (3.3.5 branch) (Win64, Release, Static)
TDB 335.63 plus updates up to and including 2017_05_03_00_world_335.sql

Confirmed. Without any addons, the quest just updates its status, neither any on-screen message nor in chat log.
Using only client tools, it is possible to see the quest update, it changes log text from:

The Fargodeep Mine

• Scout through the Fargodeep Mine.

to:

The Fargodeep Mine

• Return to Marshal Dughan at Goldshire in Elwynn Forest.

at the right spot if you track the quest from the log window so it shows up in the UI list to the right on the screen.
With an addon like Carbonite, it is possible to get a message in chat log.

[MrSmite]

So this doesn't appear to be a DB issue? I wonder if the 335 core is becoming polluted with 4x or 7x ways of doing things by accident like the SQL files have from time to time.

[ghost]

This year I have also noticed a change in how repeatable quests (especially subquests which don't give any XP) are shown, they are shown with the regular yellow quest ❕ (exclamation mark) instead of a light blue one (or even a small light blue ❔ (question mark) where you only need to pick up a quest item or similar). I seem to remember some changes related to quests made in one or some of the PRs written by @xinef1, but I don't know enough to point out where the change happened that made this new behaviour start appearing.

For this specific issue, though, it would be useful to make a list of easily accessible quests (beside the Fargodeep Mine) where this issue can be tested and confirmed. I only remember https://wow.gamepedia.com/Quest:Investigate_An'daroth / http://www.wowhead.com/quest=9160/investigate-andaroth in Ghostlands (blood elf / horde quest) and Stonetalon Standstill (http://db.vanillagaming.org/?quest=25). If you can come up with some more quests, it would be useful to see if this is a core issue, DB issue or script issue.

[ForesterDev]

diff --git a/src/server/game/Entities/Player/Player.cpp b/src/server/game/Entities/Player/Player.cpp
index 44d543c2..726ac596 100644
--- a/src/server/game/Entities/Player/Player.cpp
+++ b/src/server/game/Entities/Player/Player.cpp
@@ -16366,7 +16366,7 @@ void Player::AreaExploredOrEventHappens(uint32 questId)
                 m_QuestStatusSave[questId] = QUEST_DEFAULT_SAVE_TYPE;

                 // if we cannot complete quest send exploration succeded (to mark exploration on client)
-                if (!CanCompleteQuest(questId))
+                if (CanCompleteQuest(questId))
                     SendQuestComplete(questId);
             }
         }

Try

[ghost]

Hmm, interesting. That is a reversed check compared to the original and the comment.
Do you think any of the recent changes in the source reversed the result of that check?

[ghost]

Diff based on current commit (rev. 4843544c2178 2017-05-05 05:52:49 +0200 (3.3.5 branch)) :

diff --git a/src/server/game/Entities/Player/Player.cpp b/src/server/game/Entities/Player/Player.cpp
index dadfa60..3f0a865 100644
--- a/src/server/game/Entities/Player/Player.cpp
+++ b/src/server/game/Entities/Player/Player.cpp
@@ -16218,8 +16218,8 @@ void Player::AreaExploredOrEventHappens(uint32 questId)
                 q_status.Explored = true;
                 m_QuestStatusSave[questId] = QUEST_DEFAULT_SAVE_TYPE;

-                // if we cannot complete quest send exploration succeded (to mark exploration on client)
-                if (!CanCompleteQuest(questId))
+                // if we can complete quest, send exploration succeded (to show exploration on client)
+                if (CanCompleteQuest(questId))
                     SendQuestComplete(questId);
             }
         }

Going to test this and see how it works. (Might take a bit of time because I'm also doing other stuff.)

[STARRHELD]

@tkrokli Just output X2 "Objective Complete" on my screen. :/

[MrSmite]

@tkrokli Just output X2 "Objective Complete" on my screen. :/

In the original code (I pasted below), only one would be true (either CanCompleteQuest() or !CanCompleteQuest() and thereby calling either SendQuestComplete() or CompleteQuest().

In the proposed patch, after calling SendQuestComplete(), it falls through and calls CompleteQuest() also because both are checking the same thing: CanCompleteQuest()

What doesn't make sense to me is why would you call SendQuestComplete() if you can't complete the quest anyway (eg: !CanCompleteQuest() is true)? I know all it does is send a client packet but still it seems a bit illogical.

void Player::AreaExploredOrEventHappens(uint32 questId)
{
    if (questId)
    {
        uint16 log_slot = FindQuestSlot(questId);
        if (log_slot < MAX_QUEST_LOG_SIZE)
        {
            QuestStatusData& q_status = m_QuestStatus[questId];

            // Dont complete failed quest
            if (!q_status.Explored && q_status.Status != QUEST_STATUS_FAILED)
            {
                q_status.Explored = true;
                m_QuestStatusSave[questId] = QUEST_DEFAULT_SAVE_TYPE;

                // if we cannot complete quest send exploration succeded (to mark exploration on client)
                if (!CanCompleteQuest(questId))
                    SendQuestComplete(questId);
            }
        }
        if (CanCompleteQuest(questId))
            CompleteQuest(questId);
    }
}

[ghost]

OK, I didn't read the full section, so I suppose you do have a point there about the bad logic.
But I also didn't quite get if you are saying that the existing code does not have a good enough logic.

[xinef1]

--- a/src/server/game/Entities/Player/Player.cpp
+++ b/src/server/game/Entities/Player/Player.cpp
@@ -16217,10 +16217,7 @@ void Player::AreaExploredOrEventHappens(uint32 questId)
             {
                 q_status.Explored = true;
                 m_QuestStatusSave[questId] = QUEST_DEFAULT_SAVE_TYPE;
-
-                // if we cannot complete quest send exploration succeded (to mark exploration on client)
-                if (!CanCompleteQuest(questId))
-                    SendQuestComplete(questId);
+                SendQuestComplete(questId);
             }
         }
         if (CanCompleteQuest(questId))

This should do.

[MrSmite]

But I also didn't quite get if you are saying that the existing code does not have a good enough logic.

What I was talking about was the original code that says "if we can't complete the quest" but then it calls "SendQuestComplete()". Sounds strange to call a quest complete event when you can't complete the quest.

Maybe it was just the comment that is off.