(RESOLVED)Problem with stepup() and stepback() proc.

Page: 1 2 3 4 5
ID:2013290
 
Jan 6 2016, 6:58 am (Edited on Jan 9 2016, 12:41 pm)
(See the best response by Ter13.)
Code:
    stepback(K)
        fast=1
        loop
            if(!src||paralysed||imitated||npc||imitation)return
            if(K>0)
                K--
                var/image=new/obj/kimage(loc)
                image:dir=dir
                image:icon=icon
                image:overlays=overlays
                image:invisibility=invisibility
                moving=0
                var/P=dir
                if(dir==NORTH)step(src,SOUTH)
                if(dir==SOUTH)step(src,NORTH)
                if(dir==EAST)step(src,WEST)
                if(dir==WEST)step(src,EAST)
                dir=P
                runstep=0
                spawn(1)goto loop
        fast=0
    stepup(K)
        fast=1
        loop
            if(!src||paralysed||imitated||npc||imitation)return
            if(K>0)
                K--
                if(!istype(src,/mob/Animal))
                    var/image=new/obj/kimage(loc)
                    image:dir=dir
                    image:icon=icon
                    image:overlays=overlays
                moving=0
                step(src,dir)
                spawn(1)goto loop
        fast=0


Problem description:

This code is from my old project and was working properly last year but recently i've been restarting my project and it seems that it doesn't work anymore because in-game when one of those proc is called the player doesn't even move. So I would like to know if DM changed in some way that these code need to be fix and how to fix it? It may be an easy threat but im still a beginner coder so if you could bring explanation to Why/How/What/etc when fixing it it would really help me!

Thank you guys,
Louis53

Jan 6 2016, 7:38 am
Edited:it seems the codes worked again 0_0 idk why!
Jan 6 2016, 7:45 am
Best response
There are a lot of problems with the code you linked. I'm amazed it doesn't crash your project. You should basically never use goto.

mob/proc
    stepback(Dist)
        set waitfor = 0
        fast=1 //this is probably unsafe
        var/obj/kimage
        var/turf/t
        while(Dist--)
            if(!src||paralysed||imitated||npc||imitation)return //this is probably wrong
            if(after_image)
                kimage=new/obj/kimage(loc,src)
            moving=0 //this is probably unsafe
            Step(turn(dir,180),dir)
            runstep=0 //this is probably unsafe
            sleep(1)
        fast=0

    stepup(Dist)
        set waitfor = 0
        fast=1 //this is probably unsafe
        var/obj/kimage
        var/turf/t
        while(Dist--)
            if(!src||paralysed||imitated||npc||imitation)return //this is probably wrong
            if(after_image) //use a variable, don't test types POLYMORPHISM BRO
                kimage=new/obj/kimage(loc,src)
            moving=0 //this is probably unsafe
            Step()
            runstep=0 //this is probably unsafe
            sleep(1)
        fast=0

    /*//if you are using pixel movement, use the following code:
    Step(MoveDir,FaceDir=0,Dist=0)
        if(!MoveDir) MoveDir = dir
        if(!Dist) Dist = step_size
        var/nx = (x-1)*TILE_WIDTH+step_x+bound_x
        var/ny = (y-1)*TILE_HEIGHT+step_y+bound_y
        if(MoveDir&NORTH) ny += Dist
        else if(MoveDir&SOUTH) ny -= Dist
        if(MoveDir&EAST) nx += Dist
        else if(MoveDir&WEST) nx -= Dist
        var/turf/t = locate(nx/TILE_WIDTH+1,ny/TILE_HEIGHT+1,z)
        if(t) return Move(t,FaceDir,nx%TILE_WIDTH,ny%TILE_HEIGHT)*/

    //if you are using tile movement, use the following code:
    Step(MoveDir=0,FaceDir=0)
        var/turf/t = get_step(src,MoveDir||dir)
        if(t) return Move(t,FaceDir)


As for your kimage object, a new feature has been released that will make doing afterimages much better.

obj/kimage
    New(loc,atom/movable/source)
        if(source)
            appearance = source.appearance
            alpha = 128
Jan 6 2016, 8:08 am
A GREAT THANK YOU TO YOU! :D I'm a beginner coder and this code isn't from me so i can code but i'm not good enough to recognize clean code from buggy code. So my code could be buggy as hell and I wouldn't necessarly know.
Jan 6 2016, 8:10 am
but just for you to know the code isn't for a player to move it is for a player to dash in or to be bump back depending of the situation(Idk if it changes something?)
Jan 6 2016, 8:17 am
and my kimage code is like this actually:

    kimage
        name="Afterimage"
        icon_state="image"
        New()spawn(2)del src
