Fix for QuestListsManager.GetQuest

[Jagget]

QuestListsManager.GetQuest() will check mods for quests as well.
I removed if (File.Exists(questFile)) check, because LoadQuest() function also checks for that, and more importantly, checking mods for quest files included.
Then, if we didn't find any, we proceed as usual.

[KABoissonneault]

Can you give an example of a use case that doesn't work as expected without this change?

[Jagget]

Easy. RLQ or QP1 by @JayH2971 :)
Main quests are added in QuestList-ZZZ.txt, but not all of them. Some quests are conditional and used as consequences. So then the main quest starts others using "start quest" action.

[Jagget]

The top condition if (File.Exists(questFile)) is false, but then the game check "registered" quests. Hence, the game can't load unregistered quests. This works if Quest Pack is shipped as bunch of files, because the top condition if (File.Exists(questFile)) if true.

[KABoissonneault]

I suppose you mean that QP1 wants to use "startquest" to start quests that are not part of a QuestList, and that it works if QP1 is loose files, but does not work if it's a dfmod.

The impact seems fine. QuestLIstsManager.GetQuest is used by console commands, those RunQuest/StartQuest quest commands, and some daedra summoning procedure. I don't think this would impact cases where failure to resolve a dfmod quest is intended, so should be good

[Jagget]

RunQuest/StartQuest console commands also doesn't work, if the quest is not in the quest list. The quest action "start quest" also doesn't work, if the quest is not in the quest list. I mean it does, if the quest file is just a loose file, but if the quest file is in the dfmod, then all that jazz doesn't work

[ajrb]

So I've been dredging my memory for whether there was any good reason for me to have done it this way, in case it's a good reason, because it was definitely intentional. I can't remember anything and my conclusion is just that I was thinking of quest lists as fixed lists of all quests in a mod and very different from quest packed loose files. I can be very rigid in my thinking, I like definitive on/off definitions. Anyway I think this is all good, thanks for waiting for me to search my memory.

[KABoissonneault]

I have a bit of concerns with the approach of this change, but I don't think it's unfixable. The main problem is that I think it tries to fetch the mod asset too early, preventing init, guilds, social from doing the resolution themselves if they are available there.

I'm posting my pseudocode notes for what's involved

QuestData
	name
	path
	group
	membership
	minReq
	oneTime
	adult

LoadQuest(questName, questPath, factionId)
	LoadQuest(new QuestData() { name = questName, path = questPath }, factionId)

LoadQuest(questData, factionId, partialParse=false)
	questName = questData.name + QExt
	questFile = questData.path + questName exists
	if loose questFile exists
		quest = QuestMachine.Instance.ParseQuest(questName, File.ReadAllLine(questFile), factionId, partialParse)
	else if ModManager.Instance.TryGetAsset(questName, clone: false, out questAsset)
		quest = QuestMachine.Instance.ParseQuest(questName, ModManager.GetTextAssetLines(questAsset), factionId, partialParse)
	else
		throw new Exception("Quest file not found")
	quest.OneTime = questData.oneTime
	return quest

Note that LoadQuest in the loose file case only tries a specific path, but for mod files, it gets the quest file by name, no matter what folder it's in.
Another thing to note is that if you call LoadQuest(questName) rather than LoadQuest(questData), you lose the "one time" flag that might have been in the quest data.

So in terms of GetQuest.

Before:

if loose questName + QExt exists
	LoadQuest(questName, QuestMachine.QuestSourceFolder, factionId)
if init contains questData with name == questName
	LoadQuest(QuestData, factionId)
if init contains questData with loose path + questName + QExt that exists
	LoadQuest(questName, questData.path, factionId)
if guilds contains questData with name == questName
	LoadQuest(questData, factionId)
if social contains questData with name == questName
	LoadQuest(questData, factionId)

After:

quest = LoadQuest(questName, QuestMachine.QuestSourFolder, factionId)
if quest
	return quest
if init contains questData with name == questName
	LoadQuest(QuestData, factionId)
if init contains questData with loose path + questName + QExt that exists
	LoadQuest(questName, questData.path, factionId)
if guilds contains questData with name == questName
	LoadQuest(questData, factionId)
if social contains questData with name == questName
	LoadQuest(questData, factionId)

