This is for this question: Nested If Addition Going wrong somewhere? - Questions & Answers - Unity Discussions
I wanted to write up some suggestions, but the Answers system limits how much I can write and I thought this may be interesting for others as well, so here we go:
EDIT: Code here: http://dl.dropboxusercontent.com/u/30016491/ItemInspectorCode.unitypackage
Hi stuart6854 ,
I noticed some Design problems beside your actual problem with adding Items.
Using a List for the ItemDictionary isnāt a good idea, because that way it depends on the insertion order which position an Itemdefinition gets and you canāt access it fast by itās ID and things like you did in your ChopTree method will break
Item newItem = PlayerInventory.ItemDictionary[0]; // This will break when moving stuff around
Also gaps can be handy if you want to do stuff like
woodmaterial = 100, ironmaterial = 200, axe=25, shovel=26
ironshovelID = ironmaterial + shovel
Your instinct was good to name that Dictionary, so also use one:
static Dictionary<int,Item> _ItemDictionary = new Dictionary<int,Item>();
public static Dictionary<int,Item> ItemDictionary{
get{return _ItemDictionary;}
}
But the Problem with this is, that it doesnāt work together with something else I also wanted to suggest:
Get your basic Item definitions out of the codeā¦ Isnāt it a maintenance nightmare to add new Items to the code?
Just imagine adding your 50. āpublic Texture2D someTextureā to that PlayerInventory.
The Problem is, that when we want to be able to access those Definitions via the Inspector, we need a List<>, because Dictionaries are not supported by the Inspector, but while programming, we want to use a Dictionary for fast accessā¦
so letās use a trick to have both - and while weāre at it, the TextureDefinitions should also be static or a Singleton.
We will use a Singleton Pattern for Everything, so we can store all the Definitions in a Prefab, so we donāt tie that data to a single Scene.
Letās also not mix Item Definitions with how many of the Items we have. At the Moment you grab Items from the ItemDictionary and manipulate the currentStack - this of course doesnāt work if you later want to pass stacks around as you always manipulate the Definition.
So letās rethink that:
public enum ItemType { None, Weapon, Tool, Food, Resource, Construction }
[System.Serializable]
public class Item {
public string Name; // put this first, so it's used as a caption in Lists in the inspector
public int ID;
public string Description;
public int MaxStack;
public ItemType Type;
public GameObject ItemPrefab;
public Texture2D Icon;
}
[System.Serializable]
public class ItemStack
{
public int Size;
public Item Item;
}
Nowā¦ One Prefab to rule them all - a Singleton:
public class Definitions : MonoBehaviour {
#region Singleton Code
private static Definitions instance;
public static Definitions Instance
{
get {
if (instance == null) Debug.LogError("No Instance of Definitions in the Scene!");
return instance;
}
}
void Awake()
{
if (instance == null)
{
instance = this;
} else {
Debug.LogError("Just one Instance of Definitions allowed!");
}
}
#endregion
public List<Item> ItemDefinitions = new List<Item>();
private Dictionary<int,Item> itemDic;
public Dictionary<int,Item> ItemDictionary{
get{
// Definitions are never changed in game, so just copy references over once:
if (itemDic == null){
itemDic = new Dictionary<int, Item>();
foreach(Item item in ItemDefinitions) itemDic[item.ID] = item;
}
return itemDic;
}
}
}
Drag that Behaviour on an Empty GameObject called āSingletonsā and create a Prefab from it.
Never edit that GameObject in the Hierarchy, but always by selecting the Prefab in the Project View and editing it there. Now move all your Definitions to the inspector and we get:
Now letās also move the Craftable Definitions in here and at the same time improve them by allowing for Recipes with variable Number of Ingredients without the hack of setting the ResourceID to 99999.
We also rename this to ItemRecipe from CraftableItem, as technically itās not an Item - for that it would have to derive from Item:
[System.Serializable]
public class ItemRecipe {
public string Comment; // just for the Designer, not shown in game
public int CraftID;
public int ItemID;
public List<Ingredient> Ingredients = new List<Ingredient>();
public Item Item{
get {
return Definitions.Instance.ItemDictionary[ItemID];
}
}
}
[System.Serializable]
public class Ingredient {
public int ItemID;
public int Amount;
}
And add the following to Definitions
public List<ItemRecipe> Recipes = new List<ItemRecipe>();
private Dictionary<int,ItemRecipe> recDic;
public Dictionary<int,ItemRecipe> RecipeDictionary{
get{
// Definitions are never changed in game, so just copy references over once:
if (recDic == null){
recDic = new Dictionary<int, ItemRecipe>();
foreach(ItemRecipe rec in Recipes) recDic[rec.CraftID] = rec;
}
return recDic;
}
}
And once we move the Recipes into the Prefab, we get this:
Now before we look into the Inventory, one word of caution:
At the moment you donāt stack consumables like your Axe - I assume it breakes after a while?
If you will have consumables that stack later, you need to check their consumed status and only stack things that have the same damage.
Alright - this is where you had problems to begin with and thatās because your ChopTree method wasnāt creating new Items when it said it would, but just referenced the ones in the Dictionary, so when you wanted to stack them, it could be that you would stack the same Item instance with itself, but weāre looking into that laterā¦ for now letās just assume that we get new individual instances and want to add them to our Inventory.
public class Inventory : MonoBehaviour {
// This can also be used for chests etc.
public int Size = 8; // Number of item stacks allowed in here
void Awake()
{
stacks = new List<ItemStack>(Size);
while (stacks.Count<Size) stacks.Add(null); // initialize, so List can be used like an Array later
readonlyStacks = stacks.AsReadOnly();
}
// We don't expose this in the Inspector, because we don't want Items created that aren't a copy of our Definitions.
// This should only be used during runtime, not to give the player starting Items. This would have to be done in Code
private List<ItemStack> stacks;
private ReadOnlyCollection<ItemStack> readonlyStacks;
// we only return a readonly Version, because the Inventory methods should be used, not the List methods
public ReadOnlyCollection<ItemStack> Stacks
{
get { return readonlyStacks; }
}
// Removes the specified ItemStack from the inventory and returns it.
public ItemStack Remove(int pos)
{
ItemStack stack = stacks[pos];
stacks[pos] = null;
return stack;
}
// Removes one Item from the specified ItemStack and returns it.
public ItemStack RemoveOne(int pos)
{
ItemStack stack = stacks[pos];
if (stack == null) return null; // nothing there
stack.Size--; // take one
ItemStack newSt = new ItemStack(){ Size=1, Item=stack.Item };
if (stack.Size==0) stacks[pos] = null; // nothing left: clear slot
return newSt;
}
// Adds the specified stack to the inventory, returning the remaining stack that didn't fit or null if all fit
public ItemStack AddStack(ItemStack stack)
{
if (stack.Size<1 || stack.Item==null) Debug.LogError("Trying to add empty stack to inventory");
// First Run: Try to add stack to stacks already in the inventory
foreach(ItemStack st in stacks)
{
if (st != null st.Item.ID==stack.Item.ID) while(st.Size<st.Item.MaxStack stack.Size>0)
{
stack.Size--; // move one...
st.Size++; // ...over
}
}
// stack already empty?
if(stack.Size==0) return null;
// Second Run: Try to put the rest in an empty slot
for(int i = 0; i<stacks.Count; i++)
{
if (stacks[i]==null){ // found a free slot
stacks[i] = stack;
return null;
}
}
return stack; // return what's left
}
}
Now, how do we test this? I assume you will alter the state of Items, so we are not allowed to use the ones in the Dictionary, but need to make copies, so we can alter the damage etc. without changing the Originals.
For this, letās add some code to Item - Mono already has a method, so we donāt need to copy ourselves:
public Item Clone()
{
// return a shallow copy, as we don't need to clone the prefab and icon
return (Item)this.MemberwiseClone();
}
But be aware that referenced objects are not cloned - which is good for the prefab and icon, but if you reference something that also needs to be cloned, you need to call MemberwiseClone() on that object manually;
Also, letās add some convenience methods to Definitions:
public static Item ItemClone(int id)
{
return instance.ItemDictionary[id].Clone();
}
// used internally, doesn't check for valid stack sizes
public static ItemStack StackClone(int itemID, int size)
{
return new ItemStack(){ Size = size, Item = ItemClone(itemID) };
}
So we can finally do some Inventory tests:
public class TestStuff : MonoBehaviour {
void Start () {
Inventory inv = GetComponent<Inventory>();
for(int i = 1; i<=4; i++)
{
Debug.Log("### Round " + i);
TryAddStack(inv, Definitions.StackClone(3, 12)); // 12xChicken (maxStack:25)
TryAddStack(inv, Definitions.StackClone(4, 3)); // 3xWoodWall (maxStack:5);
TryAddStack(inv, Definitions.StackClone(2, 1)); // 1xAxe (maxStack:1)
}
Debug.Log("---");
printInventory(inv);
}
void TryAddStack(Inventory inv, ItemStack st)
{
ItemStack rest = inv.AddStack(st);
if (rest != null) Debug.Log("Couldn't fit " + rest.Size + " x " + rest.Item.Name);
}
void printInventory(Inventory inv)
{
foreach(ItemStack st in inv.Stacks)
{
if (st!=null) { Debug.Log(st.Size + " piece(s) of " + st.Item.Name); }
}
}
}
Which gives this output:
And now your ChopTree method can also be tidier:
void TryToChop(){
int amountGiven;
int itemID = -1;
Ray ray = Camera.main.ScreenPointToRay(new Vector3(Screen.width / 2, Screen.height / 2));
RaycastHit hit;
if (Physics.Raycast(ray, out hit)){
string hitTag = hit.transform.tag;
if (hitTag == "Tree"){
amountGiven = Random.Range(3, 7);
itemID = 0; // Wood
} else if (hitTag == "Rock"){
amountGiven = Random.Range(1, 4);
itemID = 1; // Stone
}
if (itemID >= 0) // hit something good
{
Inventory inv = GetComponent<Inventory>();
ItemStack choppedStuff = Definitions.StackClone(itemID, amountGiven);
ItemStack rest = inv.AddStack(choppedStuff); // watch out, choppedStuff.Size will change!
if (rest != null) Debug.Log("Dropped " + rest.Size + " x " + rest.Item.Name + " on the floor");
StartCoroutine(DisplayGUI(amountGiven, choppedStuff.Item.Name));
}
}
}
