Bug GetComponent is genuinely broken

I am at a loss for words here. I have written a game save/load system that converts the data I want to JSON and then back again. It works except I have a bug that I am pretty sure I didn't used to get and I haven't changed the code.

Basically when serialising the type of a component on an object is saved, and then also any relevant fields are saved with it.

When loading, it instantiates the relavent prefab then goes through each of the components in the save file. If the component already exists on the prefab, it just copies over the field values. If the component doesn't exist, it first adds it before then copying over the fields.

This works on all my prefabs except one - my camera.

It instantiates the prefab, but then when it gets to the CameraController component, it doesn't seem to be able to find it and so adds a new one. Leaving my camera with two camera controller scripts (one of which then gets the fields copied over, and the other one left with the default - all zeros)

Here is the code that does this little bit:

Code (CSharp):

1. foreach (string componentName in componentNames)
2. {
3.                 //Get the component
4.                 Component c = obj.GetComponent(componentName);
5.  
6.                 Debug.Log(obj.name + "->" + componentName + ": " + c);
7.  
8.                 //If the component doesn't exist (i.e. not one of the components on the prefab) then add it
9.                 if (c == null)
10.                     c = obj.AddComponent(Type.GetType(componentName));
11.  
12.                 //Get the component properties
13.                 JObject componentProperties = (JObject)JsonConvert.DeserializeObject(componentsGroup.GetValue(componentName).ToString(), settings);
14.  
15.                 //Load the component data into the component
16.                 JObject data = (JObject)componentsGroup.GetValue(componentName);
17.                 JsonUtility.FromJsonOverwrite(data.ToString(), c);
18.  
19.                 //Add the instance id of the component to the lookup
20.                 int instanceId = (int)componentProperties.GetValue("instanceID");
21.                 if (!instanceIdLookup.ContainsKey(instanceId))
22.                     instanceIdLookup.Add(instanceId, c);
23.  
24.                 //Set compoonent enable
25.                 bool enabled = (bool)componentProperties.GetValue("enabled");
26.                 ((MonoBehaviour)c).enabled = enabled;
27. }

So that debug log for everything except the camera is as expected and print the component. For the camera I get
"Main Camera->CameraController:"
Nothing, it can't find it.
So then I thought, okay sanity check. I'll add a loop that prints all the components on the "c" object. And sure enough the camera controller is there. Spelt exactly the same (as it should be, the script generates the names directly from the component when saving so can't really get a typo).

But I thought, maybe somehow some invisible characters I creeping in and messing it (no idea how but worth checking).

So I compared the length of the componentName string, with the name of the component that was found (using the same method as the save file -> component.GetType().ToString()). They are the same length.
I even did a loop, checking each individual character of both strings - they match.

So why on earth is GetComponent not finding it?
Any ideas because I'm all out?

Can add any other info if needed.

 

I tried changing it and using this instead and it found it. So yeah it's definitely bugged.

Code (CSharp):

1.  
2. //Get the component
3. Component c;
4. bool hasComponent = obj.TryGetComponent(Type.GetType(componentName), out c);
5.  
6. Debug.Log(obj.name + "->" + componentName + ": " + c);
7.  
8. //If the component doesn't exist (i.e. not one of the components on the prefab) then add it
9. if (!hasComponent)
10.                 c = obj.AddComponent(Type.GetType(componentName));
11. ;

 

Well as I just proved with my reply - it is bugged.
Maybe don't assume Unity can't have bugs just because a lot of people use it

 

I am using Type.GetType(componentName) in my changed version above - it works just fine. componentName can only come from someComponent.GetType().ToString(), so unless that script is later deleted, it will always exist

 

Saving Components types as string, in a Json file, is a very bad idea. Creating Types from strings have always been tricky in C#, because of Assemblies, Namespaces, Fully Qualified Names...

Generally speaking, you don't save Components of a game object. You need to rethink how to reconstruct your objects from save data (and your game objects data in general).

That said, if I absolutely had to do it ($100k check, or a gun to my head), I would just create a map (dictionary) of Enum -> ComponentType and save the enum value in the save file, instead of the Type name, something like this:

Code (CSharp):

