Looping and Object Instancing Issues

Page: 1 2
ID:1854550
 
May 18 2015, 12:46 pm
(See the best response by FKI.)
Code:
        mob/AI/proc/AI_Equipment()
            for(var/obj/O in src.contents)
                if(O.Equipable && O.Equipped == 0)
                    var/Slot
                    if(O.Core == "Item" || O.Core == "Weapon")
                        if(O.Slot == 2 && src.Weapons["Left Back"] == null && src.Weapons["Right Back"] == null)
                            Slot = "Left Back"
                        else if(O.Slot == 1)
                            var/list/List = list("Left Waist", "Right Waist", "Left Back", "Right Back")
                            if(src.Weapons["Left Back"] != null)
                                List.Remove("Left Back")
                            if(src.Weapons["Right Back"] != null)
                                List.Remove("Right Back")
                            Slot = pick(List)
                    else
                        Slot = O.Type
                    O.Holder = src
                    O.Equip(Slot)
            for(var/x=1; x<=5; x++)
                var/obj/Armour/A = pick(typesof(/obj/Armour) - /obj/Armour)
                if(prob(50))
                    if(src.Armour[A:Type] == null)
                        A = new(src)
                        A.Equip(A:Type)


        obj/proc/Equip(var/Slot)
            if(src.Equipable && Slot != null)
                var/mob/M = src.Holder
                switch(src.Core)
                    if("Item")
                        if(src.Equipped == 1)
                            src.Equipped = 0
                            src.icon_state = ""
                            M.overlays.Remove(src)
                            M.Weapons[Slot] = null
                        else
                            if(M.Weapons[Slot] != null)
                                var/obj/O = M.Weapons[Slot]
                                O.Equip(Slot)
                                src.Equip(Slot)
                            else
                                src.Equipped = 1
                                src.icon_state = "[src.Type] - [Slot]"
                                M.overlays.Add(src)
                                M.Weapons[Slot] = src

                    if("Armour")
                        if(src.Type == "Head" && M.Clothing["Head"] != null)
                            M.client.sound_system.PlaySound('Notification Alert.ogg', null, 0)
                            M << output("<font color=red>Game System: </font>You cannot have Equipment in both Clothing and \
                                Armour for your Head Slot. It must be either one or the other.", "System Output")
                        else
                            if(src.Equipped == 1)
                                src.Equipped = 0
                                src.icon_state = ""
                                M.overlays.Remove(src)
                                M.Armour[Slot] = null
                            else
                                if(M.Armour[Slot] != null)
                                    var/obj/O = M.Armour[Slot]
                                    O.Equip(Slot)
                                    src.Equip(Slot)
                                else
                                    src.Equipped = 1
                                    src.icon_state = Slot
                                    M.overlays.Add(src)
                                    M.Armour[Slot] = src

                    if("Weapon")
                        if(src.Equipped > 0)
                            src.Equipped = 0
                            src.icon_state = ""
                            M.overlays.Remove(src)
                            if(src.Slot == 1)
                                M.Weapons[Slot] = null
                            else if(src.Slot == 2)
                                if(Slot == "Left Back")
                                    M.Weapons["Right Back"] = null
                                else if(Slot == "Right Back")
                                    M.Weapons["Left Back"] = null
                        else
                            if(M.Weapons[Slot] != null)
                                var/obj/O = M.Weapons[Slot]
                                O.Equip(Slot)
                                src.Equip(Slot)
                            else
                                src.Equipped = 1
                                if(src.Slot == 1)
                                    src.icon_state = Slot
                                    M.overlays.Add(src)
                                    M.Weapons[Slot] = src
                                else if(src.Slot == 2)
                                    if(findtext(Slot, "Back"))
                                        src.icon_state = "Full | Back"
                                        M.Weapons["Left Back"] = src
                                        M.Weapons["Right Back"] = src
                                        M.overlays.Add(src)
                                    else
                                        M.client.sound_system.PlaySound('Notification Alert.ogg', null, 0)
                                        M << output("<font color=red>Game System: </font>Two-Handed Weapons can only \
                                            be Equipped on your back", "System Output")

                    if("Clothing" || "Treasure")
                        if(src.Type == "Head" && M.Armour["Head"] != null)
                            M.client.sound_system.PlaySound('Notification Alert.ogg', null, 0)
                            M << output("<font color=red>Game System: </font>You cannot have Equipment in both Clothing and \
                                Armour for your Head Slot. It must be either one or the other.", "System Output")
                        else
                            if(src.Equipped == 1)
                                src.Equipped = 0
                                if(src.Core == "Clothing")
                                    src.icon_state = ""
                                else
                                    src.icon_state = src.name
                                M.overlays.Remove(src)
                                M.Clothing[Slot] = null
                            else
                                if(M.Clothing[Slot] != null)
                                    var/obj/O = M.Clothing[Slot]
                                    O.Equip(Slot)
                                    src.Equip(Slot)
                                else
                                    src.Equipped = 1
                                    if(src.Core == "Clothing")
                                        src.icon_state = Slot
                                    else
                                        src.icon_state = "[src.name] - [Slot]"
                                    M.overlays.Add(src)
                                    M.Armour[Slot] = src


