20 Mar, 2010, Davion wrote in the 1st comment:
Votes: 0
I was skimming the ROM source code the other day, and I noticed this little charm!
It's on line 1271 of magic.c under spell_chain_lightning.
damage(ch,ch,dam,sn,DAM_LIGHTNING,TRUE);
level -= 4; /* decrement damage */
if (ch == NULL)
return;


I'm assuming this is an attempt to check if the player dies after he's damaged. This should never really work correctly. Does ROM even have a mechanism to work with this? I suppose you could check ch->position == POS_DEAD.
22 Jul, 2010, Rarva.Riendf wrote in the 2nd comment:
Votes: 0
'I'm assuming this is an attempt to check if the player dies after he's damaged.'
Look like it definitely.

'This should never really work correctly.
Not in stock ROM code for sure.

'Does ROM even have a mechanism to work with this?'
Maybe check for ch->validity for mobs AND ch->position should be the best thing

That explains my post about looping trhough char_list in ROM btw ;p
22 Jul, 2010, Tyche wrote in the 3rd comment:
Votes: 0
It's a hack to fix a bug. You can't reliably dereference ch there after a call to damage().
23 Jul, 2010, Kaz wrote in the 4th comment:
Votes: 0
Tyche said:
It's a hack to fix a bug. You can't reliably dereference ch there after a call to damage().


Unless I'm very much mistaken, I'm pretty sure it's only possible for ch to be NULL in that check if it was NULL going into damage() in the first place. Looks like a non-fix to me (as Davion implies).
23 Jul, 2010, Rarva.Riendf wrote in the 5th comment:
Votes: 0
'Unless I'm very much mistaken, I'm pretty sure it's only possible for ch to be NULL in that check if it was NULL going into damage() in the first place. Looks like a non-fix to me (as Davion implies). '
No there is a way ;p (just kidding, not in stock ROM). The only way could be that ch be a global variable. And it is not.
Anyone with common sense should not pick this code base to start a mud.
23 Jul, 2010, Kjwah wrote in the 6th comment:
Votes: 0
Rarva.Riendf said:
'Unless I'm very much mistaken, I'm pretty sure it's only possible for ch to be NULL in that check if it was NULL going into damage() in the first place. Looks like a non-fix to me (as Davion implies). '
No there is a way ;p (just kidding, not in stock ROM). The only way could be that ch be a global variable. And it is not.
Anyone with common sense should not pick this code base to start a mud.


The quoting feature provided by the mudbytes forum software is your friend. :p

But… To say anyone with common sense should not pick this codebase is a rather silly comment. A lot of people like the feel of ROM and anyone with common sense will choose a base they like and are comfortable with. :D
23 Jul, 2010, Rarva.Riendf wrote in the 7th comment:
Votes: 0
Kjwah said:
But… To say anyone with common sense should not pick this codebase is a rather silly comment. A lot of people like the feel of ROM and anyone with common sense will choose a base they like and are comfortable with. :D

It is silly as you can have the same feel with a lot of codebase, provided you code a little. And there is a lot less work to make another codebase looks and feels like ROM than it is to debug ROM in the first place.
23 Jul, 2010, Scandum wrote in the 8th comment:
Votes: 0
You'd need a form of garbage collection to correctly work around issues like these. Lola has it, not sure about other Dikus.

Rom uses some kind of weird memory system that should allow a ch->position check to work in 100% of the cases.
23 Jul, 2010, Rarva.Riendf wrote in the 9th comment:
Votes: 0
Look at what can happen to a char in the damage method follow to raw_kill, and cry.
ROM has no really working mecanism that works all the time when a char is killed..(it being NPC or PC)
23 Jul, 2010, Tyche wrote in the 10th comment:
Votes: 0
Kaz said:
Unless I'm very much mistaken, I'm pretty sure it's only possible for ch to be NULL in that check if it was NULL going into damage() in the first place. Looks like a non-fix to me (as Davion implies).

No you are right. My bad. It might be wild, but it certainly can't be null.
I do recall that there is a bug in the last ROM release in the chain lightning spell.
Looking at my old mud code, I see the following:
if (ch->in_room)  // <—– fix?
damage(ch,ch,dam,sn,DAM_LIGHTNING,TRUE);
level -= 4; /* decrement damage */

The above must be the fix.
23 Jul, 2010, Rarva.Riendf wrote in the 11th comment:
Votes: 0
No it is not a fix, just a check (that should or could be in the damage method anyway).
23 Jul, 2010, David Haley wrote in the 12th comment:
Votes: 0
Scandum said:
You'd need a form of garbage collection to correctly work around issues like these.

