Urgh… I hate to be one of those people but I’ve hit a problem and I cannot fathom a way round it.
I am not experienced at all in C# or programming for games, all very new territory for me. My background knowledge is excel VBA… very different! I’ve made lots of little projects to get a really basic idea of how to get things done with unity, and now I am trying to put a game together.
My game is a shop simulation, I’ve attached the entire project for you to look at.
So my problem is my programming… I know its rubbish, I am aware that this isn’t the way to do it! layer after layer of if statements on updates, its a mess but I don’t know how else to achieve the results. Would anyone take a look at my code and give me some pointers on better ways of doing things?
I’ve hit the point where I’m struggling to understand my own code to take things further.
using System.Collections;
using System.Collections.Generic;
using UnityEngine;
using UnityEngine.AI;
public class StaffAI : MonoBehaviour
{
public Transform CurrentWork;
public GameObject GameControllerObj;
public GameObject NextItemToWork;
public GameObject NextShelftoFill;
public Transform WorkThisItem;
public Transform ShelftoFill;
void Start()
{
}
void Update()
{
GameObject g = GameControllerObj;
NextItemToStock nextwork = g.GetComponent<NextItemToStock>();
NextItemToWork = nextwork.NextItemToWork;
NextShelftoFill = nextwork.NextWorkDest;
ShelftoFill = NextShelftoFill.GetComponent<Transform>();
WorkThisItem = NextItemToWork.GetComponent<Transform>();
if (CurrentWork == null)
{
CurrentWork = WorkThisItem;
NavMeshAgent agent = GetComponent<NavMeshAgent>();
agent.destination = CurrentWork.position;
}
if (Vector3.Distance(this.gameObject.transform.position, CurrentWork.transform.position) < 1)
{
CurrentWork.transform.parent = this.gameObject.transform;
CurrentWork = ShelftoFill;
NavMeshAgent agent = GetComponent<NavMeshAgent>();
agent.destination = CurrentWork.position;
}
if (this.gameObject.transform.childCount < 1)
{
Debug.Log("Trigs");
CurrentWork = WorkThisItem;
NavMeshAgent agent = GetComponent<NavMeshAgent>();
agent.destination = CurrentWork.position;
}
}
}
using System.Collections;
using System.Collections.Generic;
using UnityEngine;
public class ShelfData : MonoBehaviour {
public GameObject GameController;
public GameObject ItemCall;
public Transform ShelfCall;
public List<GameObject> items;
public GameObject L1ItemToStock;
public GameObject L2ItemToStock;
public GameObject L3ItemToStock;
public GameObject L4ItemToStock;
public GameObject L5ItemToStock;
public GameObject R1ItemToStock;
public GameObject R2ItemToStock;
public GameObject R3ItemToStock;
public GameObject R4ItemToStock;
public GameObject R5ItemToStock;
public Transform L1Pos;
public Transform L2Pos;
public Transform L3Pos;
public Transform L4Pos;
public Transform L5Pos;
public Transform R1Pos;
public Transform R2Pos;
public Transform R3Pos;
public Transform R4Pos;
public Transform R5Pos;
public bool L1Stocked;
public bool L2Stocked;
public bool L3Stocked;
public bool L4Stocked;
public bool L5Stocked;
public bool R1Stocked;
public bool R2Stocked;
public bool R3Stocked;
public bool R4Stocked;
public bool R5Stocked;
void Update()
{
if (L1Pos.childCount > 0) L1Stocked = true;
if (L2Pos.childCount > 0) L2Stocked = true;
if (L3Pos.childCount > 0) L3Stocked = true;
if (L4Pos.childCount > 0) L4Stocked = true;
if (L5Pos.childCount > 0) L5Stocked = true;
if (R1Pos.childCount > 0) R1Stocked = true;
if (R2Pos.childCount > 0) R2Stocked = true;
if (R3Pos.childCount > 0) R3Stocked = true;
if (R4Pos.childCount > 0) R4Stocked = true;
if (R5Pos.childCount > 0) R5Stocked = true;
if (L1Stocked == false)
{
ItemCall = L1ItemToStock;
ShelfCall = L1Pos;
}
else if (L2Stocked == false)
{
ItemCall = L2ItemToStock;
ShelfCall = L2Pos;
}
else if (L3Stocked == false)
{
ItemCall = L3ItemToStock;
ShelfCall = L3Pos;
}
else if (L4Stocked == false)
{
ItemCall = L4ItemToStock;
ShelfCall = L4Pos;
}
else if (L5Stocked == false)
{
ItemCall = L5ItemToStock;
ShelfCall = L5Pos;
}
else if (R1Stocked == false)
{
ItemCall = R1ItemToStock;
ShelfCall = R1Pos;
}
else if (R2Stocked == false)
{
ItemCall = R2ItemToStock;
ShelfCall = R2Pos;
}
else if (R3Stocked == false)
{
ItemCall = R3ItemToStock;
ShelfCall = R3Pos;
}
else if (R4Stocked == false)
{
ItemCall = R4ItemToStock;
ShelfCall = R4Pos;
}
else if (R5Stocked == false)
{
ItemCall = R5ItemToStock;
ShelfCall = R5Pos;
}
else
{
ItemCall = null;
}
if(ItemCall == null) this.gameObject.tag = "FullStorage";
string tag = ItemCall.name;
GameObject[] items = GameObject.FindGameObjectsWithTag(tag);
foreach (var item in items)
{
if ((this.gameObject.transform.position - item.transform.position).magnitude < 2)
{
item.transform.position = ShelfCall.transform.position;
item.transform.parent = ShelfCall;
}
}
//if (Vector3.Distance(this.gameObject.transform.position, CurrentWork.transform.position) < 1) ;
}
}
using System.Collections;
using System.Collections.Generic;
using UnityEngine;
public class NextItemToStock : MonoBehaviour
{
public GameObject NextWorkDest;
public GameObject NextItemToWork;
// Update is called once per frame
void Update()
{
GameObject[] gos;
gos = GameObject.FindGameObjectsWithTag("StorageUnit");
foreach (GameObject go in gos)
{
NextWorkDest = go;
GameObject g = go;
ShelfData nextwork = g.GetComponent<ShelfData>();
NextItemToWork = nextwork.ItemCall;
}
}
}
Honestly you picked a good time to stop and ask for help. I’ve seen code of this style go far over the horizon into the deep end where you really can’t refactor it without a lot of work.
If you could give a brief explanation about what the end result of this system is supposed to be, it will help greatly in guiding you to a better solution. And I don’t mean what each of these classes is designed for, just describe how the game should work.
Both those Updates feel unnecessary. Do you really need to do all that every frame? And doing a FindGameObjectsWIthTag in Update is usually wrong. Usually if you need to do this, you’ll store the results in a variable to reuse.
Your ShelfData script has several items repeated. If you had a class that was like Product with the variables that were relavent to that product, you could just have an array or list or something along those lines that you can loop through if you needed to. There is also the possibility that scriptable objects may serve your needs.
I can’t say much more because I don’t know what your scripts are suppose to do and you didn’t really ask anything.
I would present one script and then mention what it’s suppose to do and ask questions on it. Then when you feel you’ve been helped on it, move to another script, but you may be able to apply some of the knowledge to multiple scripts.
Very first thing that jumps out is, you need to learn to use arrays. All that repetitive code in your second script should be reduced to a couple of lines. (Also, I’m not sure whether that tutorial video covers it as well, but learn for loops, the array’s “other half”.
Lines 11 through 42 of that script could be replaced with 3 lines, for example:
public GameObject[] ItemsToStock = new GameObject[10];
public Transform[] positions = new Transform[10];
public bool[] stocked = new bool[10];
We can improve that some more, though. Because you have three different lists of things there, you should also learn to do classes. Parallel arrays (as seen above) are not great coding style, just because it’s still more places that code needs to be changed. If you changed the length of one of them to 15 and forget to change the others, suddenly you’ve got errors.
You can make a class that contains those 3 values, and make a single array of that class.
public class StockItem {
public GameObject itemGameObject;
public Transform position;
public bool stocked;
}
public StockItem[] items = new StockItem[10];
This code sample just shows how to declare these things (not much on how to use them) - see the tutorial linked above for more on that, and once you have a little better understanding of array we can proceed from there.
General thoughts:
-Think deeply about what needs to be done repeatedly. Some things don’t change, so cache those variables early and just reference them when necessary.
-Remove empty methods or move more appropriate code into them.
-Use direct references to objects wherever possible, rather than looking them up by tag/name/type.
-Use arrays rather than almost similar variables for large numbers of objects of the same type.
-Create plain C# classes to hold closely related data to reduce the number of arrays needed to reference those objects.
-Don’t code searches at all if you have known quantities of objects. Just place them in the editor and drop them on components in the inspector.
First script:
Cache the NavMeshAgent ONCE into a private variable during Start() and be done with it, rather than searching for components several times in Update().
Second script:
The many GameObject, Transform and boolean variables could have been replaced with arrays/lists. Much less typing, much less code to scroll past. A for loop would replace a lot of the code to check them. The tag string doesn’t need to exist - just use the reference directly in the lookup, if that one is really necessary (and see general stuff above).
Third script:
Caching again. Move the search into Start(), or possibly even use the editor to reference the objects in public variables if you have a fixed number. Also fetch the ShelfData from each object in Start(). Stick to only lookups in Update().
I would also strongly recommend not using tags in this fashion. If I’m reading it correctly, a bin is tagged “StorageUnit” until it is full, then its tag gets changed to “FullStorage”. Eventually I’m going to assume the bin can be emptied and changed back.
It would be smarter and more flexible to record if a bin is full or empty in a variable on the component. If you need general access, then add and remove bins from a collection as they fill and empty.
I’ve made lots of little projects to get a really basic idea of how to get things done with unity, and now I am trying to put a game together.
