set_cur_char()

I'm not certain as too how useful this function is. In my version, its: And is called in several places. Now, as far as I can tell, it is mainly used for calls used during updates, allowing the code to set what person should be looked at by the code through the global variables cur_char, cur_char_died, cur_room, and grobal_retcode, all declared as globals in mud.h. However, in most places that it seems to be calle, it is within a for loop, resetting CH each time that it processes the loop, without making a call to the other routines. It seems to me that this was used simply as a form of short hand, saving some space in ifchecks. If anyone has some deeper insight into the actual use of this function, or there is a good reason that I am totally oblivios too, I would be interested in hearing it. Thanks.

I've been puzzled many times by this function as well. It seems to be one of the many "hacks" in SMAUG.

What it's doing is setting a bunch of global variables. For example, we may want to know what room ch is in, but if ch is gone we can't access ch->room. We also may want to know what ch is currently doing something or being acted on, which is why we store cur_char. Now, why we use a global variable, instead of passing it as a function argument, is quite beyond me.

Personally this is the kind of thing that I really don't like in SMAUG. I've been working on my own codebase which is a derivate of SMAUG, ported to C++ and using classes and nifty STL features. One of my main objectives is to get rid of these magic function calls. To do that I have to study what they are used for, and then make sure that wherever I remove them, I preserve whatever functionality they had in mind when they used them.

The reason that we set cur_char, at least one that I found, is that if we're currently operating on cur_char and another function along the line wants to do something to cur_char, it needs to be aware that cur_char is being used as a node in a list. This is necessary due to the very poor implementation of linked lists (where the nodes themselves contain the data). As a side comment, it's really not good mojo to have ch->next, ch->next_in_room, and obj->next_content, obj->next, obj->next_in_room, etc... just bad stuff, and can make for many bad things.

Sorry if I wasn't very helpful... my main point is that this is one of the cryptic things in SMAUG that shouldn't be there :P

That was my basic idea as well, it may have come from an earlier version derivitive, where it was more useful, but at this point in MUD code, I don't understand the use of it. It may have been to save space, smaller code, but its much less useful, as far as I can tell.

That's very likely, but I would highly recommend against taking it out, as you'd most likely break something very important somewhere. That's why I hate it so much - it's a magic function call that has no apparent and obvious feature, but it might just be fixing one very important thing somewhere - and taking it out could prevent player files from saving quite correctly, or something crazy to that effect. *shrug* Very icky, IMHO.

Because I enjoy the risks.... I'm gonna disable what this does and see what happens. I checked how it's set, and where the resulting cur_char variable is used. It doesn't *SEEM* to be doing much of anything. I've never seen any of the error messages that are associated with it pop up on my mud.

It appears to have something to do with death, but I can't pinpoint what. It could well be that the Smaug team was looking for some heinous linked list corruption that we eventually found and fixed, but God only knows.

I'll report back after letting Valgrid chew on it for a couple of days.

Come to think of it that's entirely possible. There are many, many checks that are seemingly useless spread out all over the code, with bug messages like 'THIS SHOULD NOT HAPPEN'. My bet is that, as you said, they had a vicious bug somewhere they were trying to fix, and they used all those cheapo checks to find it, and then when they DID find it and fixed it, they just didn't take out the checks. *mutter* :)

This theory is bearing itself out as I dig deeper, despite the fact that so far the mud hasn't cared that I disabled set_cur_char.

We find this:



As you can see, it's checking ch against cur_char, and the cur_char_died variable, which is also global. The problem of course as you can well guess is this - char_died is called in infinitely more places than set_cur_char is. The screen filled with spam from grep, and while what I've done right now doesn't seem dangerous, ignoring that it's part of an even more critical check would be foolish so I'm going to undo this test :)

One other piece of evidence, this one fairly interesting since it's the only place in the code where it happens:



Now, in our code, we added a line below the bug statement and supressed the return. My guess would be that since I've not see our extract_char complain in some time that whatever the bug was got fixed.

Users of AFKMud are no doubt familiar with the extra argument tacked on to the extract_char function which is an integer argument that is passed with each call. Each call has a different number, and when passed to extract_char it gives away where the originating call came from. So our segment looks like so:



If memory serves, we only ever had one place in our code that would repeatedly trigger this, and it was with pet saving. Since we also allow multiple pets it's not entirely assured that the bug exists in standard Smaug either. But one never really knows.

On this subject, I can see some merit to the extracted_char_data, as far as I can tell it simply sticks people that have died into a list to be removed. I assume that this is so that other things can be finished in updates before the person is actually removed. However, if this is the case, why not simple remove the person at the time of death, and exit back out of all the update calls, killing them with some checks. Why have the extra union and memory used up? Is this another one of those almost entirely useless bit of code, or am I missing something? Not trying to side track from set_cur_char. Thanks, and please forgive if this is somewhat incoherant.

Oh, the extracted char data is in fact sort of necessary in the SMAUG design, due to the fact that while you're processing a character, it might die and you still might need it around.

You could argue that that's poor design - i.e. if a character dies you should be done with it - but, well, what can you say. :)

I actually do something similar in my new network code, but for slightly different reasons. When a socket is 'removed', it gets pushed onto a 'to delete' queue. At the end of every cycle, the game cleans up that queue, removing items from the main queue. The reason I need to do this is because when you iterate through the STL list, if you remove the iterator you're working on from the list, bad things can happen like no longer being able to iterate! I never access the socket again; I just need to have it (and the iterator) lying around until the end of the loop so that nothing gets broken. Nick implemented a little foreach version that technically fixes this problem (of deleting the iterator you're working on and thereby breaking your iteration), but I haven't been bothered to put it in my code yet. :P

The extra memory used up is minimal... I haven't counted it or anything but it's probably no more than 20 bytes per queue entry. That's not too bad is it :)


That extract_char bug is actually pretty useful in debugging your OWN code, I found, not SMAUG's. Sometimes, if you forget to use the magic words - separate_obj, char_from_room, whatever - and then do things like extract_char/obj on them, you can get strange results. I once somehow (don't ask me how) had an infinite loop (without actually having a loop) of extracting an object which was extracting its char which was extracting its objects... again. Riiiight... so yeah, I think that's why those checks are there.


Now here, for example, the problem I have is that why have two separate kinds of checks to see if the char died? Why is it not sufficient to loop through the extracted char list? Is there a time when the char is killed but not extracted?

I think the answer to this is that when players die, they don't get destroyed and respawn instead. This means, if my memory serves me, that they don't actually ever get pushed onto the extracted queue, which is therefore only used for mobs (or players who actually quit.) Therefore, this 'magic code' is probably used to check if a player died, and if so not do operations on them after they've respawned if they're the one being processed.

Still, I tend to think this is pretty bad form, as a function should know that whatever it is acting on died, without needing a global variable. *mutter*

The only time that I can think of that a function would be called when the person did not start it, and have died, is updates. The main update function runs through the list of characters, and check if they are dead ANYWAYS, so having something like that seems totally redundant. I've had some issues with it before causing crashes, but not really seen anything useful come from it, except for the fact that some of the code needs it. I think I'll go through and try to work out a reasonable way to not use it.

Well, after having consulted with another admin who apparently took it out completely after being told of this thread, I've ripped it out of our code as well.

And you know what? No Bad Things(tm) happened. Not one. No log spam, no crashing, no unpredicatble death behaviour. Considering we have several active mob_progs about that seek to kill each other, I'd say it's relatively safe to remove this stuff.

Valgrind never did flag anything unusual the first time either.