Question Losing objects between methods.

Not sure how else to describe it. I have a List<Hotkey> hotkeys that is initialized as empty. As the code runs I add objects to that list. For some reason down the line the object in the list loses some of it's data. Here is the model structure. More explanation after.

Code (CSharp):

1. public class Hotkey
2. {
3.     public Models.Spell Spell { get; set; }
4.     public string Number { get; set; }
5. }
6.  
7. public class Spell
8. {
9.     public string Name { get; set; }
10.     public List<Rune> Runes { get; set; }
11. }
12.  
13. public class Rune
14. {
15.     public string Name { get; set; }
16.     public NodeTypes Type { get; set; }
17. }
18.  

Here is the code where all this is being manipulated:

Code (CSharp):

1. using System.Collections;
2. using System.Collections.Generic;
3. using UnityEngine;
4. using Models;
5. using UnityEngine.UI;
6. using System.Linq;
7.  
8. public class SpellBar : MonoBehaviour
9. {
10.     [SerializeField] public GameObject hotkeyPf;
11.     [SerializeField] public GameObject spellObj;
12.     private List<Spell> spellBook = new List<Spell>();
13.     private List<Hotkey> hotkeys = new List<Hotkey>();
14.     private List<string> keys = new List<string>();
15.  
16.     private readonly float globalCooldown = 0.25f;
17.     private float globalCooldownTimer = 0f;
18.  
19.     private void Start()
20.     {
21.    
22.     }
23.  
24.     private void Update()
25.     {
26.         globalCooldownTimer += Time.deltaTime;
27.     }
28.  
29.     public void AddSpell(Spell spell)
30.     {
31.         spellBook.Add(spell);
32.    
33.         GameObject hotkeyObj = Instantiate(hotkeyPf);
34.         hotkeyObj.GetComponentInChildren<Text>().text = spellBook[spellBook.Count - 1].Name;
35.         hotkeyObj.transform.SetParent(transform);
36.  
37.         Hotkey newHotkey = new Hotkey() { Number = spellBook.Count.ToString(), Spell = spell };
38.  
39.         hotkeys.Add(newHotkey);
40.         keys.Add("Alpha" + spellBook.Count.ToString());
41.  
42.         hotkeys.ForEach(hotkey => hotkey.Spell.Runes.ForEach(rune => Debug.Log(rune.Name)));
43.  
44.         /*GameObject spellCast = new GameObject();
45.         spellCast.AddComponent<SpellCast>();
46.         spellCast.GetComponent<SpellCast>().SetSpell(newHotkey);*/
47.     }
48.  
49.     private void OnGUI()
50.     {
51.         Event e = Event.current;
52.  
53.         if (e.isKey && keys.Contains(e.keyCode.ToString()) && globalCooldownTimer > globalCooldown)
54.         {
55.             globalCooldownTimer = 0;
56.  
57.             Hotkey hotkey = hotkeys[0]; //hotkeys.Where(hotkey => hotkey.Number == e.keyCode.ToString()[5].ToString()).ToList()[0];
58.  
59.             Debug.Log("Num of Hotkeys: " + hotkeys.Count);
60.             Debug.Log("Num of Runes: " + hotkey.Spell.Runes.Count);
61.             Debug.Log("Spell Name: " + hotkey.Spell.Name);
62.             hotkey.Spell.Runes.ForEach(rune => Debug.Log(rune.Name));
63.  
64.             /*GameObject spell = new GameObject();
65.             spell.AddComponent<SpellCast>();
66.             spell.GetComponent<SpellCast>().SetSpell(hotkey.Spell);*/
67.         }
68.     }
69. }
70.  

