Missing string replaces in NPC dialogues

[Alfred]

I've been idly looking around the game world and I noticed that some variables, '%Name' in my case, are not substituted in dialogues. I've been looking around the code to find where this might belong and ended up in 'components/compiler/lineparser.cpp' - is this the place where dialogue text is parsed? It seems rather generic, but it is the only place that matches the '%' string in the code. I initially was looking at 'apps/openmw/mwdialogue/dialoguemanager.cpp', namely 'DialogueManager::compile', but it doesn't seem to utilize LineParser, only ScriptParser. I think this would be the ideal place for this kind of code, as all the necessary variables (like the NPC name) are present.

Can somebody point me in the right direction? I haven't seen any related features/bugs on the tracker, so I thought I would try to implement this as a means to get familiar with the code.

EDIT: ScriptParser utilizes LineParser, so it seems I was on the right track.

[Zini]

Nope. Not at all. The string replacement does not only affect dialogues, but nearly all user-visible text. This is actually an extremely complicated topic and probably not suitable for someone who doesn't know the code base very well.

[Zini]

btw. here is the issue. The escape character for these is ^, not %.

[scrawl]

I confirm that in dialogs it shows %Name and not ^Name. Maybe the ones with % are specific to dialogs?

Or % means a context (in this case an NPC), while ^ variables are generic and don't need a context.

[Zini]

Probably another one of those cases where the MW-developers couldn't decide on one syntax and therefore implemented more then one. I suspect % and ^ are synonymous in dialogue texts.

[Zini]

Okay, I thought a bit more about this topic. Maybe we can get at least most of it done. It is still very complex and not exactly a beginner task though.

I see one problem. Topic/quest texts are stored via their ID and not the literal text. Let's say there is a text like "^PCRank" that is shown in the journal. This would show the current rank and not the rank at the time when the journal entry was added. I assume MW doesn't behave this way (can someone confirm?).

A workaround would be to change how these texts are stored. It would inflate memory consumption and saved game size slightly, but probably not so much that it would become a problem. Anyway, this can be taken care of once the remaining implementation is finished.


I suggest to add a new hpp/cpp file pair to apps/openmw/mwgui. These files should provide a single function with the signature:

Code: Select all

std::string translateOutputText (const MWWorld::Ptr& actor, const std::string& text);

See the link in the issue for a list of escape sequences. Read carefully. This is rather tricky. Pay attention especially to the x.y syntax.
Both ^and % should be allowed. As I read the UESPWiki page, the actor argument would be the dialogue partner in dialogues and the player in any other case. We can assume instead that when actor is empty (not pointing to a MW-reference) the player is assumed.

