Feedback Is this the best implementation for my feature?

Hi Everyone, I have a feature for my game I'm working on, and it's currently going great, but I feel like this might not be the way I should be doing this.

Basically I want to make a system (originally from Pixel Dungeon) where when starting a new game, potions get randomized visuals, so you don't know what a potion is until you use one in any way.
My idea was to have all my different potion prefabs, and then when the player starts a new game, assign the materials with colors to each prefab randomly.

So in the image above you can see a "Randomizer" gameObject, which has all the potion prefabs inside of it, and also links to those gameObjects and the material assets.

This is the randomizer script:

Code (CSharp):

1. using System.Collections;
2. using System.Collections.Generic;
3. using UnityEngine;
4.  
5. public class Randomizer : MonoBehaviour
6. {
7.     public bool isDone;
8.     public List<Material> potionMaterials = new List<Material>();
9.     public List<GameObject> potions = new List<GameObject>();
10.  
11.     // Start is called before the first frame update
12.     void Start()
13.     {
14.         List<Material> availablePotionMaterials = new List<Material>(potionMaterials);
15.         List<GameObject> availablePotions = new List<GameObject>(potions);
16.         while (availablePotions.Count > 0)
17.         {
18.             int random = Random.Range(0, availablePotions.Count);
19.             availablePotions[0].transform.GetChild(0).GetComponent<Renderer>().material = availablePotionMaterials[random];
20.  
21.             availablePotions.RemoveAt(0);
22.             availablePotionMaterials.RemoveAt(random);
23.         }
24.         isDone = true;
25.     }
26. }

This sets the materials for the prefabs inside of the "Randomizer" gameObject, and then, I would use "Instantiators" where I want a random potion to be placed, like so:

Code (CSharp):

1. using System.Collections;
2. using System.Collections.Generic;
3. using UnityEngine;
4.  
5. public class RandomizerInstantiator : MonoBehaviour
6. {
7.     public Randomizer randomizer;
8.  
9.     // Start is called before the first frame update
10.     private IEnumerator Start()
11.     {
12.         randomizer = GameObject.FindGameObjectWithTag("Randomizer").GetComponent<Randomizer>();
13.  
14.         while (!randomizer.isDone)
15.             yield return null;
16.  
17.         Spawn(randomizer.potions);
18.     }
19.  
20.     private void Spawn(List<GameObject> objects)
21.     {
22.         int random = Random.Range(0, objects.Count);
23.         Instantiate(objects[random], transform.position, transform.rotation);
24.     }
25. }
26.  

This "Instantiator" waits until the Randomizer has finished setting the materials, and then spawns a potion, so I would place many of these where I want "randomized" items to be placed.


This would also work in the future when I implement saves, because the GameObjects would be saved with their materials and when the player reloads a save they wouldn't shuffle again. I might in the future expand this to weapons, enemies, etc.

But I feel like having them sitting inside of a "Randomizer" gameObject deactivated is not the correct way to handle this and might cause issues, especially with memory management. Should I be doing this differently or is this a legitimate way to do it?
Thanks.

 

You can write scripts that modify and assign colors and even down to the hairiest nittiest grittiest dirtiest most detailed feature you can imagine.

But is it a good idea? No, probably not. Code like that rots quickly and becomes a nightmare to maintain.

Instead, move up one level and use the Unity editor to author your content properly, each one a different prefab variant. This is essentially PERFECTLY what prefab variants are for.

To implement an undiscovered potion system, at game start, choose the mapping to go between a particular potion and a particular visual. Just shuffling and randomly assigning each prefab reference or some kind of lookup table would suffice.

Keep in mind a reasonable upper limit on how many of these things you have, as vision-compromised people and those playing in less-than-ideal lighting conditions may be unable to play your game if you have 27 different flavors of "dark red potion."

But you should definitely NEVER EVER EVER write code like the above.

If you have more than one or two dots (.) in a single statement, you're just being mean to yourself.

How to break down hairy lines of code:

http://plbm.com/?p=248

Break it up, practice social distancing in your code, one thing per line please.

"Programming is hard enough without making it harder for ourselves." - angrypenguin on Unity3D forums

"Combining a bunch of stuff into one line always feels satisfying, but it's always a PITA to debug." - StarManta on the Unity3D forums

 

