Redux V2 Auto Arrow Equip

[youssef911]

Hello all, I have been here in the community passively just reading posts and learning from other people who share their knowledge. It has been nearly a month now since I started to learn to code in C# through making a private server (Redux). I already know other languages Python, PHP and I know how to work with databases (MySQL). I have been lurking the Redux forum and it really has MASSIVE information shared which could be really helpful to beginners like myself.


what is the purpose?


After many trials (not saying that its hard, I am a noob as I said), I managed to make a working Arrow Auto Equip system although it is not near to perfect but it does what we need it to do in that case removing finished pack of arrows, correctly replacing the new one and deleting the old one.


I will post the code below but before I do that I need to thank so many people who shared their valuable knowledge for the community to learn from. I do not take any credit for this at all. In fact, I learned a lot and would like to specially thank Spirited and Pro4Never even though he is banned, and Many other people who shared Mina, Turk55. so credit and thanks goes to all people who shared to the community.


Also I am open for corrections to the code. it would be really helpful, consider me your student. but what is not helpful is any negative attitudes.


what I made is in players.cs I made a definition

 Spoiler

#region auto equip arrows
        public void ArrowAutoEq(int _id, byte arloc)
        {
            var found = 0;
            foreach (var i in Inventory.Values)
            {
                if (i.StaticID == _id)
                {
                    found++;
                    if (found == 1)
                    {
                        
                        Equipment.EquipItem(i, arloc, true);
                        break;
                    }
                }
            }

        }
        #endregion

Then in Combat Manager (this part after the bool hasitem= was first made as a not working outline by ETERU).

so then replaced the "//Take arrow count code region" with this.. now we are using the auto equip arrow and reassigning different id if arrows are different..

 Spoiler

//Take arrow count
            if (_magicType.WeaponSubtype == 500)
            {
                if (_magicType.UseItem == 50 && _magicType.UseItemNum > 0)
                {
                    ConquerItem item;

                    owner.Equipment.TryGetItemBySlot(5, out item);
                    if (item == null)
                        return false;

                     int _id = 0;
                     byte arloc = 5;

                     bool hasitem = (owner as Player).HasItem(1050000);

                    if (item.StaticID == 1050003 && item.Durability <= 3)
                    {
                        if (hasitem == true)
                        {
                            _id = 1050003;

                        }
                    }
                    else if (item.StaticID == 1050002 && item.Durability <= 3)
                    {
                        if (hasitem == true)
                        {
                            _id = 1050002;
                        }
                    }
                    else if (item.StaticID == 1050001 && item.Durability <= 3)
                    {
                        if (hasitem == true)
                        {
                            _id = 1050001;
                        }

                    }

                    while (item.StaticID == 1050000 && item.Durability <= 3)
                      {

                        if (hasitem == true)
                        {
                            _id = 1050000;
                        }

                         (owner as Player).ArrowAutoEq(_id, arloc);


                    }
             
                   
                    item.Durability -= _magicType.UseItemNum;
                    if (item.Durability == 0)
                    {
                        owner.Equipment.UnequipItem(5);
                        (owner as Player).DeleteItem(item);
                       
                        (owner as Player).Inventory.TryGetValue(item.UniqueID, out item);
                        (owner as Player).Inventory.TryRemove(item.UniqueID, out item);
                    }
                    owner.Send(ItemInformationPacket.Create(item, ItemInfoAction.Update));

                    
                    
                }
            }

As you can see I am checking if the dura(count of arrows) is <=3 not checking if its 0, that is because this function in combat manager is only accessible when doing a skill not when using only one arrow maybe there is another function for using one arrow so i can use the same method of reload in it. but i am using <=3 because if it goes to 1 then it doesn't reload at all. for 2 reasons
1- the scatter doesn't work when having only 1 arrow
2- as i said if no scatter then your option is to hit only one arrow and then the method isn't accessed so you are left with 0 arrows.

i got around it by adding the <=3 so when it reduces to 3 it reloads and remove the possibility that it reaches 1. idk if i messed up in this part but i guess i did.

Thanks ElitePVPers for giving helping us learn coding!!

Thanks Spirited, Pro4Never..

Thanks all..

If I am on the right track I hope I will contribute more in the future.

[daxik]

