Pin Object / Pin Object Floating causes savegame corruption if pinned object is destroyed in Vanilla SR2

[EngineOfDarkness]

Video about the issue (still uploading, about an 30 minutes left, see section "Steps to Reproduce" for written reproduction steps):

https://youtu.be/JrgjHThny8U

Note I have not yet written any code, this issue is basically all what I currently "know" about it.

Preface

I tried this in singleplayer but I assume the same problem will apply to multiplayer aswell.

It may make sense to this hide issue from the public because if this happens in MP aswell you can "grief" Vanilla Multiplayer Matches simply by "pinning" your own ship and sending it to certain doom right at the start of the game. Of Course you can (currently) grief all matches including modded ones

After that point, the match is doomed if people count on saving to continue another day, because of course the game doesn't instantly crash but happily chugs along while the proverbial doomsday clock is ticking. I'm not sure if anything but trying to "load" triggers the issue, so you might be able to play for hours and be none the wiser until you load your game again.

This happened to me in SP, twice. Once with your OpenStarRuler-Modpack and once with Colonisation Expansion from @Skeletonxf

Steps to Reproduce

1. Start new Game, no need to change anything
2. Click on your Starter Cruiser
3. Press "p" (or whatever your keybind is for "Pin Object"), should open a Popup for your Cruiser
4. Make sure the Cruiser is still selected
5. Open the Console with ^ (or whatever key you use)
6. type cheats on followed by ch_destroy
7. The Cruiser should be destroyed (Support ships left behind), note that the Popup is still open
8. Save the game
9. Load the game
10. Notice the UI is broken, if you try to open the planet ui a lot of the UI elements are missing
11. Open the Console to see the following error

Script Exception: Null pointer access
 D:\Star Ruler 2\scripts\toolkit\elements\GuiBlueprint.as
  elements.GuiBlueprint::void GuiBlueprint::display(Ship@) | Line 167 | Col 3
  overlays.ShipPopup::void ShipPopup::set(Object@) | Line 118 | Col 3
  tabs.GalaxyTab::Popup@ GalaxyTab::pinObject(Object@, bool = false) | Line 432 | Col 3
  tabs.GalaxyTab::void GalaxyTab::load(SaveFile&inout) | Line 274 | Col 5
  tabs.tabbar::void load(SaveFile&inout) | Line 945 | Col 4

Fun fact - if you try to quit the game to Desktop it now crashes instead.

If you close the Popup manually after Step 7 then the savegame corruption does not occur (shown in the Video, no output in console and the UI is not broken)

Solution(s)

1. Ensure that, upon destruction of an object, all Popups that reference said object are removed
   • most likely the "easiest" solution
2. Try to guard all locations where this object is accessed to prevent the issue
   • sounds dirty and hard to maintain, but if the above is not feasible

Solution - Considerations

Solution 1st might get complex depending on how "Popup" saving is done in Multiplayer, because we don't want a client to trigger saving a "ghost" popup (that is gone on the server), which is why I thought about the 2nd Solutioin (which doesn't strike me as particulary good either)

I still don't quite understand why this causes an issue though, because in the GalaxyTab.as there is a check if the object is not null before the Popup is created... Or maybe that just checks that a object exists in the savegame and not if the object is valid in the current gamestate (aka object still exists).

https://github.com/OpenSRProject/OpenStarRuler-Modpack/blob/master/OpenSR/scripts/gui/tabs/GalaxyTab.as#L278

In which case we'd have a 3rd Solution, check if the pointer (uuid or whatever is available) is still existing before applying the Popup.

But then again that would just be a workaround because the Popup of a destroyed object should not be saved anyway.

So a 4th Solution is to check on saving if the object is still in the game and if not, discard saving that popup. While nicer than guarding in the load (to me at least) method this is not really good for players (why is there a popup of an object that is gone? but then again if the popup simply vanishes (Solution 1) that might also confuse players).

[EngineOfDarkness]

Info, I only reproduced this in vanilla at first and just tried to reproduce it in OpenStarRuler-Modpack and Colonisation Expansion

Vanilla and Colonisation Expansion show the same error, however the OpenStarRuler-Modpack I downloaded yesterday crashes on reload of a savegame with a different error completely unrelated to this (even if I don't use the Popups).

Funnily enough if I do cause the Popup issue then OpenStarRuler-Modpack doesn't crash and just gets stuck on the loading screen forever it seems (however the error messages at this point are completely different in the log, which may point to what I said earlier about "placing" guard conditions not being a good idea afterall)

Edit: After DaloLorn fixed #9 OpenStarRuler-Modpack now behaves just like vanilla and Colonisation Expansion

[DaloLorn]

I believe object validity is indeed the problem. Adding a check for obj.valid (or rather, obj !is null && obj.valid) should suffice to prevent the save corruption.

Unfortunately, because the game serializes the popup count before starting to iterate on the popups, it will be harder to completely prevent serialization of an invalid popup. However, there are a few possible workarounds:

• Serialize a Boolean value (write0, write1, readBit) indicating the validity of the object (and, by extension, the popup). If it is false, abort (de)serialization and continue to the next popup.
• Serialize the object handle, but if it's an invalid object, abort (de)serialization and continue to the next popup. This may be preferable, since it does not increase the save size of valid popups, which are still expected to be the more common case.

If neither of these workarounds are used, the optimal solution is to do nothing on serialization, but change line 273 to prevent the creation of invalid popups. (This would be roughly analogous to Solution 3.)

I generally agree with your complaints about Solutions 1 and 2 - the former may be confusing to users, while the latter will be prohibitively complicated and require alterations to a variety of core GUI elements that are not currently overridden by the MP.

[Skeletonxf]

Serialize a Boolean value (write0, write1, readBit) indicating the validity of the object (and, by extension, the popup). If it is false, abort (de)serialization and continue to the next popup.

I've seen this approach in a few places in the base game code so I'd recommend going with this one

[DaloLorn]

There are some contextual differences between this and the other instances I can remember, so... could go either way.

[EngineOfDarkness]

I just saw this in the method (right at the end) void tick(double time)

That reads almost like popups of destroyed objects should be removed? Not sure what "parent" refers to in this context though

//Update other popups
        for(uint i = 0, cnt = popups.length; i < cnt; ++i) {
            if(popups[i].parent is null) {
                popups.removeAt(i);
                --i; --cnt;
            }
            else {
                popups[i].update();
            }
        }
        popups.sortDesc();

https://github.com/OpenSRProject/OpenStarRuler-Modpack/blob/master/OpenSR/scripts/gui/tabs/GalaxyTab.as#L667

It's probably best to debug this beforehand. Perhahps there's a mundane bug somewhere and this can be fixed without saving additional properties.

If that fails I'd probably just write a validation method that gets called in the save method before actually saving any popup related properties

basically something like

    void save(SaveFile& file) {
        file << cam;

                cleanupInvalidPopups()
        uint cnt = popups.length;
        file << cnt;
        for(uint i = 0; i < cnt; ++i) {
            Object@ obj = popups[i].get();
            file << obj;
            vec2i pos = popups[i].position;
            file << pos;
            bool isFloating = popups[i].objLinked;
            file << isFloating;
        }

        file << quickbar;
    }

    void cleanupInvalidPopups() {
        // here be dragons
    }

Since saving only happens sporadically, if saving takes a few ms (if even that) more, it probably won't matter. I doubt anyone will have more than a few dozen popups anyway. I was probably an outlier for using it as excessively...

Simply because they tank performance like crazy if you have more than a few dozen. Not sure if that's just FPS related or if updating / tracking all those objects causes tick issues.