Garbage collection only protects you from accessing invalid pointers here. It does not address the problem of characters sticking around before being deleted. For example, GC will not help you if you kill a character while traversing a room, where that character is further along in the iteration, and then you try to iterate on that character (who is now dead) and have them do something (which they shouldn't).

So GC will help avoid crashes because, by not deleting characters explicitly you do not access deleted memory, but it doesn't solve the bigger problem here.
23 Jul, 2010, Scandum wrote in the 13th comment:
Votes: 0
David Haley said:
So GC will help avoid crashes because, by not deleting characters explicitly you do not access deleted memory, but it doesn't solve the bigger problem here.

A ch_next variable guards against that, though it still fails if two characters are killed at once. You can protect your main loops by using a global ch_next, and do ch_next = ch_next->next if a to be killed character == ch_next, this protects your most important game loops.

In an ideal situation there's no difference between a dead and an alive character, other than their behavior.
23 Jul, 2010, David Haley wrote in the 14th comment:
Votes: 0
No, ch_next doesn't fully guard against it. It only works if you impose somewhat strong limits on iteration, such as only allowing one iteration at a time.

Consider the following: you loop over all characters, one iteration of which triggers a group spell that targets all characters. How can you manage the global ch_next here? Well, really, you can't, unless you have a more sophisticated way of tracking ch_next, using stacks for each level of nested loop, which means you have a fair bit more work to do.

Really, the simplest solution is to mark things as invalid and have a final sweep at the end of the logic tick that discards invalidated things. This lets you have as many loops as you want. If you want extra points, you can even hide the validation check inside an API…

SmartIterator it = room.iterate_chars();
while (it.hasNext()) {
Character c = it.next();
// do stuff
}


This pattern can let you hide the validation check inside 'next' and 'hasNext'. You just check if the next element in the actual list is valid, and skip it if it's not. Therefore, the logical list – the list as far as the code above is concerned – behaves exactly as you would expect.

(Aren't APIs nice…?)

Quote
In an ideal situation there's no difference between a dead and an alive character, other than their behavior.

That's like saying that there's no difference between a plane and a car except that one flies. :wink:
23 Jul, 2010, Rarva.Riendf wrote in the 15th comment:
Votes: 0
David Haley said:
Consider the following: you loop over all characters, one iteration of which triggers a group spell that targets all characters. How can you manage the global ch_next here? Well, really, you can't, unless you have a more sophisticated way of tracking ch_next, using stacks for each level of nested loop, which means you have a fair bit more work to do.

Well actually you can, the second loop has to have global iterator as well.
Since everything that modify the loop will go through a single method (basically extract_char is the one) you put your iterator checks in there.
It seems mad and stupid, but in the end there are not THAT many loops to protect (I counted 7 in my mud)
http://www.mudbytes.net/topic-2940 (would be a duplicate of this one, and EVERY ROM damaging area spell btw)
23 Jul, 2010, David Haley wrote in the 16th comment:
Votes: 0
Well, ok, if you start having one global variable for each possible loop nesting level, you can do it. But this is really not a very good solution, it's messy, it's confusing, and it's dangerous.

A simple API to handle invalidated characters during traversal is far simpler to use, and not hard to implement.
23 Jul, 2010, Rarva.Riendf wrote in the 17th comment:
Votes: 0
[quote=A simple API to handle invalidated characters during traversal is far simpler to use, and not hard to implement.[/quote]

If there was a simple API that would do that, No mud would use delayed extraction to avoid this annoying as hell problem. Basically the topic of my thread you did answer. I was just pointing to Davion that no the ROM code is falwed, and NO there is no solution as the stock ROM code does not handle correctly the possibility of multiple chars being invalidated at the same time.
Also why I say using a stock ROM to start a mud is madness, as the stock code itself is deeply flawed.
23 Jul, 2010, David Haley wrote in the 18th comment:
Votes: 0
The sane iteration API can be written in about 10-30 minutes. It's not hard. And it works with delayed extraction. I'm not sure what the big deal is, really. These songs and dances with global iteration variables are just asking for trouble.
23 Jul, 2010, Rarva.Riendf wrote in the 19th comment:
Votes: 0
'The sane iteration API can be written in about 10-30 minutes. It's not hard. And it works with delayed extraction. I'm not sure what the big deal is, really.'
Integration with existent stock ROM code, there is basically no check about char validity anywhere. Ie, most of the code is already flawed as invalid char are used, it does not crash because the chars only have their validity field put to false, and the rest stay valid, so usable but lead to weird behaviours. A method could call something that invalidate the char, but the line after it use the char like if it was still valid.
I really want to ask , have you already worked with stock ROM code and implement anything that does area damage in it ? If not, I would understand why you find it trivial.
If yes, hell show me the trivial code you did put in to fix this mess, cause I cannot even begin to see the end of it if I start to implement it !
23 Jul, 2010, David Haley wrote in the 20th comment:
Votes: 0
Quote
Integration with existent stock ROM code, there is basically no check about char validity anywhere.

Umm, that's why you would replace the iteration with code that checks if the character is valid. The whole point is that you would not need to push the validity checking down the line – that's exactly what you don't want to do. I already showed what the API would look like previously. Fleshing that out is pretty easy…

SmartIterator it = room.iterate_chars();

–>

Room::iterate_chars():
- construct an iteration state object
- set its current-character pointer to self.first_character_in_room


while (it.hasNext()) {

–>

iterator state object :: hasNext() :
- if current pointer is null: return false
- else:
- if current pointer is valid: return true
- else:
- recursively apply this logic to the next character


Character c = it.next();

–>

iterator state object :: next():
- basically same as above, but modify state while you're at it, and return the character in question



If you pair this with delayed extraction, i.e., a guarantee that lists are never modified during traversal, your iterations will always only touch valid characters as far as the user is concerned. And this will hold even if you have many nested iterations.

But:
Quote
A method could call something that invalidate the char, but the line after it use the char like if it was still valid.

Well, this is a problem pretty much no matter what approach you take. If some operation can invalidate the handle you have, you must check if the handle is still reasonable. If you wanted to get fancy, you could make smart handles that wrap underlying things, and pass through operations if the underlying thing is still valid, and discard operations if the underlying thing has been invalidated.

Quote
I really want to ask , have you already worked with stock ROM code and implement anything that does area damage in it ?

Don't worry, I've done my fair share of MUD (and other) programming. :smile:
0.0/108