• 15
• 15
• 11
• 9
• 10

# Crafting table not drawing correctly

This topic is 1252 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

## Recommended Posts

This one has been bugging me for a while. I have a list CraftingItems which contains 6 recipes. Each recipe holds an item, and the item holds the quantity needed for the craft. The recipe also returns an output which is the item it creates. My method for checking if a recipe is available loops through the list and checks the items in my inventory against each recipe. Now the problem starts when I use my SetItemPositionInTable method.

       public void CheckForAvailableCrafts(Player player)
{
foreach (ItemRecipe recipe in CraftingList)
{
if (recipe.CheckForItemsNeeded(player) != null)
{
Output = recipe.Output;
SetItemPositionInTable(Output);
}
else if (recipe.CheckForItemsNeeded(player) == null)
{
Output = new Item();
Output.ItemName = "empty";

SetItemPositionInTable(Output);
}
}

private void SetItemPositionInTable(Item output)
{
for (int i = 0; i < CraftingTableItems.Count(); i++)
{
if (CraftingTableItems[i].ItemName == "empty")
{
CraftingTableItems[i].ItemName = output.ItemName;
CraftingTableItems[i].Texture = output.Texture;
CraftingTableItems[i].ItemDescription = output.ItemDescription;
break;
}
else if (CraftingTableItems[i].ItemName == output.ItemName)
{
break;
}

//fix this
else if (CraftingTableItems[i].ItemName != "empty" && output.ItemName == "empty")
{
CraftingTableItems[i].ItemName = output.ItemName;
break;
}
}
}


I think the problem is in the last else if statement. But basically what is happening is that the first item in my CraftingTableItems list is always empty, and then the 2nd item works. No other crafts are shown in the table.

The strangest thing about this is if I leave only the first 3 recipes in my CraftingRecipe list and comment out the break in the last else if, this works fine. Removing the break line and keeping all the recipes in my list gives me nothing in my table.

I used the console to check if the items are being checked against the recipe correctly. Each time a new recipe is available the console writes the name of the recipe, so the CheckForAvailableCrafts method works. I've given all the information about this that I can think of :P I just can't see the problem in the SetItemInTable method.

##### Share on other sites
Try string.Equals() instead of == and != because == may compare the pointers to the strings instead of the content of the strings.

##### Share on other sites

Well, the code shown in the OP has some more flaws. It isn't described what exactly it should do, so I made some assumptions about alternatives.

1.) CheckForAvailableCrafts is needlessly complex for what it does.

a) The IF and ELSE-IF branches are mutually exclusive. Hence it is sufficient to invoke the check once once and to use both the returned and the negated value, i.e. an IF and an ELSE branch.

b) Parts that are common in both branches can be moved outside of the block, especially if they are at the end of the block.

        public void CheckForAvailableCrafts(Player player)
{
foreach (ItemRecipe recipe in CraftingList)
{
if (recipe.CheckForItemsNeeded(player) != null)
{
Output = recipe.Output;
}
else
{
Output = new Item();
Output.ItemName = "empty";
}
SetItemPositionInTable(Output);
}
}


2.) If CheckForItemsNeeded returns that a recipe can be used, it is the recipe itself that is used further (as a copy template). But if the recipe cannot be used, a new empty Item is instantiated. It would be more convenient to have a static empty Item at hand that is returned as copy template.

        public void CheckForAvailableCrafts(Player player)
{
foreach (ItemRecipe recipe in CraftingList)
{
if (recipe.CheckForItemsNeeded(player) != null)
{
Output = recipe.Output;
}
else
{
Output = Empty;
}
SetItemPositionInTable(Output);
}
}


3.) Why is CheckForItemsNeeded returning a pointer?

4.) From the implementation of SetItemPositionInTable I assume that CraftingTableItems is allocated and filled once with probably a couple of "empty" items (otherwise I'd expect some re-dimensioning inside the routine). Letting the == and equals() problematic aside, the first branch will copy the given item into the current slot if the current item in the slot is an "empty" one. (This is done also if the new item is also "empty", but that doesn't hurt really.) The 2nd branch cancels the routine if the current item has the same name as the new one. The 3rd branch clears the slot if the current item is not an "empty" one but the new item is one.

Assuming that the recipes A, B, and C are checked in this order, and A and B return "can be used" but C returns "cannot be used". The CraftingTableItems hold 5 "empty" items for now.

a) The 1st invocation of SetItemPositionInTable will copy the new item A in the 1st IF branch into slot 0, so that CraftingTableItems will look like this:

{ A, empty, empty, empty, empty }

b) The 2nd invocation of SetItemPositionInTable, handling item B, will skip all 3 IF branches for slot 0. The 2nd iteration then goes into the 1st branch, so the resulting list will be:

{ A, B, empty, empty, empty }

c) The 3rd invocation of SetItemPositionInTable, handling item C (remember t is an "empty" item), skips the 1st and 2nd branch for slot 0, but matches the 3rd one! The result will be:

{ empty, B, empty, empty, empty }

This is probably not what you want. However, you haven't written what you want, so giving an advice means to guess. E.g. should non-empty items once placed into the table hold their position, or else it is suitable that their position changes on every call? Should the table be as compact as possible, and if so, should at least the order of items remain? What is to be done if the amount of slots is exceeded by the amount of useable recipes (if this is possible at all)?
Edited by haegarr