ID:1922456
 
(See the best response by Kaiochao.)
Code:
mob
Click()
if(src.client)
return
if(src.NPC)
walk_to(usr,src)
else
walk_to(usr,src)
usr.dir = src.dir
sleep(10)
usr.Attack(src)

mob
proc
Attack(mob/M)
while(M in oview(1))
var/D = (usr.Strength + rand(0,3)) - (M.Defence + rand(0,3))
M.Health -= D
world<< "[M] was hit for [D] damage!"
M.Attack(usr)
sleep(20)


Problem description:
Everything else work but enemy doesn't attack back... why is it and how could i make it so they do.
Well, first off, it seems you have an infinite loop going on there. If you absolutely want to do combat that way, you should do it like this:
mob
proc
Attack(mob/M)
if(M && (M in oview(1, src))) //Make sure to check if M still exists for something that lasts more than one frame or may be called on nothing. Also, no while loop if you're using recursion, because they will just keep going back and forth anyways.
var/D = (src.Strength + rand(0,3)) - (M.Defence + rand(0,3))
M.Health -= D
world<< "[M] was hit for [D] damage!"
sleep(20)
if(M) //Make sure this guy still exists. The previous check for M may not even be necessary, depending on your code. But, I'm playing it safe. I think the proc should automatically terminate if src gets deleted, so no need to check for src here (though sometimes I like to do that anyways. Probably because of C++ pointers.)
M.Attack(src) //Attack AFTER the delay.


Please note that doing a battle this way may also hit the built-in recursion limit for world.loop_checks, I'm not sure. There are better ways to go about a turn-based system, though, so I wouldn't use the method you're using for an actual project. Unless it's like, a simple MUD or something, but even then I'd refine it so that it doesn't use recursion. Also, it is good practice not to call usr in procs unless you absolutely know what you're doing with it.
In response to Kamuna
Recursion done in a language like this wasteful and you shouldn't use it to solve this problem.
In response to Zecronious
Zecronious wrote:
Recursion done in a language like this wasteful and you shouldn't use it to solve this problem.

I'm not writing his code for him, I'm merely fixing it and telling him what my fixes are about. I just said I wouldn't use recursion. I dislike using recursion. If he wants to know of different ways to handle this problem, I promote him doing some research, instead of handing it to him on a silver platter. We're never going to get better programmers for BYOND that way. At least we'll know if he's serious about this if he goes and researches, if not, oh well, handing it to him would have the same effect as him not wanting to research, only making his game slightly better in the process for little work on his end. And what's worse than him not being serious is him being a moocher. So I stand by my ground.

Also, if you're going to say this, at least help this guy with his problem instead of telling me how to do it.
In response to Kamuna
We're also not going to get better programmers if you tell them to use recursion like that.

Here's an example of how I would handle this problem using loops.

mob/NPC
icon_state = "NPC"

Click()
// Begin attacking each other
usr.Attack_Loop(src)
src.Attack_Loop(usr)

mob
icon = 'Icons.dmi'
icon_state = "Player"

var
Strength = 50
Defence = 10
Health = 100
HealthCap = 100
Attack_Delay = 15

attacking = 0 // Set to zero at any time to stop attacking

proc
Attack_Loop(mob/M)
if(attacking) return
else attacking = 1
spawn()
while(attacking && (M in oview(1,src)) && !M.invisibility)
var/Damage = (Strength + rand(0,3)) - (M.Defence + rand(0,3))

if(Damage > 0)
world << "[M.name] was hit for [Damage] damage!"

if(!M.Take_Damage(Damage))
world << "[M.name] was killed by [name]!\n"

sleep(Attack_Delay)

attacking = 0

Take_Damage(Amount)
Health -= Amount
if(Health <= 0)
spawn() Die()
return 0
else
return 1

Die()
attacking = 0
Move(locate(1,1,1))
Health = HealthCap

Move()
attacking = 0
..()

mob/NPC
var/respawnTime = 100

Die()
attacking = 0

density = 0
invisibility = 101
Health = HealthCap

Move(initial(loc))
sleep(respawnTime)

density = 1
invisibility = 0
In response to Zecronious
I did not tell him to use recursion. I advised against it. Also, you didn't explain what your changes did, and you're just handing out code that may not be plug and play or even fit in his game. Please don't try to write his game for him.

That's also pretty messy code that probably could be simplified. For example, the

            if(attacking) return
else attacking = 1