Jan 6 2016, 1:09 pm
The use of goto for a simple loop is always a big clue that something's wrong. The goto statement is really only meant for complex situations that can't easily (or sometimes, speedily) be solved another way.

The : operator is another one. It's really best to avoid using it, because the . operator can force you to do better type checking. In the original code, : was used in cases where . would have worked just fine.
Jan 6 2016, 4:22 pm
so since the code worked what should I do to make it load faster and to be cleaner?
Jan 7 2016, 11:58 am
Up
Jan 7 2016, 12:23 pm
There's no way to answer your question without seeing how the code looks now after you've made use of Ter's advice.
Jan 7 2016, 1:18 pm
the code is the same as it was
Jan 7 2016, 1:21 pm
 
    stepback(K)
        if(istype(src,/mob/Animal/Lost_Cat))return
        fast=1
        loop
            if(!src||paralysed||imitated||npc||imitation)return
            if(K>0)
                K--
                var/image=new/obj/kimage(loc)
                image:dir=dir
                image:icon=icon
                image:overlays=overlays
                image:invisibility=invisibility
                moving=0
                var/P=dir
                if(dir==NORTH)step(src,SOUTH)
                if(dir==SOUTH)step(src,NORTH)
                if(dir==EAST)step(src,WEST)
                if(dir==WEST)step(src,EAST)
                dir=P
                runstep=0
                spawn(1)goto loop
        fast=0
    stepup(K)
        if(istype(src,/mob/Animal/Lost_Cat))return
        fast=1
        loop
            if(!src||paralysed||imitated||npc||imitation)return
            if(K>0)
                K--
                if(!istype(src,/mob/Animal))
                    var/image=new/obj/kimage(loc)
                    image:dir=dir
                    image:icon=icon
                    image:overlays=overlays
                moving=0
                step(src,dir)
                spawn(1)goto loop
        fast=0
Jan 7 2016, 1:22 pm
and I just realized in the code I use there is lopp everywhere because it is not me code should I try to clean these?
Jan 7 2016, 1:25 pm
Louis53 wrote:
the code is the same as it was

The advice is the same as it was.
Jan 7 2016, 1:28 pm
Yeah, I can't really offer you advice on improving the code until you've made the first changes. The original code is broken in too many ways. Ter pointed you in the right direction. Use that as a starting point.
Jan 7 2016, 1:31 pm
Ok but like you said "fast" and other things is unsafe but what should I do?
Jan 7 2016, 1:33 pm
and just for notice step up and step back is not for the player to walk it is to fast dash forward(for spells or other) and step back is too be bump back when you get hit by some spells and for now it work just well but I have no idea how to make it cleaner.
Jan 7 2016, 1:35 pm
I suppose I could be more in-depth about this kind of thing.

1) Don't use goto at all. Use a proper loop.

2) fast=1/fast=0 is a very, very brittle implementation. fast++/fast-- would probably be better. If anything else triggers that uses the fast variable and resets it to 0 during the duration of this function, it will break whatever fast does.

3) you are returning before fast is reset to 0 without resetting fast to 0. Which means that yes, it's unsafe and whatever fast does is broken.

4) You shouldn't be using istype() to check if src is a certain type. That's a really bad approach because src should not have behavior that doesn't apply to it. You need to use polymorphic concepts to your advantage and override methods properly.

5) If !src should never happen. If src ceases existing, the function will stop unless src has been manually set to null in order to prevent the function stopping when the object it was called from is deleted. Basically, again, it will never be true, therefore it's a wasted cycle.

Also, there's this little turd here:
if(dir==NORTH)step(src,SOUTH)
if(dir==SOUTH)step(src,NORTH)
if(dir==EAST)step(src,WEST)
if(dir==WEST)step(src,EAST)


follow this through logically. When a mob moves south using step, its direction is changed to south. Thus, if we enter this line of code, and our direction is north, we'll move south. Then, our direction will be south. Now because our direction is south, we'll step north, and our direction will be north. east and west if statements will fail. The same thing happens when you are facing east. This function won't even work properly for north and east facing movables because you need to be using a switch statement, or if-else pairs, or something other than four unconnected if-statements in a row.

TL;DR: You are only hurting yourself by using stolen/leaked source code. Every single line of these two functions is probably broken in some way.
Jan 7 2016, 1:36 pm (Edited on Jan 7 2016, 1:42 pm)
and my code for ki image is
 kimage
        name="Afterimage"
        icon_state="image"
        New()spawn(2)del src

//Should I use:

  obj/kimage
    New(loc,atom/movable/source)
        if(source)
            appearance = source.appearance
            alpha = 128
Jan 7 2016, 1:40 pm
It is propably because it is an old code and I don't think it was stolen or anything but I still share your opinion and I understand what you're saying(Don't take wrong sources) but I don't understand why the code is actually working well?
Page: 1 2 3 4 5