- Status Closed
- Percent Complete
- Task Type Bug Report
- Category
- Assigned To No-one
- Operating System
- Severity Low
- Priority
- Reported Version
- Due in Version Undecided
-
Due Date
Undecided
- Votes
- Private
FS#2408 - Bug in testing responses may prevent npc from responding.
I found something wrong in the code that tests for available response for an npc. Although this might have a small chance of occuring, it is wrong.
In function bool NpcTrigger::HaveAvailableResponses(Client * client, gemNPC * npc, NPCDialogDict * dict, csArray<int> *availableResponseList)
line 1030 and further in dictionary.cpp, the following is done:
When talking to an npc, what you say is parsed several times and a trigger is (possibly) found that corresponds to this. This trigger for this NPC has a list of possible responses (some KA, some quests). In this function, for all these responses it is checked if they are possible at this point. Mainly if they are part of a quest, it is tested if prerequisites are met, such as finished quests, skill, ….
This is simply done by going over all responses, and add them to a list if they are available. A bool hasAvail is set true if at least one response is added.
And that is where something goes wrong. In one of the first tests, hasAvail is set false again if that response is a quest, but not active. However, it should just not add the response. If earlier responses were added (hasAvail is true) and there are no more added after this response (and hasAvail is reset to true), this means there is a list of available responses, but it is not used :)
This will only cause problems in the very rare (and possibly not occuring) case where a single trigger (for example, give me a quest) has more responses (an npc with more quests), and the last of all those answers is not active (for example disabled because of request bug 1423 ).
To fix:
remove haveAvail = false; in line 1043. This makes this if else a bit weird, so a rewrite might be needed.
not really the first line of the function sets it as false so you are right it should be removed, so it’s set to true only if there are answers
committed a fix to r2279
I do not get the ‘not really’ part :)
Anyway, I suggest this to be tested together with
bug 1423(if that does not take too long to be implemented), as some more thorough testing of quests may be wise. Even though this change from looking at the code makes sense, the quest code itself does not always :)not really requiring a consistent rewrite.
what about moving the test case for this in that other bug description and closing this if it works right now so it doesn’t get lost?
Still do not get the ‘not requiring a consistent rewrite’ part :). I can give examples where this would go wrong.
Anyway, I also thought about closing this as it will be tested with the other bug, but only when that would not take months to be implemented :).
I checked the changed code and it looks good (better than what I suggested, as I did not dare re-order more at 2:30 am :) ).
Closing, making this part of
bug 1423.