1. public static class ComponentTypeHelper
2. {
3.     private static readonly Dictionary<ComponentTypeEnum, Type> ComponentTypes = new()
4.     {
5.         [ComponentTypeEnum.RigidBody] = typeof(Rigidbody),
6.         [ComponentTypeEnum.CustomScript] = typeof(CustomScript)
7.     }
8.  
9.     public static Type GetTypeFromEnum(ComponentTypeEnum value)
10.     {
11.         if (!ComponentTypes.TryGetValue(value, out var type))
12.             throw new Exception($"Unknown ComponentTypeEnum '{value}'. Did you add it to the mapping dictionary?");
13.  
14.         return type;
15.     }
16. }
17.  
18. public enum ComponentTypeEnum
19. {
20.     RigidBody,
21.     CustomScript
22. }

Then when loading data, I'd just call:

Code (CSharp):

1. var type = ComponentTypeHelper.GetTypeFromEnum(yourSavedEnumValue);

It's safer, cleaner and compile-time checked (using typeof() in the dictionary).

Now if you have hundreds of Components that need to be mapped... you'll have your proof that what you're doing by saving Types as strings is a very bad idea

 

Yeah I mean I'm don't love doing it but can you think of a better way of saving a list of components that an object contains to a save file (i.e. text)? Other than an enum which like you say isn't great if you have many different types. (I won't have hundreds, but potentially a few dozen)

 

Well you may need to save FullName of the type, instead of just the Name (and maybe also the Assembly's Fully Qualified Name).

But again, I strongly advice you to change your architecture, you never need to save a list of game object components. For eg. use Prefabs that have all the necessary components, and instantiate it (and if needed, pass additional data to it from the save file). Like this, you just need to save the prefab name/id in you save data.

 

That sounds like a terrible way to implement a save system. Especially adding non-existing components. That should not be a thing. Instead of adding/removing components at runtime consider enabling/disabling them.

The string-based naming is already discussed. Once you save that data, you cannot safely rename-refactor your classes at all, or you'd have to write special handling code in the loader. Both options are brutal on your mental load, and will be a source of frequent bugs.

Given the camera example, unless you allow the player to manually modify camera properties, it should not be required to save the camera or any of its fields. Say you have a platformer, the camera will focus on the player character and should restore itself after loading just by putting the focus back on the player character. If it's a top-down strategy game where the player can scroll the camera, you'd need to save the position and perhaps the zoom level at most.

Save/load should have the minimum information necessary to restore state and no more. Every field that is saved and restored should be manually specified, not grabbed and restored via reflection or similar measures if that's what you're doing.

 

Currently I do not add/remove components at runtime, so in theory that part of the system could be removed but I put it there as an extra protection. But even if the components do not change, they still have data that needs to be saved/loaded and this was the simplest way I could think of to link saved values to the component they belong in. This code works with any component simply by putting an attribue next to fields that should be saved. I don't want a save system that needs a special case implemented for every value I want to save.

The camera controller doesn't save much, just two values to do with the focal point of the controller (it's an orbiting camera) and these values absolutely need to be saved.

Reflection is used, but it is indeed manually specified with an attribute, not everything is saved.
Fields will be saved if they are public or explicity have the [SerializeField] attribute (unless ExcludeFromGameSave attribute is used)
E.g. for camera controller only refPoint and planeHeight are saved.

Code (CSharp):

1.     [SerializeField][ExcludeFromGameSave] private float orbitRotateSensitivity;
2.     [SerializeField][ExcludeFromGameSave] private float firstPersonRotateSensitivity;
3.     [SerializeField][ExcludeFromGameSave] private float pitchLimit;
4.  
5.     private Vector3 moveVelocity;
6.     [SerializeField][ExcludeFromGameSave] private float keyboardMoveSensitivity;
7.     [SerializeField][ExcludeFromGameSave] private float gamepadMoveSensitivity;
8.     [SerializeField][ExcludeFromGameSave] private float moveAcceleration;
9.     [SerializeField][ExcludeFromGameSave] private float moveFriction;
10.  
11.     private float zoomVelocity;
12.     [SerializeField][ExcludeFromGameSave] private float mouseZoomSensitivity;
13.     [SerializeField][ExcludeFromGameSave] private float gamepadZoomSensitivity;
14.     [SerializeField][ExcludeFromGameSave] private float zoomAcceleration;
15.     [SerializeField][ExcludeFromGameSave] private float zoomFriction;
16.     [SerializeField][ExcludeFromGameSave] private float zoomNonLinearityFactor;
17.     [SerializeField][ExcludeFromGameSave] private Vector2 zoomLimits;
18.     private float zoomDistance;
19.  
20.     [SerializeField][HideInInspector] private Vector3 refPoint;
21.     [SerializeField][HideInInspector] private float planeHeight;
22.  
23.     [SerializeField][ExcludeFromGameSave] private int lockoutControls;
24.     [SerializeField][ExcludeFromGameSave] private float moveToSpeed;
25.     [SerializeField][ExcludeFromGameSave] private float rotateToSpeed;