Here is a link of a video (I'm sorry for the awful quality) showing the problem through breakpoints (on lines 37 and 50): https://imgur.com/gallery/XBs5olt

1. Starting the game I open up some menus and select some runes (Fire and Velocity). I name the spell "F".
2. On saving the spell the first breakpoint is hit and I check the List<Hotkey> object and verify that the new hotkey has been added. There is one Hotkey, it has a spell attached to it, and that spell is named "F" with two runes, fire and velocity.
3. I hit continue and return the game where I hit Alpha1 which triggers the OnGui call and then the second breakpoint.
4. I check the List<Hotkey> object again. The hotkey is still there. There is still a spell attached. It still has a name but the runes are gone now.

So what the heck is happening here? How come the spell keeps it's name but the runes list empties?

I made a fiddle to model the same scenario best I could and it works there. https://dotnetfiddle.net/4rvApD

I'm really at a loss. Thank you all in advance for your time looking into this.

EDIT: Solved. I wasn't respecting the reference to runes and was clearing unintentionally. Thank you all for your input, especially you orionsyndrome!

 

Be careful processing actual business logic in OnGUI()... it can be called more than once per frame:

https://docs.unity3d.com/ScriptReference/MonoBehaviour.OnGUI.html

I don't see how that could be directly related to what you are seeing, but it's a thing to consider.

 

You're not showing how you're building spell and adding runes there.
Quite possibly you're changing the spell reference to another empty spell somewhere (or runes collection itself, which is more sneaky).

Fiddle demo doesn't prove or disprove anything.

Where do you use spellObj btw?

Try making your runes collection a readonly, see if that will crash / fail to compile.

 

In almost any case where data seems to be one way in method A / case 1 and another way in method B / case 2, the reason is that you call those methods on different objects which hold different data.

First of all as Kurt warned you, OnGUI is an event processor and is called for several events. You just check "isKey" which is true for any key event. So your code would run at least twice (on key down and on key up). I don't think that's what you want. You should actually react to a specific event by checking the event type.

As for figuring out what objects you're dealing with, you can add a context object to your Debug.Log messages. This context object has to be a UnityEngine.Object derived object. When you click on the log message in the console the Unity editor will highlight that object in your project whereever it may be. Do that for your two cases with "gameObject" and you will see which objects you're dealing with. If in both cases the same object is referenced, there has to be an issue somewhere else in your code where you may remove or overwrite the runes. Common issues are accidentally shared lists or objects and accidentally reusing them.

 

I didn't show. That is being done elsewhere. I didn't show it because I thought showing the rune's elements was enough and didn't want to confuse with extra code. I'll put it here though.

Code (CSharp):

1. using System.Collections;
2. using System.Collections.Generic;
3. using UnityEngine;
4. using Models;
5. using Constants;
6. using UnityEngine.UI;
7.  
8. public class SpellMakerMenu : MonoBehaviour
9. {
10.     [SerializeField] public GameObject modNodePf;
11.     private SubMenuController subMenuController;
12.     private RadialLayoutGroup group;
13.     private List<Rune> runes = new List<Rune>();
14.     private List<GameObject> modNodes = new List<GameObject>();
15.     private bool hasMain = false;
16.  
17.     void Start()
18.     {
19.         subMenuController = transform.parent.GetComponent<SubMenuController>();
20.         group = transform.Find("ModNodes").GetComponent<RadialLayoutGroup>();
21.         group.RadiusStart = 180;
22.     }
23.  
24.     public void OnSaveSpellButtonClick()
25.     {
26.         Spell newSpell = new Spell()
27.         {
28.             Name = transform.Find("NameInput").GetComponent<InputField>().text,
29.             Runes = runes
30.         };
31.  
32.         GameObject.Find("SpellBar").GetComponent<SpellBar>().AddSpell(newSpell);
33.  
34.         //Reset everything.
35.         transform.Find("NameInput").GetComponent<InputField>().text = "";
36.         transform.Find("MainNode").GetComponentInChildren<Text>().text = "";
37.         modNodes.ForEach(node => Destroy(node));
38.         modNodes.Clear();
39.         runes.Clear();
40.         hasMain = false;
41.  
42.         subMenuController.CloseMenus();
43.         subMenuController.SwitchControls();
44.     }
45.  
46.     public void OnNodeButtonClick()
47.     {
48.         subMenuController.ToggleRunesMenu();
49.     }
50.  
51.     public void AddNode(Rune rune)
52.     {
53.         if (rune.Type == NodeTypes.Main && !hasMain)
54.         {
55.             hasMain = true;
56.             runes.Add(rune);
57.             transform.Find("MainNode").GetComponentInChildren<Text>().text = rune.Name;
58.  
59.             ChangeNode();
60.         } else if (rune.Type == NodeTypes.Mod && hasMain)
61.         {
62.             runes.Add(rune);
63.  
64.             GameObject modNode = modNodes[modNodes.Count - 1];
65.             modNode.GetComponentInChildren<Text>().text = rune.Name;
66.             modNode.GetComponent<RuneButton>().SetRune(rune);
67.  
68.             ChangeNode();
69.         }
70.  
71.         if (runes.Count > 1)
72.             ReorderRunes();
73.     }
74.  
75.     private void ChangeNode()
76.     {
77.         GameObject modNode = Instantiate(modNodePf);
78.         modNode.transform.SetParent(transform.Find("ModNodes"));
79.         modNodes.Add(modNode);
80.     }
81.  
82.     private void ReorderRunes() => group.AngleDelta = 360 / transform.Find("ModNodes").gameObject.transform.childCount - 1;
83. }
84.  

The spell is made in the code above then sent to the first script I showed to be attached to a hotkey and watched to see if it's triggered. Nowhere in first script is the spell object touched. Again, it's so strange to me that the spell's name member remains the same while the runes are erased. I figured its some quirk with Unity or something I just haven't experienced before

I did what you said and changed some things with the script above.

Code (CSharp):

1. [SerializeField] public GameObject modNodePf;
2.     private SubMenuController subMenuController;
3.     private RadialLayoutGroup group;
4.     //private List<Rune> runes = new List<Rune>();
5.  
6.     private readonly List<Rune> runesRO = new List<Rune>() {
7.         new Rune() { Name = "Fire", Type = NodeTypes.Main },
8.         new Rune() { Name = "Velocity", Type = NodeTypes.Mod },
9.     };

And changed the following accordingly.

Code (CSharp):

1. public void OnSaveSpellButtonClick()
2.     {
3.         Spell newSpell = new Spell()
4.         {
5.             Name = transform.Find("NameInput").GetComponent<InputField>().text,
6.             Runes = runesRO
7.             //Runes = runes
8.         };
9.  
10.         GameObject.Find("SpellBar").GetComponent<SpellBar>().AddSpell(newSpell);
11.         //Stuff here not shown.
12.     }

Now when I run it, it works. The runes aren't lost. HOWEVER, when I tried to change the spell model to be a readonly collection and make the following changes:

Code (CSharp):

1.  public void OnSaveSpellButtonClick()
2.     {
3.         ReadOnlyCollection<Rune> runesRO = new ReadOnlyCollection<Rune>(runes);
4.  
5.         Spell newSpell = new Spell()
6.         {
7.             Name = transform.Find("NameInput").GetComponent<InputField>().text,
8.             Runes = runesRO
9.         };
10.  
11.         GameObject.Find("SpellBar").GetComponent<SpellBar>().AddSpell(newSpell);
12. //Stuff here not shown.
13. }

The behavior changes back...again, when I hit save there are two runes. When I trigger the spell though there are zero. I'm baffled. Did I use the wrong type of object for my model?

Code (CSharp):

1. public class Spell
2.     {
3.         public string Name { get; set; }
4.         public ReadOnlyCollection<Rune> Runes { get; set; }
5.     }
6. }

 