The issue description also contains a list of places where this function needs to be called. We have only three of them yet (books and scrolls don't have a GUi at this time). All the new code should go into the MWGui subsystem.

[Alfred]

Thanks for the feedback, indeed this issue seems to be huge... BTW, I ran a bit around and talked to some other NPCs in different locations and found another string: '%PCname'! In fact I also encountered '%name' - but this is easily covered by case insensitive matching.

The solution you propose seems quite elegant, the only difference is that both interlocutors should be passed (as they are clearly differentiated). I'm not sure all these placeholders should in fact be replaced when stored, it is much easier to only display the correct values by substituting them close to the view layer - this eliminates inflated memory consumption at only a slight overhead, which is justified I think.

Anyway, what I tried today was to modify 'DialogueManager::parseText' as it seems to be called in the right places as far as dialogues are concerned. Here is what I settled on in parseText:

Code: Select all

        //Replace names if present
        try
        {
            size_t npc_pos = find_str_ci(text, (std::string) "%name", 0);
            if (npc_pos)
            {
                text.replace(npc_pos, 5, MWWorld::Class::get(mActor).getName(mActor));
                //
            }
            size_t pc_pos = find_str_ci(text, (std::string) "%pcname", 0);
            if (pc_pos)
            {
                text.replace(pc_pos, 7, MWWorld::Class::get(MWBase::Environment::get().getWorld()->getPlayer().getPlayer()).getName(MWBase::Environment::get().getWorld()->getPlayer().getPlayer()));
            }
        } catch (const std::exception& error)
        {
            printError(std::string("An exception has been thrown: ") + error.what());
        }

This is really simple and only a 'proof of concept', but I still ended up fighting an exception caused by replace, as 'text' is not passed by reference... :/ Right, this issue needs it's own place and a more generic approach - I'll try looking in the places you mentioned.

[scrawl]

Slightly off-topic, but could we use boost for such things? As simple as

Code: Select all

#include <boost/algorithm/string/replace.hpp>
boost::algorithm::replace_all(string, search, replace);

Also, that will replace all occurences and not only the first as in your code.

[Zini]

lightly off-topic, but could we use boost for such things? As simple as

I don't think so. It isn't as simple as just text replacement. There is a more complex syntax behind it that requires proper parsing.


Anyway, I noticed a flaw in my original design. We do not store the actor when storing dialogue topics in the journal. That means we will indeed have to use the workaround I proposed, because else we will run into serious trouble.

That has a few consequences:
- The translate function should be moved from MWGui to MWWorld (since it isn't GUI-specific anymore).
- MessageBox texts should still be translated in the GUI.
- Same for dialogue messages, I guess (would have to look up the code first to say with certainty)
- All kind of journal entries (including journal topics) should be modified in the respective journal classes (JournalEntry and Topic).

The solution you propose seems quite elegant, the only difference is that both interlocutors should be passed (as they are clearly differentiated)

Nope. If there is a dialogue at all, one of them is always the player.


btw. I suggest to start with message box output instead. Much easier to test (open console, type MessageBox "sometext").

[Alfred]

Here is a more generic function for replacements:

Code: Select all

std::string DialogueManager::translateOutputText(const std::string input)
{
    std::string text = input;
    //Replace expressions if present
    if ((find_str_ci(text, (std::string) "%", 0) != std::string::npos)
            || (find_str_ci(text, (std::string) "^", 0) != std::string::npos))
    {
        if (mReplaceMap.empty())
        {
            //build replacement map
            mReplaceMap["%pcname"] = std::string(MWWorld::Class::get(MWBase::Environment::get().getWorld()->getPlayer().getPlayer()).getName(MWBase::Environment::get().getWorld()->getPlayer().getPlayer()));
            mReplaceMap["%name"] = std::string(MWWorld::Class::get(mActor).getName(mActor));
        }

        size_t str_pos;
        for (std::map<std::string, std::string>::iterator ii = mReplaceMap.begin(); ii != mReplaceMap.end(); ++ii)
        {

            while ((str_pos = find_str_ci(text, ii->first, 0)) != std::string::npos)
            {
                try
                {
                    text.replace(str_pos, ii->first.size(), ii->second);
                }
                catch (const std::exception& error)
                {
                    printError(std::string("An exception has been thrown: ") + error.what());
                }
            }

        }
    }
    return text;
}

//calls in dialogue manager:
win->addText(translateOutputText(text));

This is a version that depends on a few class specific variables, such as 'mActor' or 'mReplaceMap'. I tried to save memory by only populating the last one when it is actually needed - this means it needs to be cleared for every new conversation, but otherwise can be reused. This seems to work just fine, I'll add all the reminding variables to the map. Some variables do not actually need to be cleared, so some changes might be in order.

So, should I put this in it's own separate class? I think one singleton would be enough for the whole system, with a 'reset()' method for clearing the replace map, and maybe one for setting an actor for the conversation. Wouldn't that be more efficient than a necessity to pass one every call? I can think of situations (Journal) when no actor would be present for a 'conversation', so NPC replaces could be omitted.

I considered Boost too, but that adds another dependency, right? I had no problems with cmake and Boost because I have it, but is it necessary?