On Thu, 25 Apr 2002, Daniel Byer wrote:
> Ok, rewrote the code. Now my problems are far, far worse. I tried
> compiling the mud.. didn't work, forgot some semicolons and tried to
> reference a non existent field of a struct. Easy fix. So, it compiled,
> yay, time to test it out. Place the executable in the area folder, cd'ed
> over to it, and did a good ol' "./rom"
>
> "Bus error"
>
> And that was on startup.. didn't even send a single message before I
> received the error.
>
> Doh. Figured it might be because I was initializing the tracking struct
> fields to NULL, then the struct itself to NULL. Changed some values
> around, figured maybe int types can't be NULL, changed them to 0,
> removed the pRoomIndex->tracking = NULL; line.
You can't initialize a structure you don't have. I.E. using a pointer
before you have allocated memory for it. So yes, it probably is related
to "initializing the tracking struct fields to NULL".
You are pretty much correct about the int types not being NULL.
Technically, it will probably compile with only a warning, and run
as you expect. But it is wrong. NULL is a pointer type, not an
int type. NULL doesn't make sense in an integer context.
So you should be setting all the integer fields to 0. (I noticed
that you didn't on most of the code you posted. You should change
them. I won't bother mentioning this again later on).
> Compiled, ran. Bus Error. Then, instead of returning to the prompt, it
> just went to an empty line. So I ctrl+x'ed out of it. All of a sudden,
> another Bus error came up. They are periodically scrolling my screen..
> at the exact moment of this writing, I have -18- bus errors scrolled on
> my screen. *cough*
>
> This happened to me before with some code I was trying to install OLC
> on. So, I figured it was a lost cause, deleted the entire thing, and re
> downloaded stock rom and started my tracking endeavour. I have this
> awful feeling I am doomed to not be able to have a MUD. Evil *nix gods,
> EVIL!
>
> My new and updated code (which seems to work much, much worse than
> before):
> in "db.c" in "void load_rooms( FILE *fp )" right before the "iHash =
> vnum % MAX_KEY_HASH;" line:
> /* begin: zero out the data */
> pRoomIndex->tracking->player = NULL;
> pRoomIndex->tracking->age = 0; /* was NULL */
> pRoomIndex->tracking->dir_to = 0; /* was NULL */
> pRoomIndex->tracking->dir_from = 0; /* was NULL */
> pRoomIndex->tracking->next = NULL;
BAD BAD BAD! Just poking stuff into random memory locations is not
considered good programming practice.
> pRoomIndex->tracking = NULL;
> /* end */
>
> in "db.c" in "void reset_area( AREA_DATA *pArea )" in case 'R' (I was
> told this was in the wrong spot.. why? I would like this code executed
> on every room in every area, and this looks to me like the place to do
> it.. any other recommendations?) after the d0 = 0 for loop:
You gave the answer to "why?" right after it... "I would like this
code executed on every room in every area". That is exactly the reason
that case 'R' is not the place to put this code. Case 'R' will only
run if there is a random exit reset in the room. Unless you want to
have exactly one random exit reset in each room of the mud, this will
not behave as you would like.
It should go somewhere that gets updated once for each and every room.
I haven't done much mud coding lately, so I might have just forgotten,
but I don't remember any stock code that does this. If I am right about
that, you need to add some.
area_update() is a good place for it. Adding a loop that goes
through all rooms in an area and runs this tracking code (Outside of
any loops currently in the function) would do the trick.
> /*
> * This is the tracking reset code. Every PULSE_AREA, we
> update/destroy the tracks
> */
> /* tracks are in room, update them */
> if ( pRoomIndex->tracking ) {
> if ( ( srchTrack = alloc_mem( sizeof
> ( TRACK_DATA ) ) ) != NULL
> && ( prevTrack = alloc_mem( sizeof
> ( TRACK_DATA ) ) ) != NULL
> && ( tmpTrack = alloc_mem( sizeof
> ( TRACK_DATA ) ) ) != NULL ) {
Ok. We just allocated 3 tracking structures and pointed these three
pointers to it. Seems a bit odd to me judging by the names of the
variables, but no errors yet...
> srchTrack = pRoomIndex->tracking;
OOPS! That memory we just allocated a millisecond ago is now lost into
the void, never to be seen again. This is an error.
> prevTrack->age = NULL;
> prevTrack->dir_to = NULL;
> prevTrack->dir_from = NULL;
> prevTrack->player = NULL;
> prevTrack->next = NULL;
>
> firstTrack = TRUE;
> /* go through all tracks, update */
> for ( ; srchTrack; ) {
> /* make sure we don't kill tracks we want to
> keep */
> killTrack = FALSE;
> /* age the tracks */
> srchTrack->age += 1;
> /* keep the last struct so we don't lose it */
> prevTrack = srchTrack;
Right idea, wrong place. You want to keep track of the LAST structure,
so you can delete this one from the linked list. Having another
copy of this same one doesn't help any. This line should be later on
near the end of the loop.
Oh yeah.. And since you allocated memory for prevTrack just now, that
memory just went on a very very long vacation.
> /* this is fun. tracks will "age" based on the
> sector type. forest tracks
> * last longer than tracks in the mountains, so
> we act accordingly
> */
> switch ( pRoomIndex->sector_type ) {
> case 0: case 1:
> /* since PULSE_AREA is the amount of beats
> it takes for the area to age,
> * we do X * PULSE_AREA so track aging is
> dynamic. makes it easy to change
> * the age rate of areas without changing
> track code
> */
> if ( srchTrack->age >= ( 3 * PULSE_AREA ) )
> {
> killTrack = TRUE;
> break;
> }
> case 2: case 4: case 5:
> if ( srchTrack->age >= ( 7 * PULSE_AREA ) )
> {
> killTrack = TRUE;
> break;
> }
> case 3:
> if ( srchTrack->age >= ( 15 * PULSE_AREA ) )
> {
> killTrack = TRUE;
> break;
> }
> case 10:
> if ( srchTrack->age >= ( 10 * PULSE_AREA ) )
> {
> killTrack = TRUE;
> break;
> }
> }
>
> if ( killTrack ) {
> if ( firstTrack ) {
There's a little easier way of keeping track of whether it's the first
one... "if ( srchTrack == pRoomIndex->tracking )" would tell you, and
get rid of the hassle of this firstTrack variable.
> tmpTrack = srchTrack;
I must have missed something. What's this line doing in this code?
And don't forget.. The first time this runs in each room, we just
gave away some more memory allocated a second ago.
> free_mem( pRoomIndex->tracking, sizeof
> ( *pRoomIndex->tracking ) );
> pRoomIndex->tracking = tmpTrack;
Umm.. what? For this code to be run, srchTrack == pRoomIndex->tracking,
and you just set tmpTrack = srchTrack...
So.. After we free the memory pRoomIndex->tracking is pointing to,
you set pRoomIndex->tracking = tmpTrack, which is == pRoomIndex->tracking.
a = 3; b = a; a = b;... Doesn't that seem kind of pointless?
> srchTrack = tmpTrack;
Same comment applies here. srchTrack == tmpTrack already.. You just
made sure of that a couple of lines ago!
All these pointless assignment statements, and you forgot the most
important one... Pointing pRoomIndex->tracking to the next one.
Try this instead:
tmpTrack = pRoomIndex->tracking->next;
free_mem ( pRoomIndex->tracking, sizeof ( *pRoomIndex->tracking ) );
pRoomIndex->tracking = tmpTrack;
Since srchTrack is == pRoomIndex->tracking at this point, you could
also use this code...
tmpTrack = srchTrack->next;
free_mem ( srchTrack, sizeof ( *srchTrack ) );
pRoomIndex->tracking = tmpTrack;
Both logically equivalent. Just please use one of them instead of the
awful mess you have now.
> }
> else {
> prevTrack->next = srchTrack->next;
Good. Except that in the current code, prevTrack == srchTrack.
This line is correct. It's the way you're setting prevTrack that's wrong.
Fix that, and this will work as expected.
> *tmpTrack = *srchTrack;
Ok. You lost me here. Why are you saving a copy of the data you're about
to delete? (And if you deleted the first track in the room already,
tmpTrack isn't pointing to memory you should be changing)
> free_mem( srchTrack, sizeof
> ( *srchTrack ) );
> *srchTrack = *prevTrack->next;
Umm.. what? Delete the memory srchTrack is pointing to, then stick
something in it... Umm.. NO!
This should look more like:
prevTrack->next = srchTrack->next;
free_mem ( srchTrack, sizeof ( *srchTrack ) );
> }
> }
>
> if ( srchTrack != pRoomIndex->tracking ) {
This should never be true if you deleted anything. srchTrack will
be pointing off into memory you gave up all rights to, and
pRoomIndex->tracking will hopefully be pointing to the first tracking
structure (Or NULL if there is not one).
> firstTrack = FALSE;
As I stated before, determining whether it's the first one isn't hard
at all, so this firstTrack variable just unnecessarily makes the code
a little harder to follow
> }
> /* end for */
> }
> /* end if */
> }
> /* end if */
> }
Hmm.. seems like we allocated some memory earlier, and never freed
anything. (Most, if not all of it was already forgotten about, so
we can't free it at this point. But there might still be some
possible way to free a little bit of it by now).
> /* END */
>
> in "act_move.c" in "void move_char( CHAR_DATA *ch, int door, bool
> follow )" between "char_from_room( ch );" and "char_to_room( ch,
> to_room );":
> /* TRACKING CODE. COMPLETE(?). */
> if ( do_track ) {
> send_to_char( "DO_TRACK IS TRUE\n\r", ch );
>
> if ( ( srchTrack = alloc_mem( sizeof( TRACK_DATA ) ) ) !=
> NULL ) {
*Sigh* Not again..
> /* initialize the struct */
> srchTrack->player = NULL;
> srchTrack->dir_to = NULL;
> srchTrack->dir_from = NULL;
> srchTrack->age = NULL;
> srchTrack->next = NULL;
>
> /* Tracking is already in room, so we modify/possibly add to
> it */
> if ( in_room->tracking ) {
> /* Iterate through the list */
> for ( srchTrack = in_room->tracking; srchTrack; ) {
We just threw some more memory into the bottomless pit.
> /* If the tracks are ch's, update them */
> if ( strcmp( srchTrack->player, ch->name ) == 0 ) {
strcmp returns 0 (false) if the strings match.
So you're saying "If this track was NOT left by this player, then update it".
> srchTrack->dir_to = door;
> srchTrack->age = 0;
> }
> else {
> /* If we're at end of list, and no one's been
> found, we add a new entry */
> if ( !srchTrack->next ) {
> if ( ( inTrack = alloc_mem( sizeof
> ( TRACK_DATA ) ) ) != NULL ) {
> inTrack->player = str_dup( ch->name );
> inTrack->age = 0;
> inTrack->dir_to = door;
> inTrack->dir_from = NULL;
> inTrack->next = NULL;
> srchTrack->next = inTrack;
> send_to_char( "Allocated. First
> block.\n\r", ch );
> /* get us out of loop ;) */
> break;
Ah.. very nice.
I was beginning to wonder if you'd get any pointer code right or not.
> }
> }
> else {
> srchTrack = srchTrack->next;
> }
> }
> }
> }
> /* no tracking in room, let's make some! */
> else {
> if ( ( inTrack = alloc_mem( sizeof( TRACK_DATA ) ) ) !=
> NULL ) {
> inTrack->player = str_dup( ch->name );
> inTrack->age = 0;
> inTrack->dir_to = door;
> inTrack->dir_from = NULL;
> inTrack->next = NULL;
> in_room->tracking = inTrack;
> send_to_char( "Allocated. Second block.\n\r", ch );
> }
> }
>
> /* gotta allocate the to_room also */
> if ( to_room->tracking ) {
> /* iterate through the to_room tracks */
> for ( srchTrack = to_room->tracking; srchTrack; ) {
> if ( strcmp( to_room->tracking->player,
> ch->name ) == 0 ) {
Same thing as above.. Invert the test.
> to_room->tracking->dir_from = rev_dir[door];
> to_room->tracking->age = 0;
Eh? If the 3rd track belongs to this guy.. then we update.. the first one...
> }
> else {
> if ( !srchTrack->next ) {
> if ( ( toTrack = alloc_mem( sizeof
> ( TRACK_DATA ) ) ) != NULL ) {
> toTrack->player = str_dup( ch->name );
> toTrack->age = 0;
> toTrack->dir_to = NULL;
> toTrack->dir_from = rev_dir[door];
> toTrack->next = NULL;
> srchTrack->next = toTrack;
> send_to_char( "Allocated. Third
> block.\n\r", ch );
> break;
> }
> }
> else {
> srchTrack = srchTrack->next;
> }
> }
> }
> }
> else {
> /* no tracking in room, make tracks */
> if ( ( toTrack = alloc_mem( sizeof( TRACK_DATA ) ) ) !=
> NULL ) {
> toTrack->player = str_dup( ch->name );
> toTrack->age = 0;
> toTrack->dir_to = NULL;
> toTrack->dir_from = rev_dir[door];
> toTrack->next = NULL;
> to_room->tracking = toTrack;
> send_to_char( "Allocated. Fourth block.\n\r", ch );
> }
> }
>
> }
> }
> /* END TRACKING CODE */
>
> A note of comment. I switched the "player" field if TRACK_DATA to a
> char * instead of CHAR_DATA * so we can just use the player's name. This
> way, I can actually keep tracks around when the player logs off, piece
> of cake. In fact, that's the way it's currently set up.. I'd have to
> make changes in order to prevent the tracks from being permanent (blah
> :P).
This isn't extremely efficient (Lots of string comparisons are involved).
One possible improvement would to keep track of the player's ID also.
You will still have to do the string comparisons when someone looks at
the tracks (I don't know a better way), but then you could just compare
integers when somebody moves and save a couple of string comparisons.
I wouldn't worry about it since it probably would have a negligible
effect on the mud's performance, but I just thought I'd throw the idea
in there.
> Well, let me know what your comments are this time.. by the way, I
> -really- appreciate all your help. I'm at least gaining good programming
> experience destroying my operating system ;)
It's a little harder than that to destroy your OS. That's why you're
getting the Bus Errors. The OS is protecting itself from you.
Now if you tried running the same code in an older version of DOS/Windows...
That would destroy the operating system.
> Oh, and I just stumbled across the reset commends. No wonder 'R' is a
> bad place to put it. Would 'D' be better?
Nope. It would run a lot more (several times at once in some rooms),
but still not once per room.
Just stay away from the resets in general! They have nothing in
common with your code.
> I'm sure some of these little
> issues, like where it's placed, is probably making all the difference
> right now.. i'm fairly confident my tracking code is decently stable ;)
I can't say that I share that same confidence. :)
> And for the heck of it:
> struct track_data
> {
> char * player; /* player to pass through exit */
> sh_int age; /* age of tracks */
> sh_int dir_to; /* direction tracks go to */
> sh_int dir_from; /* direction tracks come from */
> TRACK_DATA * next; /* next tracking data, say, if another
> player passes through exit */
> };
>
> Another note about the bus error.. it happens, seriously, before I
> receive ANY feedback the mud startup normally gives (resets, mobs, etc).
> It's like, something completely different is going wrong. Because it
> won't even start trying to work. Who knows, maybe that's the way it DOES
> work.. *hopes so*
>
> --Daniel, the long winded.
My main recommendation would have to be to take a break from coding for a
while and study up on pointers. If you understood how pointers work
and how to use them correctly, the vast majority of the errors in this
code would be taken care of.
Dennis