You didn't understand what I meant with readonly.
Anyway, do you know how references work?

You clearly produce a list just once in line 13

Code (csharp):

1. private List<Rune> runes = new List<Rune>();

and then you reuse this same object over and over again in line 29

Code (csharp):

1. Runes = runes

I'm not sure how and why you don't see that as a problem. It's an immediate red flag.
Each rune list must be its own respectable instance, you can't just reuse the existing one.

And then you clear it right after.

Your code is a mess btw, and I don't mean that to hurt your feelings. That particular combination of doing things all over the place, mixing & matching all kinds of lambdas and naming, and then using Linq to iterate? Ooh boy.

I don't know if you can appreciate how complicated-looking code can oversaturate your senses to the point of missing out on obvious errors. That's the only reason why style matters.

 

I'm not sure how else to interpret what you are saying. I created a read-only, assigning it's members at the same time. Then passed it to my other component and it worked. It just didn't solve my problem since I need to be able to add to these lists.

I commented out the runes.Clear() and that fixed the issue. I'll try to respect references more and learn about them.

I take no offense at my code's messiness. I just don't know what good C# code looks like so I don't know what repos to use as examples. Can you recommend some repos for me to look at?

Thank you for your help in this. I truly appreciate it.

 

It doesn't matter really, because it wouldn't help in this case.

The point of that was to prevent re-assigning the whole instance to the field, because I was 90% certain that you were overwriting the actual instance with another one. Readonly keyword would prevent you from doing that, potentially highlighting the point of failure.

If you carefully read the posts by Kurt and Bunny, they also suspected the same thing.

However, in the end, you never actually created another instance, so this wouldn't highlight the source of the problem at all, but it was worth trying.

 

Well yes and no. The coding quality varies a lot on the Github and elsewhere. I've seen quite well-written examples and repos and usually it's in some commercial assets or asset-worthy libraries, but I couldn't name even one from the top of my mind. I'm sure you can find something.

It's all about stylistic choices, there is nothing super-bad with how you write your code, don't take me wrong. But in my opinion, get rid of Linq in Unity. Yes, it can be useful sometimes, maybe even for what you're doing here, in small doses, but try to write it more legibly, create dedicated methods, name everything properly. Make sure each function does what it says on the tin. Focus on single responsibility. Do not introduce gotchas will multiple naming systems. Don't be clever with anonymous functions / lambdas, use them sparsely. Be explicit whenever possible, but do not write overly verbose code either. And so on.

There are so many things I would do differently with your code, but I don't have time just now to recreate it. And now I'm seeing it's kind of hard to explain this in a sentence. But this is what got you derailed, you can't really read everything like a prose, and you ought to.