Problem description:
I'm currently trying to create a function which will equip my AIs automatically without me having to go through each of them and add the choicest of armour and weapons for them. The "weapons" work easily because I had already went ahead and add a few of those to each of my AIs, but I seem to be having problems trying to create armour.

I have changed the armour-piece of the code numerous times in an attempt to see what's being done, and outputted the results to the screen. On a few occasions, I noticed that somehow, it was adding a null object of type armour to the contents, and failing to equip the armour (which would be a given, since the armour's Type variable would be null).

Weirder yet, I tried to output what the list of armour objects that would be referenced when using typesof in the for-loop and got a blank screen.

Any ideas what's going on here or what I'm doing wrong?
May 18 2015, 2:08 pm
One thing I noticed is var/obj/Armour/A = pick(typesof(/obj/Armour) - /obj/Armour) should be referencing an object. You are using it as if it is [an object], when in fact it is a type path of an armor object. That's probably why you are getting null objects.

So, the corrected version:

// the for() loop initializes here...
var/obj/Armour/A = new pick(typesof(/obj/Armour) - /obj/Armour)
if(prob(50))
    // old  version
    /*
    if(src.Armour[A:Type] == null)
        A = new(src)
        A.Equip(A:Type)
    */

    // corrected version
    if(src.Armour[A.Type] == null)
        A = new(src)
        A.Equip(A.Type)


If this is something you are doing a lot, I'd recommend using references to global object prototypes, so you aren't creating new armor objects each time that proc is called.
May 18 2015, 2:25 pm
That method is one of the ways I've tried before, and it still failed:

http://puu.sh/hSmWs/6b93692190.png

Basically, the "Armour List" there should have all object armour types, given that I took out the 50% probability if statement.

Anything else you could recommend?
May 18 2015, 2:54 pm
That method is one of the ways I've tried before, and it still failed

Is this in response to my revised snippet, or the second part of my post? I'm assuming the latter.

I also forgot to mention you could also do this:

var/A = pick(typesof(/obj/Armour) - /obj/Armour)
if(prob(50))
    if(src.Armour[A] == null)
        // alternative (and also correct) version
        A = new A(src)
        A:Equip(A)
        
        // OR, for clarity's sake..
        var/obj/Armour/a = new A(src)
        a.Equip(a)
May 18 2015, 3:26 pm
My response was to your revised snippet, not the piece about global object prototypes.

Also, while your two alternatives do work to some extent, they neglect a key point:

The if-statement 
 if(src.Armour[A.Type] == null)