I fear in the latter case, all the checks for init / guilds / social are essentially ignored, because LoadQuest is greedy for mod quests, and always picks them up no matter which folder it's in. Though the only thing we risk losing is the oneTime flag, I think it's important enough not to ignore.

My suggested approach: add the load from mods at the end. Give the registered quests a chance to resolve first, and then pick an unregistered file at the end.

I don't like how DFU always picks mod assets by name without care for the relative folder they're in, but ideally we should only do the "pick unregistered file" from the Quests folder, like we do for loose files. I don't expect it from this PR, as I know it's not a thing DFU does well. Worth investigating for later.

[Jagget]

if init contains questData with name == questName
	LoadQuest(QuestData, factionId)
if init contains questData with loose path + questName + QExt that exists
	LoadQuest(questName, questData.path, factionId)

this part is actually have more to that, if the loose file exist, and we are asking to LoadQuest(questName), all mods will be checked if the replacement exist. But if there's no loose file, then mod replacements will be ignored.

---

What about this variant?

public Quest GetQuest(string questName, int factionId = 0)
{
    string questFileName = questName + QExt;
    // Check each registered init quest & containing folder.
    foreach (QuestData questData in init)
    {
        if (questData.name == questName)
            return LoadQuest(questData, factionId);

        string questFile = Path.Combine(questData.path, questFileName);

        Quest initQuest = LoadQuest(questFile, questData.path, factionId);
        if (initQuest != null)
            return initQuest;
    }

    // Check guild quests.
    foreach (FactionFile.GuildGroups guildGroup in guilds.Keys)
        foreach (QuestData questData in guilds[guildGroup])
            if (questData.name == questName)
                return LoadQuest(questData, factionId);

    // Check social quests.
    foreach (FactionFile.SocialGroups socialGroup in social.Keys)
        foreach (QuestData questData in social[socialGroup])
            if (questData.name == questName)
                return LoadQuest(questData, factionId);
    
    // Check QuestSourceFolder containing classic quests and mods.
    Quest quest = LoadQuest(questName, QuestMachine.QuestSourceFolder, factionId);
    return quest;
}

[KABoissonneault]

I think you should still keep the loose file check at the top, as that should have priority over registered mod quests. It's not a great feature, but no reason to make this inconsistent with how loose files generally work.
I know there's lots of redundant checks in there. If you wanna make some private "LoadLooseQuest" vs "LoadModQuest" helpers that only does one or the other, it could make it more explicit and have less double checking.

[ajrb]

Firstly thanks for spotting that unintended consequence KAB.

I'm keen for the classic quests to get handled before looking in mods or questlists etc. I think by loose files you mean classic quests since they're all loose. People could put new quests in there but I created the quest packs folder to keep them separate. Packs have to have quest lists that are auto-registered so they also don't allow loading.

The aim of this PR is (I think) to allow mod quest scripts to get loaded using the same mechanism that loads classic (loose) quests by creating a temporary QuestData object with the name and the classic quests asset folder path. This does check mods too for that quest filename but that was only intended for when a registered quest (from a list) is being sought. Hence why the file check was done first before doing the call to LoadQuest.

As you probably know from discord my preferred solution is that mods register lists of all their quests, and for those that shouldn't be offered should be set as requiring 101 rep, which should not happen since 100 is max. This seems like a lot of effort to avoid adding a single line to a file by mod authors but maybe I am missing the point like I missed what KAB caught?

[KABoissonneault]

I think by loose files you mean classic quests since they're all loose

I do admit that I was mixing up two things. The check for

string questFile = Path.Combine(QuestMachine.QuestSourceFolder, questFileName);

is classic quests only. This is not what I intended to mean by "loose quests".

I've had another look, and I have a better understanding of the problem that this PR is trying to solve.

Take Jehuty's Random Little Quests (RLQ).

There are 88 quests under QuestPacks/JHRLQ, but only one is found in QuestList-RLQ.txt: RLQinit. RLQInit.txt itself calls something like start quest RLQvampireencounter to start those quests.
For this reason, I call quests that are not part of a quest list "loose", they are simply not registered anywhere in the QuestListManager. QuestListsManager.GetQuest can pick up those quests despite that through the following code

            // Check each registered init quest & containing folder.
            foreach (QuestData questData in init)
            {
                ...

                questFile = Path.Combine(questData.path, questFileName); <-- we use RLQInit's qestData.path
                if (File.Exists(questFile))
                    return LoadQuest(questName, questData.path, factionId);
            }

