Equipment that takes cargo space unavailable after reload when all cargo space is in Large Cargo Bay

[dybal]

The setup:

I got a GalCop Viper as an usable derelict, saved it in HyperSpaceHangar and did a shipdata-override to add the role player to it... defined extra_cargo property in that shipdata-override so it doesn't use the default because I think the default, 15TCs, is too much for a ship with around 33t of mass. Created an entry in shipyard.plist for the role, and added EQ_CARGO_BAY to the optional equipment.

With all that done, I restarted Oolite, loaded the savefile and used HyperSpaceHangar to switch the player ship to the GalCop Viper. F5F5 showed it with 0t cargo space, as it should. I then bought the Large Cargo Bay, and F5F5 showed 9t of cargo space, the value I defined in extra_cargo, around 25% of ship's mass. Saved the game, then reloaded the savefile and F5F5 still showed 9t of cargo space - everything fine so far.

(That's my setup, but I guess it could be replicated with any ship by overriding its shipdata.plist with max_cargo=0 and extra_cargo=n, n>=2)

The bug:

Then I bought Self-Repair System (from oolite.oxp.Thargoid.RepairBots), which uses 2t of cargo space. F5 listed Self-Repair System in the ship's equipment, and F5F5 listed the remaining repair system charges as 10 - again as it should.

Then I saved the game and reloaded the savefile... and now F5 doesn't show Self-Repair System in the ship's equipment list, F5F5 has no line with the remaining repair system charges, and when I look at the Latest.log file there isn't a line from LogEvents OXP saying the ship got EQ_REPAIRBOTS_CONTROLLER after the list of worldscripts... but the savefile has EQ_REPAIRBOTS_CONTROLLER in the ship's extra equipment dict.

I'm guessing the equipment award that generates those LogEvent lines just after the worldscripts list is done by the core game... if so, the core game is discarding the EQ_REPAIRBOTS_CONTROLLER equipment... does the core game savefile loading deal with equipments that increase cargo space before dealing with the equipment that decreases it? If it tried to award EQ_REPAIRBOTS_CONTROLLER before EQ_CARGO_BAY was awarded there would not be the 2TCs of cargo space that it needs available.

EDIT: I forgot to say, that was with Trunk: 1.89.0.7222-200430-3b91106

[dybal]

A brief update: I changed my shipdata-overrides to have max_cargo=2 and extra_cargo=7 and now, after buying Larga Cargo Bay and Self-Repair System and saving, all is well upon reload.

[dybal]

I looked at the code, and if I understand it correctly (I had never heard of Objective-C before discovering Oolite), addEquipmentFromCollection (in PlayerEntity.m) loops through the equipment dictionary and calls addEquipmentItem to add each equipment, and that in turn call a function in ShipEntity.m that keeps track of the cargo space used by equipments and rejects the to add the equipment if there is no room, and addEquipmentFromCollection doesn't look at addEquipmentItem return value to see if the equipment addition was successful.

Perhaps addEquipmentFromCollection could keep track of the equipments that could not be added and try adding them again after cycling through the equipment dictionary? That's what ShipStorageHelper does...

[dybal]

I did some debugging... this is the sequence:

- PlayerEntity's setCommanderDataFromDictionary loads the playerShip data from the shipdata.plist definition using the dataKey recovered from the savefile;
- PlayerEntity's setCommanderDataFromDictionary loads the equipment list from the savefile and calls (indirectly) addEquipmentItem from ShipEntity.m to add each equipment;
- ShipEntity's addEquipmentItem tests keeps track of the space used by the installed equipments and if the equipment isn't a Passenger Berth and adding the equipment would make it over the original shipdata.plist max_cargo value, doesn't add the equipment;
- ShipEntity's addEquipmentItem, just before adding the equipment, adds extra_cargo to max_cargo if the equipment is Large Cargo Bay and the ship isn't the player ship;
- after some other setup setCommanderDataFromDictionary discards the current max_cargo (result of the shipdata.plist max_cargo minus the space taken by equipments other than Passenger Berths), recovers max_cargo from the savefile and if it's greater than the original value from shipdat.plist, adds Large Cargo Bay and then subtracts the Passenger Berths space.

This looks like a similar bug I found in Ship Storage Helper...

In my case, Large Cargo Bay is processed before Self-Repair System, but since space from the Large Cargo Bay is added in this phase to max_cargo only for NPCs, when it's Self-Repair System turn it's rejected because max_cargo is still 0. Then latter when the correct value of max_cargo is recovered from the savefile (and that value came to be by adding the extra_cargo to the original shipdata value when I bought the Large Cargo Bay, then subtracting from it the space taken by Self-Repair System when I bought it), the rejected equipments are forgotten and there is no second try to add them.

This is what I propose:

- add extra_cargo to max_cargo in ShipEntity's addEquipmentItem when adding Large Cargo Bay for all ships;
- don't make an exception for Passenger Berth in ShipEntity's addEquipmentItem... if the equipmet takes cargo space, subtract it;
- store the rejected equipments in Pass 1 of PlayerEntity's addEquipmentFromCollection, and add a second loop in its Pass 1 to retry adding those equipments;
- make sure any max_cargo is adjusted if necessary for any equipment removed on PlayerEntity's addEquipmentFromCollection validation pass;
- where PlayerEntity's setCommanderDataFromDictionary now recovers max_cargo from the savefile and adjusts it (around line 1473), do a sanity check but don't modify the current max_cargo obtained from the shipdata.plist + installed equipments modifications;
- look for the code that adds cargo space when the user buys Large Cargo Bay... if it calls ShipEntity's addEquipmentItem it would not have to adjust max_cargo.

Does anybody see a problem with that?

If not, how is it done? I guess I should create a branch, implement and test the change, then create a PR?

[another_commander]

If not, how is it done? I guess I should create a branch, implement and test the change, then create a PR?

Thanks for the analysis. Be aware that you are looking at some of the trickiest code in the entire game - tough start you've made for entering the dark side . Changes in this part, even small and seemingly insignificant ones, have potential to break things in places you'd least expect them to.

Yes, what you have written in the quoted part is exactly how you should proceed. Make sure to test, test and then test again some more when you are sure that it all works. Try to play for a few days with any changes you decide to make incorporated in your own binary and look out for any issues that may pop up. Important: - Make sure you play without OXPs (or with a minimal case one, just for the purposes of this testing) to make sure that you are exercising core game code only.

[dybal]

If not, how is it done? I guess I should create a branch, implement and test the change, then create a PR?

Thanks for the analysis. Be aware that you are looking at some of the trickiest code in the entire game - tough start you've made for entering the dark side . Changes in this part, even small and seemingly insignificant ones, have potential to break things in places you'd least expect them to.

Yes, what you have written in the quoted part is exactly how you should proceed. Make sure to test, test and then test again some more when you are sure that it all works. Try to play for a few days with any changes you decide to make incorporated in your own binary and look out for any issues that may pop up. Important: - Make sure you play without OXPs (or with a minimal case one, just for the purposes of this testing) to make sure that you are exercising core game code only.

OK, will do, minimal OXP use... have to use some though after being confident core-game is trouble-free, since it was OXP created space-taking equipments that triggered the bug... my concern is Ship Config OXP, mainly because I haven't used it (yet, I'm saving it for when I the all-the-kit-you-can-get game gets boring) and don't know exactly how it discovers and alters available space (if it does...).

I already play for at least a week with any OXP mods I do before proposing them to the mantainer... this one I will play with a bit longer (but perhaps not as long as I've been playing with HyperCargo next version... )

I will look for the code that deals with buying and removing player ship equipment to make sure everything is consistent and track down how playerShip.cargoSpaceAvailable is "mantained" to make sure it has the correct value.

Right now, EQ_CARGO_BAY and EQ_PASSENGER_CARGO_BAY both have damage_probability = 0 in equipment.plist... can I assume that will keep?

[another_commander]

Right now, EQ_CARGO_BAY and EQ_PASSENGER_CARGO_BAY both have damage_probability = 0 in equipment.plist... can I assume that will keep?

Yes, this is deliberate and should remain unchanged.