is to prevent the code from creating an Armour Object that's not needed to be equipped to a slot that already has an Armour Object equipped there (for example, if the AI already has a Hauberk equipped to their Torso, and we randomly got a Doublet as the next piece of equipment to equip, then skip it, we've already got Torso protection).

So your current if-statement doesn't exactly, "fit the bill", but I've been "tinkering" with it:

mob/AI/proc/AI_Equipment()
            for(var/x=1; x<=5; x++)
                var/O = pick(typesof(/obj/Armour) - /obj/Armour)
                if(prob(65) && !src.contents.Find(O))
                    new O (src)
            for(var/obj/O in src.contents)
                if(O.Equipable && O.Equipped == 0)
                    var/Slot
                    if(O.Core == "Item" || O.Core == "Weapon")
                        if(O.Slot == 2 && src.Weapons["Left Back"] == null && src.Weapons["Right Back"] == null)
                            Slot = "Left Back"
                        else if(O.Slot == 1)
                            var/list/List = list("Left Waist", "Right Waist", "Left Back", "Right Back")
                            if(src.Weapons["Left Back"] != null)
                                List.Remove("Left Back")
                            if(src.Weapons["Right Back"] != null)
                                List.Remove("Right Back")
                            Slot = pick(List)
                    else
                        Slot = O.Type
                    O.Holder = src
                    O.Equip(Slot)


This works, but I'm not 100% sure that it will avoid making duplicates.
May 18 2015, 4:01 pm
Best response
I made a mistake in my second snippet; I didn't realize you had a Type variable, and instead registered it as the built-in type in my mind.

My first snippet is closer to what you want (noticed an error there as well, but it still pointed out the issue). Going back to the original problem though, I want to try and elaborate on the problem I see.

for(var/x=1; x<=5; x++)
    // variable A is the type path of a /obj/Armour object
    // e.g. /obj/Armour/Head_Armour
    var/obj/Armour/A = pick(typesof(/obj/Armour) - /obj/Armour)

    if(prob(50))
        // Now here, you're assuming A is an Armour object, but it's not.
        // It's a type path as stated above, so you want to create an object of type A
        // so we can access the Type variable correctly.
        var/obj/Armour/a = new A
        if(src.Armour[a.Type] == null)
            // Move the temp object to the AI's contents and make it official,
            // otherwise it's sent to the garbage collector after this proc returns.
            a.Move(src)
            a.Equip(a.Type)


Here is what you had originally:

for(var/x=1; x<=5; x++)
    var/obj/Armour/A = pick(typesof(/obj/Armour) - /obj/Armour)
    if(prob(50))
        // Incorrect check here, because you are dealing with a type path,
        // not an object.
        if(src.Armour[A:Type] == null)
            A = new(src)
            A.Equip(A:Type)


With your original setup, that if() statement checking if the armor is already equipped, incorrectly returned true, creating that funky null object you mentioned.

Even though your new setup works, it's wrong. You are checking for a type path in src's contents, when I'm pretty sure you want to be checking for an object instead. You get a false truth as a result.
May 18 2015, 4:14 pm
Oh yeah, that definitely did the trick.

Cheers, mate!
May 19 2015, 7:13 am
You're probably going to want to bulletproof that equipment system at some point. You should never have an "equipped" var on your equipment, because it should be the wearer's job to keep track of whether it's equipped. This prevents a lot of possible weird scenarios: like for instance if you ever added something that could steal items (even equipped ones) from a player, you wouldn't want the equipment to think it's still in use.

A couple of other things about the current code: I think you'd rather call prob(50) first before picking the armor type; otherwise the pick() is done for no reason. Also I would recommend switching to a system that lets a var choose likely armor types, as you probably don't want every random enemy to wear any random armor--most games would theme this. So you might want to think of ways to make the enemies extensible.
May 19 2015, 7:19 am
The equipment system is already "bulletproof'd" since when any equipment is disarmed (or the like), the equip function is called immediately, and then taken from the contents of the user (if needed, anyway).