Kinda at this moment i could say - _magicType.UseItemNum is used to define actual "needed" amount of arrows to perform action so use it to compare nessesity of replacing arrows (gunna get stuck on fixed scatter and 20 lvl arrows someday the way it is however sounds very low probability but real). Btw i dont remember id's of arrows but check if youre trying to equip highest lvl arrows at the end so you always end up choosing 73 lvl ones rather than 1 lvl ones instead.

[youssef911]

Thanks for the reply @ i just tried other arrows and they reload to lv 1 arrow like you said so i will try and fix that and post an update.

Okay so i figured out how to make it reload the specific type of arrow i am using it was pretty simple actually and i was just elseif -ing the hell over it so i modified it to this

 Spoiler

//Take arrow count
            if (_magicType.WeaponSubtype == 500)
            {
                if (_magicType.UseItem == 50 && _magicType.UseItemNum > 0)
                {
                    ConquerItem item;

                    owner.Equipment.TryGetItemBySlot(5, out item);
                    uint id = item.StaticID;
                    if (item == null)
                        return false;
 
                   
                    item.Durability -= _magicType.UseItemNum;
                    owner.Send(ItemInformationPacket.Create(item, ItemInfoAction.Update));

                    if (item.Durability <= 2)
                    {

                        owner.Equipment.UnequipItem(5);
                        (owner as Player).DeleteItem(item);
                        AutoEquip(item, id);

                    }
                    
                }
            }

The only problem now that still persists is that this only works if is used with scatter, and scatter cannot be used when the amount of arrows left is 1 so we use normal attack therefore we are left with 0 arrows no reload because we used basic attack if i sort it out i will modify this update

I don't mean to be nagging on the issue but at last I have completed my code that i wanted to share. still its not perfect because when we reach 2-1 arrows scatter doesn't hit anymore so we will have to basic attack. i have added the same method i put to skill also to auto equip with bow's basic attack. so when 2-1 arrows are left we can just basic attack them and it will reload automatically. if anyone knows how to remove the lock on 2-1 arrows not to do skill do tell.


Also here is the final code