RLQInit is an init quest, and therefore DFU will try to look for quest files in QuestPacks/JHRLQ.

The issue described by Jagget, as I understand it, is that if Jehuty takes all those files and puts them in a dfmod, then they are completely missed by this code. RLQInit will be loaded properly (because Jagget's last PR allowed for QuestLists in dfmods to auto-register), but the check for a loose quest will only work for files in QuestPacks, not in a dfmod.

We're back to my previous complaint that there is not good solution for DFU to check for "QuestPacks/JHRLQ" in a dfmod. The only thing we can do is check for "RLQvampireencounter.txt" anywhere in any mod if anyone tries to call start quest RLQvampireencounter.

I'm honestly not a fan of this. I think we'll easily end up picking random text files that are not quests. But something needs to be done for the transition to dfmods to work.

An idea:

Given that the mod builder knows that QuestList-RLQ.txt is a quest list, and that it's auto-registered with the new ModInfo Contributes... couldn't we add an extra field that tracks "loose quests" in the folder of each auto-registered QuestList? These quests would be picked up by QuestListManager as an extra field like mod.ModInfo.Contributes.LooseQuests, with both the "quest path" and the "quest name".

public void DiscoverQuestPackLists()
{
    ...

    foreach (var mod in ModManager.Instance.GetAllModsWithContributes(x => x.QuestLists != null))
    {
        foreach (var questList in mod.ModInfo.Contributes.QuestLists)
        {
            if (!RegisterQuestList(questList))
            {
                Debug.LogErrorFormat("QuestList {0} is already registered.", questList);
            }
        }

       foreach (var looseQuest in mod.ModInfo.Contributes.LooseQuests)
       {
            looseQuests.Add(new LooseQuest { looseQuest.path, looseQuest.name });
       }
    }
}

Then, we would simply add the following code in GetQuest

            // Check each registered init quest & containing folder.
            foreach (QuestData questData in init)
            {
                if (questData.name == questName)
                    return LoadQuest(questData, factionId);

                questFile = Path.Combine(questData.path, questFileName);
                if (File.Exists(questFile))
                    return LoadQuest(questName, questData.path, factionId);

               // Check loose quests in mods
               if(loose quests contains quest where quest.path == questData.path and quest.name == questName)
                   return LoadQuest(questName, questData.path, factionId);
            }

This solution avoids adding literally every text file as a potential quest to be picked up here, and mirrors the way StreamingAssets mods work (by adding the folder of init quests as a valid place to pick up quests).

[Jagget]

Am I right, that there are no paths inside of dfmod? Even the code that is parsing QuestList from the mods, don't pass path in, because it can't.

// Seek from mods using pattern: QuestList-<packName>.txt
TextAsset questListAsset;
string fileName = QuestListPrefix + questList + QExt;
if (ModManager.Instance != null && ModManager.Instance.TryGetAsset(fileName, false, out questListAsset))
{
    try {
        List<string> lines = ModManager.GetTextAssetLines(questListAsset);
        Table table = new Table(lines.ToArray());
        ParseQuestList(table);
    } catch (Exception ex) {
        Debug.LogErrorFormat("QuestListsManager unable to parse quest list table {0} with exception message {1}", questListAsset.name, ex.Message);
    }
}

and inside the ParseQuestList(table) method you'll find questData.path = ""

So it will all come down to this evetually, that assets from the dfmod could be loaded only by the filename?

[KABoissonneault]

Yeah sorry, scratch the "path" part. Too complicated for no reason.

When auto-registering quest lists, just store a list of quest names that are in the same folder as one of the quest lists. Then, when loading that quest name with GetQuest, just check if there's any loose quest with that name.
The goal is really just to make sure we're not grabbing some random txt file, and that we're checking if a registered quest file exists before we try doing a mod file lookup.

[ajrb]

Sounds like a good middle ground to me KAB.

[KABoissonneault]

I finally got around to running this. Seems good. I'll probably send a test build to JayH before release to make sure we don't have one last thing to fix before quest mods can properly migrate to dfmods