And actually, yes, I do want every random enemy to wear any random armour. I did make your correction with the probability and pick statements though.
May 19 2015, 7:29 am
Having an equipped var and remembering to change it as needed isn't bulletproof--it merely works for now. Bulletproof is designing it so you don't have to remember. The equipped var is redundant because the wearer should always know what they're wearing, so you have two ways of tracking the same piece of info and that presents the possibility--however well-guarded against--of the two getting out of sync.

I should also point out that when you do use vars that can only be true or false, use !myvar instead of myvar==0 and myvar instead of myvar==1 in your if/for/while statements. If the var was ever anything other than 0 or 1, it would mess up any logic that expects that.
May 19 2015, 7:33 am
That is the reason why the variables and their if-statements are done precisely like that.

My equipment system uses the "equipped" variable to check for two "types" of equipped: -- 1 being that it's on the person, and 2 being that it is armed in the Player's hand, which is precisely why the if-statements are done like that (0 being that it's not even equipped). Furthermore, it is for that very same reason why it's not redundant, you're just not seeing "the full picture" so you interpret it as such.
May 19 2015, 8:22 am
Hmm. Do you have types of equipment that you can use either way? Because otherwise you still don't need the equipped var to have multiple values. The only purpose would be if you could take off your hat and bash people with it, although that'd just as easily still fit in a slot-based system and again the equipped var would be redundant.
May 19 2015, 8:30 am
The "equip" variable is used for all object types. Typically, if the value for it is 2, it's to imply that the object is a Weapon which is equipped, is also "armed" / "in the Player's hand".

In the future, I intend to make more equipment which will continue to be like this, which will include, Armour, Treasure, and Clothing that can actually be "armed".

And while I still do use a slot-based system as well for equipment, having the "equip" variable also serves for some miscellaneous things, such as ensuring Players can not trade objects they currently have equipped so long as the "equip" variable is true (1 or 2). Sure enough, I could also use "src.Weapons.Find(O)" to get the same thing, but I imagined using a single comparison statement (if(O.Equipped) return) would be easier than checking all the different equipment slots / lists (Armour, Weapons, and Clothing).
May 19 2015, 9:18 am
Consider this: Replace the equipped var with a proc.

For example, in a slot-based system, say each mob has a list called equipment (which isn't initialized till it's needed). It's an associative list, so equipment["weapon"] would be the current weapon. And then say each equippable object has a var called slot.

obj/item/proc/Equipped()
    var/mob/M = loc
    return istype(M) && M.equipment && M.equipment[slot]==src

The benefit is that you now keep track of what's equipped in only one place: the wearer's equipment list. There's no possibility whatsoever of that ever coming out of sync.
May 19 2015, 10:28 am
So basically, you're telling me to change my numerous lists of Weapon, Armour, and Clothing, to just one, being Equipment?

Seems like a hassle in my opinion because the Equipment is done in such a way that both Armour and Clothing are equipped to similar areas (Head, Torso, Hand, etc), but do not overwrite each other (with the exception of the Head area). Now, to do this would mean having to make associative parts such as "Equipment["Torso-Clothe"]", "Equipment["Torso-Armour"]", and so on to get the same effect.

As it stands right now anyway, the only time the "equip" variable is changed is within the obj/proc/Equip() (which is what uses the slot-based system that I already told you about), and with the obj/Weapon/proc/Arm(var/Arm) (which is what arms the Weapon to a Player's Hands for use, and also uses a similar slot-based system with the mob's Hands-list).

That being said, should I ever decide to use a "Disarm" Skill (which I already have), it would just need to first call the Arm() for the correct arm that Weapon is in and then the Equip() for that same Weapon (which it already does).

I still do not see in anyway how this will "go out of sync" because of the way in which I use them. Naturally, I try to keep everything Object-Oriented-esqued (that is, certain variables for objects are only ever changed within certain functions) to avoid such a thing.

All of that being said, I'm still in favour of using my own method as like I said, the equipped variable also provides for some miscellaneous things, as I'm only reading the value and not changing it.

Still, thanks for the suggestion.
May 19 2015, 10:49 am
In response to Yoren
Yoren wrote:
So basically, you're telling me to change my numerous lists of Weapon, Armour, and Clothing, to just one, being Equipment?

Telling no; it is your game after all. But suggesting, very much. Using a single comprehensive system to track equipment is really the most bulletproof option, and it prevents a lot of duplicate code. When code has to be duplicated in several places it gives rise to all sorts of potential errors. Doing it all in one set of procs is a great deal cleaner.

The downside is, refactoring can be quite a pain. But I've found it to be a rewarding pain. Changing an unwieldy design to something simpler usually reaps enormous benefits.

Seems like a hassle in my opinion because the Equipment is done in such a way that both Armour and Clothing are equipped to similar areas (Head, Torso, Hand, etc), but do not overwrite each other (with the exception of the Head area). Now, to do this would mean having to make associative parts such as "Equipment["Torso-Clothe"]", "Equipment["Torso-Armour"]", and so on to get the same effect.

That's not really a problem, though. Heck, you could also have two slot vars that work together. The point is ultimately it'd all be kept in one place. Fewer lists to manage, fewer lines of code, much less code to maintain.

I still do not see in anyway how this will "go out of sync" because of the way in which I use them. Naturally, I try to keep everything Object-Oriented-esqued (that is, certain variables for objects are only ever changed within certain functions) to avoid such a thing.

The gist is, you have two vars pointing to the same information. The mob says "I have X equipped", and X says "I am equipped". Your current code may not allow them to get out of sync, but it's a situation easily avoided entirely by a minor change. That way you have peace of mind knowing that this will never be an issue.
May 19 2015, 11:21 am
Lummox JR wrote:
Telling no; it is your game after all. But suggesting, very much.

Semantics are minuscule to me. It is not as if I'm taking offense. So if you took it as such, I'm afraid you've misread / misinterpreted.

Using a single comprehensive system to track equipment is really the most bulletproof option, and it prevents a lot of duplicate code. When code has to be duplicated in several places it gives rise to all sorts of potential errors. Doing it all in one set of procs is a great deal cleaner.

The system in play is a simple system to keep track of equipment, I've no difficulty with understanding it thus far anyway (and I'm the only Programmer). If there is only 1 way in which equipment can be equipped, why then, how could you get even more bulletproof than that? Sure enough, I could place the Arm() within the Equip() as well too, but I just haven't. No particular reason why either, to be honest.