With this system if I create a new component, I can specify within that script what needs to be saved and what doesn't and I do not have to touch my save system as it is universal.

 

Saving/Loading in games has always been a manual (and painful) process, and it's for a reason. If you absolutely have to continue using your automated, reflection using, string to types system, save the type's FullName as I said previously.

 

I think I will actually use an enum map as suggested above, that way it will still work even if I rename classes - I will just have to update the map.

 

You could still use reflection to auto-populate the components (at least the MonoBehaviours you have access to) by creating your own attribute, something like:

Code (CSharp):

1. [MapComponentType(ComponentTypeEnum.CustomScript)]
2. public class CustomScript : MonoBehaviour
3. {
4.  
5. }

But... I still cringe that I had to write this

 

You understand you just moved the lump in the carpet somewhere else, right?

You still have a lumpy carpet save system. Consider trying a smoother one. You'll really enjoy it.

Load/Save steps:

https://forum.unity.com/threads/save-system-questions.930366/#post-6087384

An excellent discussion of loading/saving in Unity3D by Xarbrough:

https://forum.unity.com/threads/save-system.1232301/#post-7872586

Loading/Saving ScriptableObjects by a proxy identifier such as name:

https://forum.unity.com/threads/use...lds-in-editor-and-build.1327059/#post-8394573

When loading, you can never re-create a MonoBehaviour or ScriptableObject instance directly from JSON. The reason is they are hybrid C# and native engine objects, and when the JSON package calls

new

to make one, it cannot make the native engine portion of the object.

Instead you must first create the MonoBehaviour using AddComponent<T>() on a GameObject instance, or use ScriptableObject.CreateInstance<T>() to make your SO, then use the appropriate JSON "populate object" call to fill in its public fields.

If you want to use PlayerPrefs to save your game, it's always better to use a JSON-based wrapper such as this one I forked from a fellow named Brett M Johnson on github:

https://gist.github.com/kurtdekker/7db0500da01c3eb2a7ac8040198ce7f6

Do not use the binary formatter/serializer: it is insecure, it cannot be made secure, and it makes debugging very difficult, plus it actually will NOT prevent people from modifying your save data on their computers.

https://docs.microsoft.com/en-us/dotnet/standard/serialization/binaryformatter-security-guide

 

