26 Oct, 2007, Kristian_sj wrote in the 1st comment:
Votes: 0
Hey.
I could really use some fresh eyes on this bug I am encountering.
Basically I am creating a manuel quest code. I have a bool has_quest_completed [MAX_QUESTS] in the pcdata struct, so that when someone completes a quest, for instance quest number 4, bool has_quest[4] changes to FALSE and bool has_quest_completed[4] changes to TRUE.

The function is declared as:
void modify_quest(CHAR_DATA *ch, int type, bool add,int number) where type is has_quest (defined as 1) or has_quest_completed (defined as 2), bool add is TRUE if we need ot add, otherwise FALSE if remove, number is the quest number.

Now the weird part:
When I want to add a quest to pcdata->has_quest_completed the relevant code is:

case(2)://has_quest_completed
{
if (ch->pcdata->has_quest_completed[number] == TRUE) //already has completed quest
{
bug("Char has already completed the quest: %d.",number);
return;
}
bug("adding compquest %d",number);
ch->pcdata->has_quest_completed[number] = TRUE;
return;
}

Now the weird part, I have had numours checks on either side of the ch->pcdata->has_quest_completed[number] = TRUE; and it all shows that when I for instance throw in number = 1, I get the has_quest_completed[1] set to TRUE, however the character suddenly gets ch->pcdata->has_quest[21] set to TRUE as well.
If I throw in number = 4, the has_quest_completed[4] is set to TRUE as it should, but now the character gets pcdata->has_quest[24] set to TRUE as well.
So for some odd reason it seems that the code set has_quest[2X] (x being the number) to TRUE.

The code is pretty basic so I fear the bug is somewhere else - however the checks I made right before and after the line tells me that, that perticular line is acting odd.

Any ideas anyone ?

Thanks
26 Oct, 2007, Zeno wrote in the 2nd comment:
Votes: 0
Will the code work in other places of the code?

Does this happen to all arrays?
26 Oct, 2007, Kristian_sj wrote in the 3rd comment:
Votes: 0
Just thought I would provide you with the full switch for adding (deleting (add == FALSE) is similar but sets the values to FALSE)
if (add == TRUE) //start with the cases where we add a quest, can_get_quest sanity check has been performed before modify_quest is called
{
switch (type)
{
case(1)://has_quest
{
if (ch->pcdata->has_quest[number] == TRUE) //already has quest
{
bug("Char already has quest: %d added.",number);
break;
}

if (ch->pcdata->has_quest_completed[number] == TRUE) //already has completed quest
{
bug("Char already has completed quest: %d.",number);
break;
}
bug("adding hasquest %d",number);
ch->pcdata->has_quest[number] = TRUE;[i]
break;
}

case(2)://has_quest_completed
{
if (ch->pcdata->has_quest_completed[number] == TRUE) //already has completed quest
{
bug("Char already has completed quest: %d.",number);
break;
}
bug("adding compquest %d",number);
ch->pcdata->has_quest_completed[number] = TRUE;
break;
}
default:
break;
}
return;
}


As far as my testing goes this is the only place that causes trouble, so should be the only array.
I am not sure what you mean by if the code works in other places - if I take the if (add == FALSE) first instead of add == TRUE the error is still the same.
The code removes and adds quests as it should, just somehow the sideeffect explained above happens with the " ch->pcdata->has_quest_completed[number] = TRUE;" line.

(just to clarify) If I take on quest 1, and complete it I get:
qlog
Has quest completed 1
Has quest 21

if I take on quest1 and complete it, and then quest2 and complete it I get:
qlog
Has quest completed 1
Has quest completed 2
Has quest 21
Has quest 22

Somehow ch->pcdata->has_quest_completed[number] = TRUE; sets ch->pcdata->has_quest[2x] = TRUE;
:thinking:
26 Oct, 2007, Davion wrote in the 4th comment:
Votes: 0
Maybe

bug("adding hasquest %d",number);
ch->pcdata->has_quest[number] = TRUE;[i]
break;


this has something to do with it. Remove that
[i]
there. Don't know exactly what it's doing there.
26 Oct, 2007, Kristian_sj wrote in the 5th comment:
Votes: 0
Hmm sorry, I must somehow have misclicked the italic button or something when posting my code.
the is not in the code :)
Doing some more tracing - some things are pretty odd - will update when I narrow down what exactly is odd (besides the apparent problem)

GAAH… found the problem :)

in merc.h I had made the following defines:
bool has_quest [MAX_QUESTS_OBTAINABLE];
bool has_quest_completed [MAX_QUESTS];

where max_quests_obtainable = 20, and has_quest_completed = 100. My intentions was to make the characters only able to have 20 ongoing quests at a time.
That messed things up - changing it to has_quest [MAX_QUESTS] did the trick :)

Sorry for taking up you guys time

[email]null[/email]
15 Nov, 2007, BumpInTheNight wrote in the 6th comment:
Votes: 0
If I could make a recommendation?

Combine the tables and use int instead of bool, then make a few defines:

#define QUEST_NOTDOING 0
#define QUEST_ON 1
#define QUEST_COMPLETE 2

Would save file size and immensely increase simplicity.
15 Nov, 2007, Kristian_sj wrote in the 7th comment:
Votes: 0
Might be better for simplicity however I fail to see how it would save much file size.

I would get something like
1 1
2 0
3 0

for player on quest 1, not on quest 2 not on quest 3 etc. if I understand correctly.

Right now I have for instance, with a player who completed quest 1 and 2:
QC 1
QC 2

in the playerfile, so well, not that much saved I think.

Thank you for your ideas though - different ways to make it I guess.
My code works as intended though and well, I think it is fairly simple, so I don't think I will change the backbone code now :)
15 Nov, 2007, David Haley wrote in the 8th comment:
Votes: 0
You'd save an entry per pfile. But interestingly enough, as suggested, the proposal will *lose* space, since it was suggested that you use one int (= 4 bytes on most systems) instead of two bools (i.e. char = 1 byte). If however you used a single char (which is still enough to hold the three suggested values) it would save you one byte per pfile. Wooo, let's go have a party. :tongue:

As for simplicity, well, if the flags really are mutually exclusive in that they can both be false, or only be true one at a time, then it does make it simpler to have numeric constants. If you have two flags, it could be tempting (or just an accident) to set both to true or something like that.
16 Nov, 2007, Kristian_sj wrote in the 9th comment:
Votes: 0
Hmm, guess you are right about the simplicity.
However I do have sanity checks to avoid the situation of both being on a quest and having completed it. (for instance).
But I do see now how int definiations would have been easier.

Thanks for the headsup but well, as said before, it works flawlessly so far, so unless I notice some funky behaviour I doubt I'll redesign it. At least not at the moment. *mutters something about a long todo list :)*
0.0/9