ZScript: Readonly instances are not preserved in saved games

[Player701]

Consider the following ZScript (also attached to this report):

Code: Select all • Expand view

class TestInv : Inventory
{
    class<Actor> _classField;
    readonly<Actor> _instanceField;
   
    override void BeginPlay()
    {
        Super.BeginPlay();
        _classField = 'DoomImp';
    }

    override bool Use(bool pickup)
    {       
        if (!_classField)
        {
            Console.Printf("This block is never reached!");
        }
        else if (!_instanceField)
        {
            Console.Printf("Instance field is NULL");
            _instanceField = GetDefaultByType(_classField);
        }
       
        if (_instanceField)
        {
            Console.Printf("Instance field == %s", _instanceField.GetClassName());
        }
       
        return false;
    }
}

The following testing procedure demonstrates the buggy behavior:

1. Load the example script and go to any map.
2. Type "give TestInv" in the console.
3. Type "use TestInv" in the console. You will see two messages: "Instance field is NULL", then "Instance field == DoomImp".
4. If you type "use TestInv" again any number of times, you will only see the second message each time: "Instance field == DoomImp". This is expected behavior, since the value has been cached in _instanceField and is not NULL anymore.
5. Save your game and load the save.
6. Type "use TestInv" yet another time. You will see two messages like in the beginning, indicating that _instanceField has been NULLed. This is not expected behavior. _instanceField should have preserved its value.

It appears that at least in the case when an instance is retrieved via GetDefaultByType, it behaves as a transient variable. This implicit behavior is even worse than this kind of thing, since there is no error message indicating that the variable could not be saved. If such instances are not meant to be saved, then please at least add an error message to inform the user that they've done something bad and their code will not work properly.

Tested in GZDoom g3.7pre-281-gf11b20122, g3.7pre-283-g1b82e2078

[Graf Zahl]

You seem to trigger every single bit of unimplemented detail the engine has to offer. How are you managing that.?
You are correct about one thing: Actor default references are implicitly transient, their very nature makes it impossible to restore them from a savegame because they do not fit the pattern of serializable objects. They look like one but the deserializer won't be able to retrieve a pointer to them.
You shouldn't hold any pointers to them in persistent storage. But like many other things, either the engine goes for being flexible and slightly volatile or for foolproof but annoying. If this gets warned about it'd block a lot of legitimate use.

[Player701]

You seem to trigger every single bit of unimplemented detail the engine has to offer. How are you managing that.?

Probably because it's not possible to know for sure what is implemented and what isn't. Even though the source code is available, I'm not very familiar with it, and even if I look in there, it's not always obvious whether something is a bug or a technical limitation.

In this particular case, I thought I would cache the default instance so that I don't have to retrieve it every time. In my code, the class name passed to GetDefaultByType is not a compile-time constant like in the above example (actually, it's not a compile-time constant there either, but that can be easily changed), and the number of actors on the level that utilize this functionality could potentially be very high, so I thought caching would be faster. If I got an error message that something couldn't be serialized, there (probably) wouldn't be any questions about it - I would just make the corresponding field transient and write a method to check if the value is not null and cache it again otherwise (and I guess I will need such a method in this case as well).

What attracted my attention here was the lack of any obvious indication (obvious to the end-user, I mean) that a variable is not getting serialized. What I thought I would get didn't quite correspond to what I actually got, and I was quite surprised when I found out what the cause of the problem was.

[Graf Zahl]

Welcome to the wonderful world of programming!

The problem here is, that to the VM it looks like a normal class instance, but since it isn't, trying to restore it cannot succeed. Ultimately the VM is just a dumb bytecode interpreter, not a complex intelligent system that knows everything about the data it processes. That makes it very flexible, because it just interprets the code it gets fed, but in return it has no contextual information about what a reference points to. Just like any real programming environment that can sometimes lead to non-obvious errors. Safety always comes at a price, sometimes it's worth paying, sometimes it isn't.

Regarding efficiency, in this case, caching won't make any difference. Retrieving the defaults in an intrinsic that requires one or two VM instructions depending on from where you call it (The defaults are just a reference field in the class descriptor, there is no complex lookup involved.)

[Player701]

Regarding efficiency, in this case, caching won't make any difference. Retrieving the defaults in an intrinsic that requires one or two VM instructions depending on from where you call it (The defaults are just a reference field in the class descriptor, there is no complex lookup involved.)

I see. I am familiar with the concept of a VM but (like I said) not with GZDoom's source code, so it's hard for me to know whether some operation incurs any significant performance overhead. Other aspects can sometimes be more obvious (for example, I'm all but certain that Find/GetCVar does a linear search, and that's where caching would help, especially if a value is looked up every tick by many actors).

[Graf Zahl]

Correct, but CVAR references aren't serializable as well.

[Player701]

Correct, but CVAR references aren't serializable as well.

And it is clearly indicated by an error message when attempting to save the game, so it doesn't raise too many questions.

See, I understand that the current implementation of the VM has its limits and it's probably not worth fixing this particular issue. But it's still a kind of an uneasy feeling when you know that it's possible to write code that appears to work, only it doesn't. There are no error messages, no VM aborts - but the code is fundamentally broken, and you don't suspect a thing until you begin testing it thoroughly. And when it finally breaks, you weren't even expecting it would be this part that turns out to be broken - it just feels counter-intuitive that it doesn't work like you originally expected it to. This is precisely what happened to me. Well, I guess I will inform Marrub about this so that he can update his unofficial ZScript documentation - this way, more people will learn to avoid falling into this trap in their own code.