Thanks for the quick reply Kurt.
Don't know how to reply to a specific part of the message so I'll do it like this.

>
Instead, move up one level and use the Unity editor to author your content properly, each one a different prefab variant. This is essentially PERFECTLY what prefab variants are for.

To implement an undiscovered potion system, at game start, choose the mapping to go between a particular potion and a particular visual. Just assigning each prefab reference or some kind of lookup table would suffice.
>

So It's completely neccessary for me to have the prefabs in the Hierarchy at game start right? Especially for saving the game later. I'm new to Unity terms so I'm not sure I follow.

>
Keep in mind a reasonable upper limit on how many of these things you have, as vision-compromised people and those playing in less-than-ideal lighting conditions may be unable to play your game if you have 27 different flavors of "dark red potion."
>

Of course, accessibility is very important to me, if I need more colors they'll have patterns or combinations as well, always making contrast.

>
But you should definitely NEVER EVER EVER write code like the above.
>

I will make sure to write code more carefully in the future, I'm moving from web JS to C# and did that last night but I usually don't allow messy code.

 

The hierarchy is the scene, so no, not necessary. Possible I suppose, but usually one does not put all the content in the game scene at once.

Prefab variants let you make a base prefab for the potion, then make as many variants as you want that are the base prefab + changes. Those changes can be almost anything. Get familiar with it otherwise you will struggle to create large amounts of content.

If you have potion prefabs A, B, C and D and you have three possible potions (health, mana, invisibility), then you would create (as part of your serialized game state) a lookup table.

health
mana
invisibility

And shuffle up A,B,C,D and assign them linearly when the game starts.

After that, use that lookup table to decide which prefab to spawn.

 

So, I just had time to check back on this.
I basically just changed this

On my Randomizer script, instead of selecting prefabs on the scene, I created prefab variants and used those assets as the potions that can be instanced. Then at Start, the randomizer applies a random material to the variant directly. ( I had no idea I could make changes to assets that are not in the scene and they would work without saving changes after stopping)

Now to save that when loading a game I suppose I'll just save the material assignments on the script in a variable, and use that instead of the material randomizer when the game loads.

Now I just have to check if having the prefabs referenced in "Randomizer" aren't already being loaded into memory even though they aren't on the scene, that's my only fear atm. Will have to find a video about that stuff too.

Huge thanks for pointing out Prefab Variants, I was wondering if there was something like that and will use it a lot.

 

I ended up with this, works like a charm, and I'll be able to save and load it without any problem:

Code (CSharp):

1.  
2. using System.Collections;
3. using System.Collections.Generic;
4. using UnityEngine;
5.  
6. public class Randomizer : MonoBehaviour
7. {
8.     public bool isDone;
9.     // The potion prefab Variants and the materials that can be assigned to them
10.     public List<Material> potionMaterials = new List<Material>();
11.     public List<GameObject> potions = new List<GameObject>();
12.  
13.     // Where the materials will be saved when assigned
14.     private List<Material> assignedMaterials = new List<Material>();
15.  
16.     void Start()
17.     {
18.         // If materials where assigned before, place them in the prefabs in order
19.         if (assignedMaterials.Count > 0)
20.         {
21.             for (int i = 0; i < potions.Count; i++)
22.             {
23.                 AssignMaterial(potions[i], assignedMaterials[i]);
24.             }
25.         }
26.         // Else, assign random materials to prefabs
27.         else
28.         {
29.             List<Material> availablePotionMaterials = new List<Material>(potionMaterials);
30.             for (int i = 0; i < potions.Count; i++)
31.             {
32.                 int random = Random.Range(0, availablePotionMaterials.Count);
33.                 AssignMaterial(potions[i], availablePotionMaterials[random]);
34.  
35.                 assignedMaterials.Add(availablePotionMaterials[random]);
36.                 availablePotionMaterials.RemoveAt(random);
37.             }
38.         }
39.         isDone = true;
40.     }
41.  
42.     void AssignMaterial(GameObject potion, Material material)
43.     {
44.         GameObject potionFill = potion.transform.GetChild(0).gameObject;
45.         if (potionFill)
46.         {
47.             Renderer renderer = potionFill.GetComponent<Renderer>();
48.             if (renderer) {
49.                 renderer.material = material;
50.             }
51.         }
52.        
53.     }
54. }
55.