GetComponent is genuinely broken

Scripting
,
1

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:

foreach (string componentName in componentNames)
{
                //Get the component
                Component c = obj.GetComponent(componentName);

                Debug.Log(obj.name + "->" + componentName + ": " + c);

                //If the component doesn't exist (i.e. not one of the components on the prefab) then add it
                if (c == null)
                    c = obj.AddComponent(Type.GetType(componentName));

                //Get the component properties
                JObject componentProperties = (JObject)JsonConvert.DeserializeObject(componentsGroup.GetValue(componentName).ToString(), settings);

                //Load the component data into the component
                JObject data = (JObject)componentsGroup.GetValue(componentName);
                JsonUtility.FromJsonOverwrite(data.ToString(), c);

                //Add the instance id of the component to the lookup
                int instanceId = (int)componentProperties.GetValue("instanceID");
                if (!instanceIdLookup.ContainsKey(instanceId))
                    instanceIdLookup.Add(instanceId, c);

                //Set compoonent enable
                bool enabled = (bool)componentProperties.GetValue("enabled");
                ((MonoBehaviour)c).enabled = enabled;
}

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.

2

GetComponent is used by everyone in Unity. If it were broken as your post states, you’d know about it so maybe you should consider that this cannot be the case.

Unfortunately I cannot really debug your code above in my head so I’m not sure what else to offer. Maybe someone else can go through it and see something obvious.

3

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

//Get the component
Component c;
bool hasComponent = obj.TryGetComponent(Type.GetType(componentName), out c);

Debug.Log(obj.name + "->" + componentName + ": " + c);

//If the component doesn't exist (i.e. not one of the components on the prefab) then add it
if (!hasComponent)
                c = obj.AddComponent(Type.GetType(componentName));
;
4

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

5

I’m not assuming that, I was telling you to consider that it wasn’t. No need to twist my words.

Well if it’s a bug then you need to submit a bug report if it’s 100% broken.

6

If there’s a component name that isn’t working then try dumping what “Type.GetType(componentName)” gives you, maybe NULL and not a type if the component name cannot be found.

Are you sure it’s not expecting a qualified type name for this type?

7

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

8

That’s not what I said. There’s a reason why qualified type names exist. I’m not sure what ToString gives you here but I can check.

I believe you need to use:

By “working” are you saying “Type.GetType” with that component-name is returning a valid type?

I would’ve thought it was because I’m sure that GetComponent or TryGetComponent would give you an argument exception if you passed NULL so I’m not sure what’s going on in your case here without more info.

9

Just to show an example based upon a super quick check:

// This won't work, will return NULL.
UnityEngine.Debug.Log(Type.GetType("UnityEngine.Rigidbody2D"));

// This will work fine.
UnityEngine.Debug.Log(Type.GetType("UnityEngine.Rigidbody2D, UnityEngine"));

// This won't work.
UnityEngine.Debug.Log(Type.GetType(typeof(Rigidbody2D).ToString()));

// This will work fine.
UnityEngine.Debug.Log(Type.GetType(typeof(Rigidbody2D).AssemblyQualifiedName));

It has to be assembly-qualified. Maybe this will help, assuming you’ve not already got that component name output in your JSON.

10

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:

public static class ComponentTypeHelper
{
    private static readonly Dictionary<ComponentTypeEnum, Type> ComponentTypes = new()
    {
        [ComponentTypeEnum.RigidBody] = typeof(Rigidbody),
        [ComponentTypeEnum.CustomScript] = typeof(CustomScript)
    }

    public static Type GetTypeFromEnum(ComponentTypeEnum value)
    {
        if (!ComponentTypes.TryGetValue(value, out var type))
            throw new Exception($"Unknown ComponentTypeEnum '{value}'. Did you add it to the mapping dictionary?");

        return type;
    }
}

public enum ComponentTypeEnum
{
    RigidBody,
    CustomScript
}

Then when loading data, I’d just call:

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 :smile:

11

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)

12

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.

13

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.

14

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.

    [SerializeField][ExcludeFromGameSave] private float orbitRotateSensitivity;
    [SerializeField][ExcludeFromGameSave] private float firstPersonRotateSensitivity;
    [SerializeField][ExcludeFromGameSave] private float pitchLimit;

    private Vector3 moveVelocity;
    [SerializeField][ExcludeFromGameSave] private float keyboardMoveSensitivity;
    [SerializeField][ExcludeFromGameSave] private float gamepadMoveSensitivity;
    [SerializeField][ExcludeFromGameSave] private float moveAcceleration;
    [SerializeField][ExcludeFromGameSave] private float moveFriction;

    private float zoomVelocity;
    [SerializeField][ExcludeFromGameSave] private float mouseZoomSensitivity;
    [SerializeField][ExcludeFromGameSave] private float gamepadZoomSensitivity;
    [SerializeField][ExcludeFromGameSave] private float zoomAcceleration;
    [SerializeField][ExcludeFromGameSave] private float zoomFriction;
    [SerializeField][ExcludeFromGameSave] private float zoomNonLinearityFactor;
    [SerializeField][ExcludeFromGameSave] private Vector2 zoomLimits;
    private float zoomDistance;

    [SerializeField][HideInInspector] private Vector3 refPoint;
    [SerializeField][HideInInspector] private float planeHeight;

    [SerializeField][ExcludeFromGameSave] private int lockoutControls;
    [SerializeField][ExcludeFromGameSave] private float moveToSpeed;
    [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.

15

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.

16

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.

17

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:

[MapComponentType(ComponentTypeEnum.CustomScript)]
public class CustomScript : MonoBehaviour
{

}

But… I still cringe that I had to write this :eyes:

18

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://discussions.unity.com/t/799896/4

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

https://discussions.unity.com/t/870022/6

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

https://discussions.unity.com/t/892140/8

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() on a GameObject instance, or use ScriptableObject.CreateInstance() 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

19

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)
9371393--1310765--upload_2023-9-27_19-19-20.png

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.

20

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() issues sorted!