ID:2340181
 
(See the best response by IainPeregrine.)
Code:
/*
Target System V1 by Rocetti 2018
Creates a list of targets and put things on it by clicking,
but since its for RPG the list is just of 1 obj/mob
*/




client
var/list/target = new /list () // set a list called target
//targets are selected on click.. so click proc comes to the scene
Click (atom/T) // define a dummy variable called T of type atom which can be any of the Area, Turf, Obj or Mob
if (isobj(T) || ismob(T)) //checks if T is an obj or mob, cause in this game you interact with these two types
if (length (target) == 0) // if the target list have nothing inside then...
if (T in target) // if T is already in target list then..
return //do nothing and go mind your business
else // but if T is not in target list then...
target.Add (T) // add T to target list
else // but if the target list already have something inside then...
target.Remove (target[1]) // remove whatever is inside and then
if (T in target) //same thing as before
return
else
target.Add (T)
else // this line is to remove any target if you click in anything that is not Obj or Mob
if (length (target) > 0)
target.Remove (target[1])

..()

/*
calls a proc to check if the target is in view when you move
have a pixel movement issue but is not a big problem yet
when you move towards a tile where you will not see the target it
takes the target off before you completly can't see the target
is the way the pixel movement is implemented, but since there is no need
for moving to tile glide and this small margin of error will probably not annoy anyone
(there is a small chance ranged attackers will be pissed off
but lets say they need to master the art of range limits)
*/


Move ()
if (length (target) > 0)
targetInView (target[1], target)

..()


mob/player/Stat()
..()
if (length (client.target) > 0)
statpanel ("Target", client.target)


// creates a procedure to check if target is in view
proc
targetInView (var/atom/T, var/list/L)
for (T in L)
if (T in view())
return
else
L.Remove (T)


Problem description:
My first working "all by myself" code so i'm really proud and i just wanted to know: Is this right? anything that could improve performance?
If everything is ok ill post it in the tutorials and snippets.
thnx :)
Best response
Ahoy there! Congrats on getting your feet wet writing code :)
Also, congrats on actually commenting it!

My biggest criticism is that you're using a list for your target. In your first comment you even admit that the choice of a list doesn't make sense considering the game you made this for. I'm guessing you wanted to keep it a list because "someday" you might want to have multiple targets. That's great to be thinking ahead, but if you're going to pull out a system and put it somewhere for other people to use, it should work logically.

The second thing that strikes me are your comments aligned to the right of the code. Trust me, that's a loosing battle. Every time you edit a line or insert a new line you're going to have to rework the flow of that monster. Don't be afraid to put the comments right in there with the code.

Which leads to the third thing. Putting the comments in the code can look silly because you have the comment "// add T to target list" right next to the line "target.Add (T)" and it's like - yeah, we get it. Putting the comments in with the code can help us to understand what comments to write: comments that explain _why_ we're doing something, not just _what_ we're doing.

Also, give your variables descriptive names - not just of what they are, but what they do in the project. I get that a variable named "A" is an atom. That's not useful. Naming it "anAtom" isn't much better. Naming it "newTarget" tells me exactly what the variable represents. Your variable, proc, and object names are your first priority when it comes to commenting.

The only other issue I see is that you don't seem to understand 100% how arguments work in procs. Your targetInView() proc takes two arguments, an atom T and a list L. That means that we send the proc a specific T and a specific L. But the proc itself doesn't use the T that we send it. Instead, it uses the variable named T as a container to loop through all of L. In fact, the more I look at this proc, the more problems I see...

So this is what's actually happening in that proc. It takes a list L and loops through the atoms in it, starting at the first item in the list. If the item in the list that it's checking is not in view, it removes it from the list. If the item it's currently checking is in view, then it returns immediately and no further items are checked. Probably not what we want to do, but we'd never see that because currently this system only puts one item into the list.

I'm going to rewrite this in my style. That doesn't mean that what you made was wrong, but maybe you'll get some ideas to help you out :)

