Starting VVVVVV without data.zip wrecks save data in 2.4

[Daaaav]

After compiling #869, I first ran that version of the game without data.zip. Which, as expected, resulted in the "You do not have data.zip!" message. However, after I added data.zip, I was unexpectedly greeted with a fullscreened VVVVVV set to 30 fps (neither my settings nor the defaults), and everything in both settings.vvv and unlock.vvv seemed to have been reset to either initialized or uninitialized values (only the settings specific to the localization branch were untouched). I then renamed data.zip in my localization branch, and the same reset happened (this time including the localization settings).

I say uninitialized values... For example:

        <window_width>1970225920</window_width>
        <window_height>661546092</window_height>

I don't know when this started, but 2.3.6 does not do this, and master does, so this bug was introduced somewhere in 2.4.

Last month I also lost my saves by random experimentation with loading code, so maybe we should first add some kind of protection to make sure the game never writes save data before it has actually loaded them from either the save files or from defaults, and only then fix this bug. The random experimentation was my own fault and very uncommon to happen, and this specific bug can be fixed, but who knows whether a similar bug won't be introduced again in the future.

[InfoTeddy]

So, this issue should more-or-less be fixed now.

997363c makes it so the window width and height are initialized values (320 and 240) if we can't get the size of the window from SDL (which was happening because, it does not exist if we are trying to quit the game so early). 75ee657 explicitly makes it so we won't write anywhere if the filesystem isn't initialized, which was the behavior previously but was only implicit because it relied on screenbuffer being NULL, and it was removed in a refactor (d0ffafe).

Just before I close this issue I want to make sure that this

so maybe we should first add some kind of protection to make sure the game never writes save data before it has actually loaded them from either the save files or from defaults, and only then fix this bug.

is fully addressed by 75ee657? Or do you think we should add some more protection somewhere else?

[Daaaav]

Just before I close this issue I want to make sure that this

so maybe we should first add some kind of protection to make sure the game never writes save data before it has actually loaded them from either the save files or from defaults, and only then fix this bug.

is fully addressed by 75ee657? Or do you think we should add some more protection somewhere else?

The idea I had was to add some sort of "canary option", a variable among the other options that's initialized to a certain "bad" value (like false) and then set to true when either loading the saves or when setting the defaults for all options when the settings don't exist. Not that this option would need to be saved to the settings files, it'd just be part of loading settings to set that value to true. But I guess VVVVVV doesn't really have one central place that specifically goes through all options to set defaults - the default settings come from a mixture of initial variable values, ScreenSettings_default(...) and maybe some other init things I haven't looked at closely.

So to make a full analysis of what specifically is going on here: if data.zip is missing, then FILESYSTEM_init(...) (as called from main) fails. This causes VVV_exit(1); to be called, VVV_exit calls cleanup();, and cleanup calls game.savestatsandsettings();. And by the time we were trying to initialize the filesystem, we haven't tried to load the stats and settings yet. So 75ee657 does protect against this specific case, but after the filesystem is initialized, there's still more code that could fail before loading stats and settings. For example, this block:

    // This loads music too...
    if (!graphics.reloadresources())
    {
        /* Something wrong with the default assets? We can't use them to
         * display the error message, and we have to bail. */
        SDL_ShowSimpleMessageBox(
            SDL_MESSAGEBOX_ERROR,
            graphics.error_title,
            graphics.error,
            NULL
        );

        VVV_exit(1);
    }

So if something's wrong with the default assets (such as bfont.png dimensions not exact multiples of 8!) the problem still happens, because now the filesystem has already been initialized. And nobody knows whether a VVV_exit won't also be added somewhere inside NETWORK_init(), graphics.init(), game.init(), etc somewhere down the road.

So, the best idea is probably to "unlock" writing to the saves at the top of game.loadstats and game.loadsettings.