21 Dec, 2008, David Haley wrote in the 61st comment:
Votes: 0
If you have duplicate lists, why are you using a temporary room? What exactly are you doing when you determine that ch and vch are in the same room?
21 Dec, 2008, ghasatta wrote in the 62nd comment:
Votes: 0
DavidHaley said:
If you have a local copy of the list, just iterate over the local copy of the list. (…) So you don't need to restart the iteration from scratch. It would suffice to maintain two parallel traversals, for example.)

I would think that your second 'parallel' traversal is possibly invalidated as you iterate through it, so each time you locate the element you are looking for, you would then do your operation, and then start at the top of the list again. Otherwise you haven't solved the initial problem (namely, the room's list of elements is mutable as you iterate through it).

One of the assumptions that I made in reading through the thread and in writing up my previous post was that the list element, when it is destroyed or changed, has no conception of whether or not it is still valid. If that's not the case, it seems pretty straightforward to test the list element for validity (whatever this may be, presence in a certain room, having a certain state, etc) as you iterate through your local copy of the list, as Scandum suggested.
21 Dec, 2008, David Haley wrote in the 63rd comment:
Votes: 0
I don't think you'd need to start over every time, because the list is invalidated only in very specific ways; anyhow, I don't think it's really relevant right now. Basically, I think we all agree that in the end of the day, you need some way to detect if a given actor has been 'invalidated', and that this test has to be separate from the traversals. I think one of our problems is that we're all discussing half-expressed ideas that are open to a fair bit of interpretation, and we're arguing against the interpretation in our heads as opposed to the what the person actually meant. :smile:
22 Dec, 2008, Scandum wrote in the 64th comment:
Votes: 0
DavidHaley said:
If you have duplicate lists, why are you using a temporary room? What exactly are you doing when you determine that ch and vch are in the same room?

The temporary room should probably be called a junk room, given it's used as a form of garbage collection. If you directly free your mobs the duplicated list will hold invalid pointers, hence you send killed/purged mobs to a junk room or something mutual.
22 Dec, 2008, quixadhal wrote in the 65th comment:
Votes: 0
The bigger issue is that iterating over a list should not change the list structure. It may modify elements in the list, but addition of new elements, or deletion of existing elements should not be done during iteration.

This is akin to the old C practice of:
for( i=0; i< foo; i++) { if( blah(i) ) i = foo; }


Sure, C let you get away with it, but it's bad practice to exit the loop early by fiddling with the loop counter. It's not clear, and it can lead to subtle bugs if more code between the assignment and the bottom of the loop expects i < foo to be true.

ASSUMING nothing elsewhere in the code has an issue with dying bodies continuing to be hit, simply marking them as dead and not doing the extract until the update loop seems fine to me. You don't need to move them out of the room… what can they do before their next execution round? I suppose there might be a few interesting fencepost cases, like a damage reflection shield doing an extra retaliatory blow as someone flogs the dead horse. That might be a good thing. :)

In other words, don't call raw_kill at all until the update loop. Flag them as dead, but let the rest of the code just continue to treat them as a normal living critter… before you check the next command in their queue (or the next combat action they might do), you check their dead flag and THEN do raw_kill, extract_char, etc….
22 Dec, 2008, David Haley wrote in the 66th comment:
Votes: 0
It is possible to make list traversal robust to list modification, if you are careful. It can be very hard to never do anything at all during list traversal; this is more difficult as you add powerful scripting etc. to your game. It's more complicated than just killing people, really: there's also moving them around, for example.
22 Dec, 2008, Sharmair wrote in the 67th comment:
Votes: 0
DavidHaley said:
Well, actually, it depends on how the separate queue is implemented. If it's implemented using its own list structure, you can test if a character has died this round by testing for presence in the queue, but you can still use the next_in_room pointer to go to the next character in that room.

The SMAUG extraction queue system works ALMOST like this stock (changing the reference of round to
pulse), and if you just comment out the two lines that NULL the room list pointers in char_from_room, it
will work EXACTLY like this. I have been doing some tests with this and have not had any crashes or other
problems deleting both the current and next character from a room. This is using no holder at all ei. using
for(vch = room->first_person; vch; vch = vch->next_in_room). This method looks like it solves the
general delete any characters from a list problem. So far it looks like having active room pointers in the
queue does not seem to cause any problems. There still may be the problem of moving characters to other
rooms, but that would not crash (but is still an issue to work on).
60.0/67