this method goes to Players.cs (or at least that's where i put it).

 Spoiler

#region auto equip arrows
        public void ArrowAutoEq(uint _id, byte arloc)
        {
            var found = 0;
            foreach (var i in Inventory.Values)
            {
                if (i.StaticID == _id)
                {
                    found++;
                    if (found == 1)
                    {
                        
                        Equipment.EquipItem(i, arloc, true);
                        break;
                    }
                }
            }

        }
        #endregion

Then i added this method in combat manager basically just checking if the player has the item or not to avoid repeating my self in both skill and phys

 Spoiler

#region autoequipcheck
        private void AutoEquip(ConquerItem item, uint id)
        {
            uint _id = id;
            byte arloc = 5;

            bool hasitem = (owner as Player).HasItem(_id);
            if (hasitem == true)
            {
                (owner as Player).ArrowAutoEq(_id, arloc);
            }
            else
                (owner as Player).SendMessage("There is not enough arrows to equip!", ChatType.Task, ChatColour.Red);

 owner.Equipment.UnequipItem(5);
                (owner as Player).DeleteItem(item);
        }
        #endregion

Then below this line "owner.Equipment.TryGetItemBySlot(5, out item);"

in "//Deal damage to target" in each section attack and skill
we need to set the variable id to get our current arrow's static id:

 Spoiler

uint id = item.StaticID;

then this goes after unequipping and deleting the item in both attack and skill section meaning under this line in each part "(owner as Player).DeleteItem(item);" i added this :

 Spoiler

AutoEquip(item, id);

[Super Aids]

What the **** is this?

private void AutoEquip(ConquerItem item, uint id)
        {
            uint _id = id; // WTF ????????????????

[Spirited]

Just some feedback with your arrow count, it looks like if the item returned is null, your method will crash with a null exception here (below). I don't know when that could be null, but it's good practice to check before using any accessor properties on the object.

owner.Equipment.TryGetItemBySlot(5, out item);
uint id = item.StaticID; <-- null exception
if (item == null)
    return false;

Also, type casting using an as operation comes at a performance cost. It's not much, but it does help with readability as well if you only cast once. It also helps to align text properly. For example with the AutoEquip method, it looks like you have multiple lines in your else statement, but since you have no brackets, it actually evaluates as such:

#region autoequipcheck
private void AutoEquip(ConquerItem item, uint id)
{
    var player = owner as Player;
    if (player.HasItem(id) == true)
        player.ArrowAutoEq(_id, 5);
    else
        player.SendMessage("There is not enough arrows to equip!", ChatType.Task, ChatColour.Red);
		
    owner.Equipment.UnequipItem(5);
    player.DeleteItem(item);
}
#endregion

I'm not sure if that was intentional or not, but I thought I'd check. The last bit of feedback I have is minor, but I don't think you need an integer "found" variable unless you plan on changing it to a bool and returning that bool if the ArrowAutoEq method doesn't find arrows. For example:

#region auto equip arrows
public bool TryArrowAutoEq(uint _id, byte arloc)
{
    bool found = false;
    foreach (var i in Inventory.Values)
    {
        if (i.StaticID == _id)
        {
            Equipment.EquipItem(i, arloc, true);
			found = true;
            break;
        }
    }
	
	return found;
}
#endregion

Here are some resources on code style as well, in case you're interested.


Cheers.

[youssef911]

So, I did as advised and changed the code in a way that meets with all of the issues pointed out. so first I removed the _id variable (so silly of me rly).
then I modified the arrowautoeq() method to be like this:

 Spoiler

 #region auto equip arrows
        public void ArrowAutoEq(uint _id, byte arloc)
        {
            bool found = false;
            foreach (var i in Inventory.Values)
            {
                if (i.StaticID == _id)
                {
                    found = true;
                    if (found)
                    {
                        
                        Equipment.EquipItem(i, arloc, true);
                        break;
                    }
                }
            }

        }
        #endregion

however I did leave it as void because I checked if the arrow was equipped later as I will show you.
I modified alot in the AutoEquip() method.
1- declared the owner as Player on top.
2- added checks to see if the player has other types of arrows to equip
3- check if the arrow has been equipped

here is the method :

 Spoiler

 #region autoequipcheck
        private void AutoEquip(ConquerItem item, uint id)
        {
            
            bool hasit = owner.Equipment.TryGetItemBySlot(5, out item);
            var _owner = owner as Player;
            if (hasit == true && item.Durability <= 2)
            {
                
                //owner.Equipment.UnequipItem(5);
                _owner.DeleteItem(item);
            }
            
            byte arloc = 5;
           
            bool hasitem = _owner.HasItem(id);
            bool hasitem2 = false;

            if (hasitem == true)
            {

                _owner.ArrowAutoEq(id, arloc);
 
            }
            else
            {

                if (!hasitem)
                {
                    id = 1050000 - 1;
                    for (int i = 0; i < 3; i++)
                    {
                        id++;
                        hasitem2 = _owner.HasItem(id);
                        
                        if (hasitem2 == true)
                        {

                            _owner.ArrowAutoEq(id, arloc);

                            break;

                        }
                    }
                    bool isequip = owner.Equipment.TryGetItemBySlot(5, out item);

                    if (isequip == true)
                    {
                        _owner.SendMessage("Arrows Reloaded to nearest one i can find!", ChatType.Task, ChatColour.Red);
                       
                    }
                    else
                        {
                        _owner.SendMessage("There is not enough arrows to equip!", ChatType.Task, ChatColour.Red);



                        owner.Equipment.UnequipItem(5);
                        _owner.DeleteItem(item);
                        

                    }
                    
                    
                }
                
            }
        }
        #endregion

this method is then called here for basic attack

 Spoiler

 //Deal damage to target
                state = SkillState.Attack;
                nextTrigger = Common.Clock + owner.AttackSpeed / (owner.HasEffect(ClientEffect.Cyclone) ? 3 : 1);
                uint dmg;
                if (owner.WeaponType == 500)
                {
                    ConquerItem item;
                    owner.Equipment.TryGetItemBySlot(5, out item);
                 
                    if (item == null || item.Durability < 1)
                    {
                        AbortAttack(); return;
                    }
                    if (!owner.Map.IsTGMap && item != null)
                    {
                        item.Durability -= 1;
                        owner.Send(ItemInformationPacket.Create(item, ItemInfoAction.Update));
                        
                            if (item.Durability == 0)

                            {
                                uint id = item.StaticID;
                                // owner.Equipment.UnequipItem(5);
                                (owner as Player).DeleteItem(item);
                                AutoEquip(item, id);

                            }
                        

                    }
                    dmg = owner.CalculateBowDamage(target, null, true);

                }
                else
                    dmg = owner.CalculatePhysicalDamage(target, null, true);

And also called here for auto reloading with scatter:

 Spoiler

//Take arrow count
            if (_magicType.WeaponSubtype == 500)
            {
                if (_magicType.UseItem == 50 && _magicType.UseItemNum > 0)
                {
                    ConquerItem item;

                    owner.Equipment.TryGetItemBySlot(5, out item);
                    
                    if (item == null)
                        return false;
                    if (item != null)
                    {

                        item.Durability -= _magicType.UseItemNum;
                        owner.Send(ItemInformationPacket.Create(item, ItemInfoAction.Update));

                        if (item.Durability <= 2)
                        {
                            uint id = item.StaticID;
                            // owner.Equipment.UnequipItem(5);
                            (owner as Player).DeleteItem(item);

                            AutoEquip(item, id);

                        }
                    }

                }
            }

So as you can see I have checked if the item was null first before calling anything from it in both basic attack part and scatter part. Thank you again for your time. I really appreciate this guidance and please excuse my silly mistakes.

there is a problem I cant quite catch what causes it, when the script runs for some reason the item seems to get unequipped but not deleted (with scatter).

[12tails]

Just to complement something, as Spirited said, instead of using the foreach to search for the id, you can use the Linq library to do it tho:

 Spoiler

public bool TryArrowAutoEq(uint _id, byte arloc)
{
    bool found = false;
    foreach (var i in Inventory.Values)
    {
        if (i.StaticID == _id)
        {
            Equipment.EquipItem(i, arloc, true);
			found = true;
            break;
        }
    }
	
	return found;
}

you could do:

 Spoiler

public bool TryArrowAutoEq(uint _id, byte arloc)
{
     Item[] _searched = (from a in Inventory.Values
                                   where a != null and a.StaticID == _id
                                   select a).ToArray();
     if (_searched.Lenght == 0) 
           return false;

     Equipment.EquipItem(_searched[0], arloc, true);
     return true;
}

By the way, the other things were already said

[Spirited]

Originally Posted by 12tails

Just to complement something, as Spirited said, instead of using the foreach to search for the id, you can use the Linq library to do it tho:

 Spoiler

public bool TryArrowAutoEq(uint _id, byte arloc)
{
    bool found = false;
    foreach (var i in Inventory.Values)
    {
        if (i.StaticID == _id)
        {
            Equipment.EquipItem(i, arloc, true);
			found = true;
            break;
        }
    }
	
	return found;
}

you could do:

 Spoiler

public bool TryArrowAutoEq(uint _id, byte arloc)
{
     Item[] _searched = (from a in Inventory.Values
                                   where a != null and a.StaticID == _id
                                   select a).ToArray();
     if (_searched.Lenght == 0) 
           return false;

     Equipment.EquipItem(_searched[0], arloc, true);
     return true;
}

By the way, the other things were already said

I would be careful with using a lot of LINQ for production level code. LINQ can be pretty bad on performance. A lot of the time, it helps to look at better algorithms, like using a hash map rather than doing a search.

[Super Aids]

Originally Posted by Spirited

I would be careful with using a lot of LINQ for production level code. LINQ can be pretty bad on performance. A lot of the time, it helps to look at better algorithms, like using a hash map rather than doing a search.

The biggest issue with overusing LINQ is when people overuse ToList() and ToArray() because it defeats the purpose of LINQ entirely.

You should only use those two if you really want to copy the data or if you need to enumerate the sequence more than once.

In the case above a hashmap would have solved the issue.

In fact Candy Conquer used that:


---

Also in the case above instead of ToArray() just do FirstOrDefault() since you just need the first element of the sequence and not all of them.

[Yupmoh]

Might be too late for this but why not use TryGetValue? I read it would be better performance wise since its only 1 lookup is performed as opposed to looping through the entire Inventory and checking if the ID is equal to what you provided?