I've no cause to duplicate the Equip() function either, and when it comes time to create Armour, Clothing, and Treasure that can be armed, it'd be a matter of changing obj/Weapon/proc/Arm(var/Arm) to /obj/proc/Arm(var/Arm) and be the end of that (or again, just attach it to Equip()).

Changing an unwieldy design to something simpler usually reaps enormous benefits.

I think this is simply a matter of coding design preferences as oppose to "unwieldy design".

That's not really a problem, though. Heck, you could also have two slot vars that work together. The point is ultimately it'd all be kept in one place. Fewer lists to manage, fewer lines of code, much less code to maintain.

As a matter of fact, that is a problem. It's as if I'm categorizing everything in a fashion that could be improved upon, in the same way in which I refer to Equipment to encompass Weapons, Armour, and Clothing in meaning. Doing that to me is what looks "unwieldy in design", which is why again, this seems like a thing of code design preference.

I also do not mind having the extra lists as again, it looks tidier to me than throwing everything into 1 list. Also, from what I remember considering Algorithm Analysis and Design, searching through 1 huge list would be more "taxing" than searching through 1 short list that you know holds what you want (after all, there's no reason to search through Armours if you're equipping a Weapon, right?). Again -- Code Design Preferences.

The gist is, you have two vars pointing to the same information. The mob says "I have X equipped", and X says "I am equipped". Your current code may not allow them to get out of sync, but it's a situation easily avoided entirely by a minor change. That way you have peace of mind knowing that this will never be an issue.