Maybe, but other than this issue (which I've now solved) it has given me much less headaches than a more "manual" way of saving which I've done on several projects in the past. So maybe it will need a few improvements here and there to make it more robust and maybe one day I will end up deciding this was a terrible idea, but for now I find it a much easier way of generating save files so I'm going to stick with it.
E.g. here is what the camera controller part of the save file looks like - just like how I would probably have saved it manually (okay maybe scale isn't 100% needed to be saved for a camera, but that's small overhead on one object)


I also do not try and "re-create" an instance of a monobehaviour. The component already exists on the instantiated prefab, and simply the values are copied over. I also have a system in place to fix references from id numbers, as obviously references to other objects can not simply be stored as text.

Also don't use playerprefs, just saving to a series of text files - makes save files more easily transportable.

As for security, this is a single-player game and quite honestly I don't care if players want to bump their stats etc. by modifying their save file - to each their own. I have mod support anyway so it's kind of a moot point.

 

That is a totally reasonable approach. If you fixed it, why throw it away now?

Sweet. I can see from this and the above that you are a realist and that is AWESOME.

Unfortunately too many gamedevs take this antagonistic "I will keep you from evaaaaar cheating in my game on your computer!!!!1 lulz!11!1" approach to save games.

Glad you got the GetComponent<T>() issues sorted!

 

In that case, you should have used a Data class (a.k.a POCO), that holds all the object's data , instead of a bunch of private serialized fields. Then you would just deserialize it from Json and pass it to the Prefab instance. Very simple and efficient.

EDIT: Can you tell us how did you solve it, for future readers. Thank you and happy that you solved it.

 

There are various ways I could improve it I'm sure, but for now as long as it works I find it really easy to use when I create a new script and need to specify what should/shouldn't be saved.

I used TryGetComponent instead and that worked - see my post above

 

I'm hesitant to wade into this whole debate; but this system seems brittle for another reason not brought up: many components can be included more than once on the same GameObject. Colliders are a prime example, but any MonoBehaviour can be stacked unless you explicitly declare it [DisallowMultipleComponent]. Checking if a component is present will only work when you're sure that there's only ever going to be one copy of the component.

There's a reason Unity has decided not to provide a "save whole object at runtime" facility, even though it relies heavily on saving scenes, saving prefabs, inspecting objects, providing an Undo stack, and instantiating objects all via the very same serialization mechanism. In the Editor's case, it can know when the code underlying the Type differs from the saved serialized form. At runtime, you're going to have to be just as sensitive to all the quirks of the internal object model and contracts that the Editor implements.

 

The „working fine so far“ part may come apart when you have made your first release, and then update your game, and then realize old saves aren‘t compatible with the changes you made.

Every savegame system needs versioning, on the savegame level at least and typically also on a per type level. You don‘t want to be in a situation where you need to load old fields and decide what to do with their data without knowing what savegame version they belong to, or: not knowing what data you are reading and how to interpret it. The real tricky changes are when no field name or type changed but how the value gets interpreted. Perhaps it used to be euler angles but now that same Vector3 is in radians - but was it already saved as radians or not? And was it the version with the bug where the xz values were accidentally flipped?

 

Pretty much this. Data you want to save really should be wrapped into pure C# objects. A good save system should contain as little, if not zero, quirky Unity specific data unless the serialiser specifically supports them (such as the Odin serialiser).

Separating model from view helps a lot here. If you plan ahead, you easily save the state of your game without much work by keeping it all separate in the pure C# land.

 

This is something I am aware of. Currently I don't re-use the same component more than once on an object so it's not an issue but if I ever do then I'll update my code to proces duplicates. Things like colliders are not included in the save file as they don't change so there is nothing to save.

 

The save file also includes a version number, if there is anything that would break between versions - this is handled by a separate function to patch the save file before it's loaded. This isn't even a problem specific to my solution. Even if you went the more typical route and used a C# class to create a save file, you're still going to get errors if you later delete something from that object, or add something new unless you handle it.

 

Nah, not with JSON.

- If a field is provided in the JSON data stream and no longer present in the save class, it is ignored.

- If a new field is present in the save class and there is no data for it, it remains

default(T)

While both of those things may later cause errors in your game's business logic, you won't "get errors" from either of those cases from the underlying serialization process.

JSON is awesome. Also, always be sure to leverage sites like:

https://jsonlint.com
https://json2csharp.com
https://csharp2json.io

 

Exactly. An error is an error, regardless of where it props up it's still going to need handling.
And I am already using JSON...the only difference is the source of the data
My system would have the exact same results as what you described - values will just be their default if they don't exist in the save file.
It works like this:
Serialise the component using JSON utlities
Delete anything that isn't wanted in the save file.

Then for loading:
Load the JSON using json utilities.

It works the same as a normal save system. Just the structure is not predefined by a class. There is no reflection based stuff on the fields - only on the components and attributes so it knows where they go then the usual JSON deserialisation stuff happens

E.g. say I have 3 objects each with 1 component (call them A, B, C)
FOR SAVE:
Serialise object1.A (same way as you would for a save class object), delete anything that is flagged to be excluded from the save file.
Serialise object2.B, delete anything that is flagged to be excluded.
Serialise object3.C, delete anything that is flagged to be excluded.

FOR LOAD:
Instantiate object1 prefab (this already has an A component)
Detect that some block of fields belong to the A component of object1.
Deserialise that JSON to populate object1.A
Fix references to other objects

Repeat for the other objects.

It's really not that much different to a regular save system at its core, just allows me an easier way to define what should/shouldn't be saved