If you stumble upon any of my tutorials/examples, this is also very unlike my actual production code, so not very representative of how code should inside the actual project.

 

So as I predicted, it was a shared List Since your "SpellMakerMenu" script seems to "build" spells, you probably want to simply create a new list instead of calling Clear on the old one that is now referenced by the last created spell. Though that's why it's common to actually use a constructor where you pass in the list of runes and the constructor would actually create a copy of the passed collection so it has its own.

Whenever you create an object you should think really hard about who actually owns that object and who should have a reference to it. Everyone with a reference to an object can interact with that object. Since the runes inside a spell should only belong to that spell, the spell will need its own list.

 

Talking about spells, I forgot to spell out a fix

Code (csharp):

1. Runes = runes // line 29

should be

Code (csharp):

1. Runes = new List<Rune>()

And line 13 is not needed, but then also AddRune method has to be changed to actually reach the desired collection, instead of accessing local

runes

.

I mean I guess you can leave line 13 as declaration only

Code (csharp):

1. private List<Rune> runes;

And then reassign that on the spot with (line 29)

Code (csharp):

1. Runes = runes = new List<Rune>()

(Obviously get rid of

runes.Clear()

like you said you did)
This would work, but it's not a good design, imo.

 

If you're still reading this, here's your first second code box remade. I've done this strictly by looking this one example, so it's far from ideal, but you can maybe notice the differences and form some ideas.

Code (csharp):

1. using System;
2. using System.Text;
3. using System.Collections;
4. using System.Collections.Generic;
5. using UnityEngine;
6. using Models;
7. using UnityEngine.UI;
8.  
9. public class SpellBar : MonoBehaviour {
10.  
11.   [SerializeField] GameObject _hotkeyPf;
12.   [SerializeField] GameObject _spellObj;
13.  
14.   List<Spell> _spellBook;
15.   List<Hotkey> _hotkeys;
16.   List<string> _keys;
17.  
18.   readonly float globalCooldown = 0.25f;
19.   float globalCooldownTimer = 0f;
20.  
21.   void Awake() {
22.     _spellBook = new List<Spell>();
23.     _hotkeys = new List<Hotkey>();
24.     _keys = new List<string>();
25.   }
26.  
27.   void Update() {
28.     globalCooldownTimer += Time.deltaTime;
29.   }
30.  
31.   public void AddSpell(Spell spell) {
32.     _spellBook.Add(spell);
33.  
34.     var obj = produceSpellBookHotKey(_hotkeyPf, _spellBook.Count);
35.  
36.     obj.GetComponentInChildren<Text>().text = _spellBook[_spellBook.Count-1].Name;
37.     _keys.Add($"Alpha {_spellBook.Count.ToString()}");
38.   }
39.  
40.   GameObject produceSpellBookHotKey(GameObject prefab, int index) {
41.     var obj = (GameObject)Instantiate(prefab);
42.     obj.transform.SetParent(transform);
43.  
44.     var newHotkey = new Hotkey() {
45.       Number = index.ToString(),
46.       Spell = spell
47.     };
48.  
49.     _hotkeys.Add(newHotkey);
50.  
51.     foreach(var hotkey in _hotkeys) {
52.       Debug.Log(stringifyList(hotkey.Spell.Runes, (i, item) => item.Name ));
53.     }
54.  
55.     return obj;
56.   }
57.  
58.   void OnGUI() {
59.     var e = Event.current;
60.  
61.     if(e.isKey && keys.Contains(e.keyCode.ToString()) && globalCooldownTimer > globalCooldown) {
62.       globalCooldownTimer = 0f;
63.  
64.       var hotkey = _hotkeys[0];
65.       Debug.Log($"Num of Hotkeys: {_hotkeys.Count}");
66.       Debug.Log($"Num of Runes: {_hotkey.Spell.Runes.Count}");
67.       Debug.Log($"Spell Name: {_hotkey.Spell.Name}");
68.       Debug.Log(stringifyList(hotkey.Spell.Runes, (i, item) => item.Name ));
69.  
70.     }
71.   }
72.  
73.   static StringBuilder _sb;
74.  
75.   static string stringifyList<T>(IList<T> list, Func<int, T, string> labelOf) {
76.     if(item is null || list is null || list.Count == 0) return string.Empty;
77.     if(_sb is null) _sb = new StringBuilder(); else _sb.Clear();
78.     for(int i = 0; i < list.Count; i++) {
79.       if(i > 0) _sb.Append(", ");
80.       _sb.Append(labelOf(i, list[i]));
81.     }
82.     return _sb.ToString();
83.   }
84.  
85. }