Such a thing carries integrity/security all by itself. If X says, "I am equipped" but the mob says, "I do not have X equipped", then it'd be a simple thing of an if statement to check that / keep the mob (or X) from proceeding any further (and I don't even have need to do that in the first place, since that everything concerning Equipment only carries on if it is Equipped correctly first, which can only be accomplished through Equip() / Arm(var/Arm)).

And if my code will not allow them to get out of sync, I do believe then it is still easily avoided as I've only made the changes within the 2 functions -- Thus, I do believe I already have peace of mind knowing that it won't be an issue, mate.

May 19 2015, 11:33 am
Oh, no offense taken (or meant, either). I'm just speaking from my own experience that using fewer vars to say the same thing is better, and fewer lists to track what basically amounts to the same concept is better too. In the end it's all your decision, but I will say it's surprising how these kinds of things can bite you later on.
May 23 2015, 11:29 pm
Bump.

I've developed some new problems, though, of the "same cloth" as the problem mentioned earlier.

        mob/verb/Check_Types()
            usr << "======================================"
            usr << "Produce"
            for(var/obj/Produce/O in typesof(/obj/Produce) - /obj/Produce)
                var/obj/Produce/P = new O
                usr << "Produce: [P.type]"
            usr << "======================================"
            usr << "Materials"
            for(var/obj/Material/O in typesof(/obj/Material) - /obj/Material)
                var/obj/Material/M = new O
                usr << "Material: [M]"
            usr << "======================================"
            usr << "Vegetation"
            for(var/obj/Vegetation/O in typesof(/obj/Vegetation) - /obj/Vegetation)
                var/obj/Vegetation/V = new O
                usr << "Vegetation: [V]"
            usr << "======================================"


Note, that this code was ultimately just done to test what was really happening here. Basically however, I'm trying list all of the respective objects. In the long run, my goal is to have other objects use these objects within them (like a Tree Object having Vegetation Objects within its contents).

However, the output of the test run was similar to my first problem:

http://puu.sh/hYlYa/3f86375dcf.png

-- Nothing. I even tried tweaking it to "new O (usr)" and then display the contents of the usr, but again -- Nothing. Yet, strangely enough, if I tweak the code like so (make note of the Vegetation Object):

mob/verb/Check_Types()
            usr << "======================================"
            usr << "Produce"
            for(var/obj/Produce/O in typesof(/obj/Produce) - /obj/Produce)
                var/obj/Produce/P = new O
                usr << "Produce: [P]"
            usr << "======================================"
            usr << "Materials"
            for(var/obj/Material/O in typesof(/obj/Material) - /obj/Material)
                var/obj/Material/M = new O
                usr << "Material: [M]"
            usr << "======================================"
            usr << "Vegetation"
            var/obj/Vegetation/O = pick(typesof(/obj/Vegetation) - /obj/Vegetation)
            var/obj/Vegetation/V = new O
            usr << "Vegetation: [V]"
            usr << "======================================"


Which is basically how I was told to deal with the first problem I had in this post -- It works without a hitch:

http://puu.sh/hYmc6/2558601d8f.png

Is there something here I am misunderstanding about the typesof() or object instancing? 'cause at the least, there's seem to be no real difference in what I'm doing between the two code snippets, only the method in how the type-path of an object is acquired.
May 23 2015, 11:31 pm
The problem is that O is defined as an /obj/Material object, but O will only ever hold a type path--not an object. The for() loop is trying to find only objects of type /obj/Material (and so on) in a list of type paths--no objects--and therefore not finding any.

If you just use var/O the problem should go away.
Page: 1 2