/*
Target System V1 by Rocetti 2018, edited by IainPeregrine
Allows the client to target one atom/movable by clicking it.
*/




client
// This will store the client's current target:
var/atom/movable/target

Click (atom/newTarget)
// Clear target if newTarget isn't a movable atom.
if (!istype(newTarget, /atom/movable))
target = null
return ..()
// Change current target to new target
target = newTarget
// Do the default clicking behavior
. = ..()

/*
calls a proc to check if the target is in view when you move
have a pixel movement issue but is not a big problem yet
when you move towards a tile where you will not see the target it
takes the target off before you completly can't see the target
is the way the pixel movement is implemented, but since there is no need
for moving to tile glide and this small margin of error will probably not annoy anyone
(there is a small chance ranged attackers will be pissed off
but lets say they need to master the art of range limits)
*/


Move ()
// Do the default behavior, which is to move the client's mob
. = ..()
// Remove target if it is no longer in view of the mob
if (target)
targetInView ()


mob/player/Stat()
..()
if (length (client.target) > 0)
statpanel ("Target", client.target)


// creates a procedure to check if target is in view
/* I'm making it a client proc, because the program needs to know
"in view of what?" By default, view() checks the view of the usr var,
But this variable is unsafe in procs (doesn't always mean
what you think it means). Instead, we'll tell the code
"in view of the client's mob", which is safe.
*/

client/proc
targetInView ()
// Remove current target if it's no longer in view
if(!(target in view(mob))
target = null


Now that we've seen a single target edit, let's go back to the list idea and see if we can have a list of targets.

/* Client Targeting
Extends the client so that players can click to target mobs and objs.
Targeted objects will remain targeted until the player moves out of view.
Also, the player can cancel all targeting by clicking a non-valid object.
*/


client
// This will store the client's current targets:
var/list/targets = list()

// The player can target movable atoms by clicking them:
Click (atom/newTarget)
// Clear all targets if newTarget isn't a movable atom.
if (!istype(newTarget, /atom/movable))
targets.Cut()
return ..()
// Cancel out if this client is already targeting newTarget.
if(newTarget in targets)
return ..()
// Add newTarget to the client's current targets
targets.Add(newTarget)
// Do the default clicking behavior
. = ..()

// When the player moves, the client will cease to target atoms out of range:
Move ()
// Do the default behavior, which is to move the client's mob
. = ..()
// Remove all targets that are outside the mob's view
var/list/currentView = view(mob)
for(var/atom/movable/targetCheck in targets)
if(!(targetCheck in currentView)
targets.Remove(targetCheck)

// Stats!
mob/player/Stat()
I really don't know how stats work, so this one is left for you! xD


I hope that's what you were looking for, and you've learned something useful from what I wrote :)
OMG <3
loved the reply :D
thnx for the edit! will review my code
(will give you credit for the help)

well i learned a lot with this!
I havent made a deep revision of the code and you opened my eyes to lots of things.

(this code is the result of my time away from this engine that i spent learning C# for Unity. So I just came back and didnt made a real review of the reference and guide hahaha)

Actually let me use this space here to share this tip:
If you are a non programmer and is trying to learn this art: learn C# for Unity! lots of materials and really good game dev logic. The DM Language is awesome but if you learn C# you will see how awesome it really is
(just sharing my experience its not a rule)
client
var/atom/movable/target

Click (atom/newTarget)
if (!istype(newTarget, /atom/movable))
target = null
return
target = newTarget

. = ..()

Move ()
..()
if (target)
if (target in view())
return
else
target = null
return


mob/player/Stat()
..()
if (client.target)
var/atom/T = client.target
statpanel ("Target")
stat ("Name: [T.name]")
stat ("Description: [T.desc]")


Just reviewed and saw that the Proc was actually disposable. Not sure if it have any performance difference but I think is more clean a code like this.
And putting the "..()" before checking if target is in view solve the problem i described originally.

Thnx man :D
In response to Rocetti
No need for this double check that does nothing otherwise. Also no need to write return if nothing happens outside the if statement.
if(target && !(target in view(src)))
target = null