could look better (but that's just personal opinion), and I advise against using spawn all willy nilly. You can use set waitfor = 0 if you really need to.

All that extra stuff isn't necessary to solve his problem. Just fix the problem and move on, if he has more questions, let him ask. Giving that all to him at once is only going to make him more confused. Also, the click code is changed to something he doesn't want (he obviously wants them to walk up to the mob first). He may have done it in an odd way, and I probably should have mentioned that he should research and/or ask that too. Oh, and by the way, it seems like they attack at the same time, instead of one after the other like it seems like he wanted in his original code. He didn't even ask for invisibility checks either, maybe he doesn't want that, who knows. Why do they become invisible on death? And way more... But I'm not going to get into it. Just know that that's ANOTHER way I don't want him to code. It seems like bad practice all over the place in that code, and you're just giving it to him and not asking him to research.
Best response
Recursion is when a proc calls itself. A while() loop is not recursion, and it isn't always an infinite loop.

OP: Your Attack() contains usr abuse. To refer to the object that owns the current proc, you use src, not usr.
I'll comment on your Attack():
        // This proc represents "src attacks M"
Attack(mob/M)

// "while M is visible to usr within 1 tile (and isn't usr)"
// The problem here is that oview() defaults to usr.
while(M in oview(1))

// usr.Strength is invalid for NPCs.
var/D = (usr.Strength + rand(0,3)) - (M.Defence + rand(0,3))

M.Health -= D
world<< "[M] was hit for [D] damage!"

// Make M (the one being attacked) attack usr (the player who initiated combat)
M.Attack(usr)

sleep(20)

When a player clicks an NPC, player.Attack(NPC) is called.

Inside player.Attack(NPC),
* src == player
* usr == player
* M == NPC
=> M.Attack(usr) becomes NPC.Attack(player).
=> while(M in oview(1)) becomes while(NPC in oview(1, player)). It ends up being correct in this case.
* * oview(1) becomes oview(1, usr) becomes oview(1, player).

Inside NPC.Attack(player),
* src == NPC
* usr == player
* M == player
=> while(M in oview(1)) becomes while(player in oview(1, player)). This is bad.
* => The while() condition fails immediately because Ref is never in oview(Range, Ref). This is essentially why NPCs never attack back.

To fix the usr abuse in this case, you need to keep track of what objects you really want to have involved. You can use descriptive variable names to make your code more readable:
        // "src attacks the Target"
Attack(mob/Target)

// "while the Target is in oview(1, src), repeat this code:"
while(Target in oview(1, src))

// "this is the damage calculation"
// "it involves the src's Strength and the Target's Defence"
var damage = (Strength + rand(0,3)) - (Target.Defence + rand(0,3))

// "Target's health is reduced by the damage amount"
Target.Health -= damage

// "Tell the world that the Target took damage"
world<< "[Target] was hit for [damage] damage!"

// "Make the Target attack src"
Target.Attack(src)

// Wait for 2 seconds
sleep 20

// Now, repeat the loop.

If you use the code above, the condition won't fail unexpectedly, but there's still a problem with it: if A attacks B, then because of the line "Target.Attack(src)", B will immediately attack A, which will cause A to attack B again, and so on. This is an infinite loop, although it's slightly less direct (because Attack() is being juggled between the two objects, A and B), and it has nothing to do with the while() loop. Worse, this all happens before that sleep(20), so there's no time between attacks, and the game will freeze. The overall logic of your combat system is broken.

So, that's the thread. Your Attack() needs to be redesigned and rewritten from scratch. If you would like help with that, feel free to describe the combat system you intended.
In response to Kaiochao
Yes, and his code had recursion in it because it made A attack B, and then B immediately attack A, and then A attack B again (it is going to another proc and then coming back to itself, arguably making it recursion), which caused an infinite loop as well because of that and because he used a while loop in the way that he did. He also called M.Attack(usr) before sleep(20), and they generally won't be able to move away in time for the condition to turn false. I was not talking about the while loop ^^;

The while loop was unwarranted in this case too, as an if statement does what he wants (or what it seems like he wants). If he wanted to do a while loop, he'd have to separate the attack logic and make the clicking call an attack loop, which does (if it is attached to the mob instead of being a global proc):

    while(M && (M in oview(1, src)))
src.Attack(M) // Can leave src. out if you want.
sleep(20)
M.Attack(src)
sleep(20)


...To make it super simple.

Yeah, he does need to do some rewriting. I think he'd be better off telling his design in Design Philosophy and we point him to good sources there, then he can go to Developer Help if he's really stuck on something. But I dunno, helping him here works too, but I'm afraid it'd derail the topic a bit.
In response to Kamuna
Kamuna wrote:
I did not tell him to use recursion.

Let me quote you telling him to use recursion.
you should do it like this:


That's also pretty messy code that probably could be simplified.

Not at all. It's minimal, functional and organised. If believe it could be done better then I welcome you to make your edits to it.


For example, the

>             if(attacking) return
> else attacking = 1
>

could look better (but that's just personal opinion)

If it could look better then tell me how you would do it?


I advise against using spawn all willy nilly. You can use set waitfor = 0 if you really need to.

1. I'm not using spawn() willy nilly.
2. If you wanna take out any of my spawns() then go ahead and come to the realisation that they're there because you need them.

All that extra stuff isn't necessary to solve his problem.

Extra stuff? What extra stuff are you talking about?


Just fix the problem and move on

I just did...


if he has more questions, let him ask.
Am I standing in his way of asking further questions?

Giving that all to him at once is only going to make him more confused.
No... It'll make him think about how it works.

Also, the click code is changed to something he doesn't want (he obviously wants them to walk up to the mob first).
He can work that out. He already worked it out once already. I thought you said I shouldn't do it all for him? Gosh.


He may have done it in an odd way, and I probably should have mentioned that he should research and/or ask that too.

Yeah I think you probably should've mentioned that.

Oh, and by the way, it seems like they attack at the same time, instead of one after the other like it seems like he wanted in his original code.

Again, he can figure that out. It's exactly the same as what he already did. I thought you said I shouldn't do everything for him? Stick with a point.

He didn't even ask for invisibility checks either, maybe he doesn't want that, who knows.
You obviously don't understand why they're there, does that really make you the best person to criticize them?
They're there because you don't want to be able to attack a mob which has respawned and is remaining dormant until it reappears.


Just know that that's ANOTHER way I don't want him to code.
What way? The correct way? The faster way? The more modular way? The just all around better way.

Come on you're jelly.


It seems like bad practice all over the place in that code, and you're just giving it to him and not asking him to research.

Then point out any bad practices you can find. Oh you can't find any and that's why you didn't mention any that I've broken.

You don't even understand anything I wrote and yet you're trying to convince me that my code is somehow inferior to yours... Oh gosh, I wish you didn't have be so jelly. There's too many people like you here. Soon as someone says you're wrong you have to throw a stupid fit.
I'm not going to repeat myself, I just pointed out some of the things that were bad practices, and I'm not going to sit here and review all of your code. As for extras, for example, dying is not something he asked for. And uh, that code is correct, as there are multiple ways to solve a problem, it just has its flaws (as does the one I posted, but I did that on purpose as to only fix his single, easy to fix problem, not his design or way of coding). I'm not throwing a fit, and I'm not saying that my way is the best way. And I definitely didn't say the code I posted was in any way good, because it wasn't, all I did was fix his problem and tell him what he should do to further his knowledge. I'm sorry that you took it personally or the wrong way, that was not my intention ^^

I just want us all to get along, work together, and learn together, so please do not get angry so easily.

And, personally, I would keep the code formatted uniformly, at least in that one file, and not use else when it isn't needed, and use TRUE and FALSE for booleans so that I can remember that it is a boolean in the future:

            if(attacking)
return
attacking = TRUE

//OR if you really want it on the same line:

if(attacking) return
attacking = TRUE


But as I've said, that's just personal taste. Sorry!
I think the tone of the thread could use a lighter touch, especially on Zecronious's end. It isn't the end of the world if one particular developer does it a particular way. Kamuna did mention that he would do it a different way depending on his programming style. I have my own way of tackling the issue OP mentioned. There's always more than one answer.
In response to Zecronious
I forgot to place that post as a response, so I'll just post this one here.

And, for future reference, I'm a girl.

But yeah, I'm really sorry for helping to derail the topic, I just didn't want him to be babied and get into the habit of having most of his code written for him. But, just let the OP decide what is best for him. Perhaps he'll use a mix of our methods, one of our methods, or none of our methods at all. I'm just here to give simple advice. My intention was to make one post and let everyone else do their own thing, but I didn't expect to be responded to, especially not in what I believe to be such a passive aggressive manner. However, I do know some of the traps, so I try everything in my power not to let it flame up, plus I try to keep an open and positive mind when first talking to strangers. If I don't like you, I won't talk junk to you or about you, I'll just let you do you. I wish to have a nice community anyways, so I also encourage keeping hate to a minimum, and if you must talk to someone you hate, don't let your emotions come out. But that's just me!
Kamuna:
And, for future reference, I'm a girl.
My apologies. I like your thinking. I am the same way in that I prefer to keep things in an optimistic tone to avoid drama and negativity wherever possible. Sp kudos to you.
In response to Kaiochao

So, that's the thread. Your Attack() needs to be redesigned and rewritten from scratch. If you would like help with that, feel free to describe the combat system you intended.

Well the combat im going for is something like how runescape has it but really not. Instead there are different skills you can do when attacking and such.

Ps: Thank you Kamuna and Zecronious for giving advice and play nice.
mob/proc

Attack(mob/Target)
while(Target in oview(1,src))
if(!Attacking && !Target.NPC)
var damage = (Strength + rand(0,3)) - (Target.Defence + rand(0,3))
if(damage <=0)
damage = 0
world<< "[Target] was hit for [damage] damage!"
Attacking = 1
sleep(30)
Attacking = 0
Target.Attacking = 0
Target.Attack(src)
else
world<< "[Target] was hit for [damage] damage!"
Target.Health -= damage
Attacking = 1
sleep(30)
Attacking = 0
Target.Attacking = 0
Target.Attack(src)
else
return


This seems to work.. But is it flawed? Also how would i make so that if the src or the target gets out of range to stop attacking. With sleep there is a delay so if you walk away then walk back before the sleep can process you get attacked again. Do i have to do something totally different?
In response to Kidman90
You should be able to describe what you want in words before you translate it into DM. It might be easier for you to debug your logic when it's in the language you understand best.

This Attack() has code that doesn't need to be repeated. In general, if you have something like this:
if(A)
B
C
else
D
C // C happens either way!

It can be reduced to:
if(A)
B
else
D
C

Also, the first if() might as well be inside the while() condition.

Those were pretty much stylistic issues, though.

If it's doing something that you don't want, then yeah, it's flawed. There are many ways to get what you want, though. For example, you could split the sleep(30) into 30 sleep(1) and check for the target more frequently.
In response to Kidman90
//This #define (preprocessor definition) should be in another file like !defines.dm because order matters in preprocessing, so we want it to be in a file that is alphabetically before all of our other files.
#define TICK_LAG world.tick_lag //Replace world.tick_lag with whatever you set world.tick_lag to. If you use world.fps instead, this is 10/world.fps, but calculate it yourself and put it there. Assuming your tick_lag is not dynamic. This is here because you're going to probably be using this later down the line, and using a preprocessor definition is generally better than using world.tick_lag.

mob/proc
Attack(mob/Target)
//If you don't mind the check being infrequent to save CPU, you can set wait to 1 or higher.
//Since wait and complete should be constants, you could just place them right in or make preprocessor defines for them (like TICK_LAG)
//But for the sake of simplicity and since this seems to only be used for one function that won't be called super frequently, we'll make them local variables.
var wait = TICK_LAG //Check every frame (or however long you want)...
var complete = 30 //3 seconds to complete the first part and switch over to Target.Attack(src).
var timer = 0 //Timer variable that will help us determine when we're ready to attack. Could shorten it to just "t".
if(!Attacking && Target && !Target.NPC && (Target in oview(1,src))) //Move that if statement up here so that you can't attack while you're already attacking, but the attack loop goes on. Assuming the NPC variable is constant, so that we don't have to check it multiple times.
//We only want to do this once, so put this under the if statement instead of while loop.
var damage = (Strength + rand(0,3)) - (Target.Defence + rand(0,3))
if(damage < 0) //No need for checking if it is 0...
damage = 0
//No need for an else statement here. Try to keep it DRY whenever you can: Don't repeat yourself.
world << "[Target] was hit for [damage] damage!"
Target.Health -= damage
Attacking = 1
Target.Attacking = 1 //We're both in an attack state here. Also, I would just TRUE and FALSE for the Attacking variable, but I'm not going to change your design here.
do //So we won't check twice in a row, I use a do...while loop.
sleep(wait)
timer += wait
if(timer >= complete)
//Normally I would put timer -= complete here to restart the loop, but we're returning anyways in this case.
Attacking = 0
Target.Attacking = 0
Target.Attack(src) //Still using "recursion", eh? That could be considered a flaw. Though this is not as bad as the first time.
return //Return so that we don't hit the outside of the loop and so that we don't continue to do anything else.
while(Target && (Target in oview(1,src))) //This is an example of when we can repeat ourselves. What would be better so that you can easily change the range is to have a range variable, but we only use this in two places, made it clear that we want it to be oview(1, src), and we use recursion so I won't even bother. Note though, that this won't work if mobs have separate ranges, because of the recursion. This is where not using recursion would come in handy.
//Also, I think that using oviewers might be better because that only uses mobs. So, those are some things to look into.

//If we're out of range, we're both no longer attacking!
Attacking = 0
Target.Attacking = 0


There you go, don't forget to read the comments!