Print

Please login or register.
1 Hour 1 Day 1 Week 1 Month Forever
Login with username, password and session length

[Jaghaimo]

Edit 4: Updated title since I believe to know what's happening

Edit 3: Build with the above code: https://github.com/jaghaimo/starpocalypse/releases/tag/2.2.0-rc1

Edit 2: Digging through the save, it's the submarketTradeData node that holds the reference to submarket that has been removed. Perhaps I could somehow regenerate it after swapping submarkets around?

Edit 1: Submarket swapper: https://github.com/jaghaimo/starpocalypse/pull/22/files#diff-aa90a795e07d25699120caedfe9c9ab3403b21db18e629ebfd334924f746b698R10

To be reproduced with Starpocalypse mod.

As Starpocalypse is meant to be true utility mod, I make sure all changes are made in a transient way. One change coming to my mod is to use custom submarket plugins. To make those transient, I replace vanilla submarkets with my own submarkets (which use my custom plugins, that extend vanilla one). Before game save, I replace all custom submarkets with their vanilla equivalents. This operation succeeds, as can be seen in the logs below (double save, first one finds submarkets to revert, second one does not):

Code

/* opened Jangala */
982700 [Thread-3] INFO  starpocalypse.submarket.SubmarketSwapper  - Swapping open_market with regulated_open_market
982701 [Thread-3] INFO  starpocalypse.submarket.SubmarketSwapper  - Swapping generic_military with regulated_generic_military
982701 [Thread-3] INFO  starpocalypse.submarket.SubmarketSwapper  - Swapping black_market with regulated_black_market
986143 [Thread-3] INFO  starpocalypse.submarket.RegulatedOpenMarket  - Removing Marines
/* 1st save - Jangala gets swapped back BUT regulated is present on it in the save (with ref pointing back to Jangala) */
990346 [Thread-3] INFO  starpocalypse.StarpocalypseMod  - Disabling submarket swapper
990346 [Thread-3] INFO  starpocalypse.submarket.SubmarketSwapper  - Swapping regulated_open_market with open_market
990346 [Thread-3] INFO  starpocalypse.submarket.SubmarketSwapper  - Swapping regulated_generic_military with generic_military
990346 [Thread-3] INFO  starpocalypse.submarket.SubmarketSwapper  - Swapping regulated_black_market with black_market
990347 [Thread-3] INFO  com.fs.starfarer.campaign.save.CampaignGameManager  - Saving to ../saves/save_BradleySerpentis_8538074428255052117...
990349 [Thread-3] INFO  com.fs.starfarer.campaign.save.CampaignGameManager  - Saving stage 2
990349 [Thread-3] INFO  com.fs.starfarer.campaign.save.CampaignGameManager  - Saving stage 3
991648 [Thread-3] INFO  com.fs.starfarer.campaign.save.CampaignGameManager  - Saving stage 4
991648 [Thread-3] INFO  com.fs.starfarer.campaign.save.CampaignGameManager  - Saving stage 6
992057 [Thread-3] INFO  com.fs.starfarer.campaign.save.CampaignGameManager  - Saving stage 6B
992281 [Thread-3] INFO  com.fs.starfarer.campaign.save.CampaignGameManager  - Renaming [campaign.xml] to [campaign.xml.bak]
992281 [Thread-3] INFO  com.fs.starfarer.campaign.save.CampaignGameManager  - Renaming [descriptor.xml] to [descriptor.xml.bak]
992282 [Thread-3] INFO  com.fs.starfarer.campaign.save.CampaignGameManager  - Renaming [campaign.xml.inprogress] to [campaign.xml]
992283 [Thread-3] INFO  com.fs.starfarer.campaign.save.CampaignGameManager  - Renaming [descriptor.xml.inprogress] to [descriptor.xml]
992283 [Thread-3] INFO  com.fs.starfarer.campaign.save.CampaignGameManager  - Saving stage 7
992283 [Thread-3] INFO  com.fs.starfarer.campaign.save.CampaignGameManager  - Saving stage 8
992283 [Thread-3] INFO  com.fs.starfarer.campaign.save.CampaignGameManager  - Finished saving
992316 [Thread-3] INFO  starpocalypse.StarpocalypseMod  - Re-enabling submarket swapper
Time quicksaving: -2,531 ms
/* 2nd save - nothing to replace, AND there's no regulated submarket in the save */
1315962 [Thread-3] INFO  starpocalypse.StarpocalypseMod  - Disabling submarket swapper
1315963 [Thread-3] INFO  com.fs.starfarer.campaign.save.CampaignGameManager  - Saving to ../saves/save_BradleySerpentis_8538074428255052117...
1315965 [Thread-3] INFO  com.fs.starfarer.campaign.save.CampaignGameManager  - Saving stage 2
1315965 [Thread-3] INFO  com.fs.starfarer.campaign.save.CampaignGameManager  - Saving stage 3
1317350 [Thread-3] INFO  com.fs.starfarer.campaign.save.CampaignGameManager  - Saving stage 4
1317351 [Thread-3] INFO  com.fs.starfarer.campaign.save.CampaignGameManager  - Saving stage 6
1317653 [Thread-3] INFO  com.fs.starfarer.campaign.save.CampaignGameManager  - Saving stage 6B
1317884 [Thread-3] INFO  com.fs.starfarer.campaign.save.CampaignGameManager  - Renaming [campaign.xml] to [campaign.xml.bak]
1317885 [Thread-3] INFO  com.fs.starfarer.campaign.save.CampaignGameManager  - Renaming [descriptor.xml] to [descriptor.xml.bak]
1317886 [Thread-3] INFO  com.fs.starfarer.campaign.save.CampaignGameManager  - Renaming [campaign.xml.inprogress] to [campaign.xml]
1317886 [Thread-3] INFO  com.fs.starfarer.campaign.save.CampaignGameManager  - Renaming [descriptor.xml.inprogress] to [descriptor.xml]
1317887 [Thread-3] INFO  com.fs.starfarer.campaign.save.CampaignGameManager  - Saving stage 7
1317887 [Thread-3] INFO  com.fs.starfarer.campaign.save.CampaignGameManager  - Saving stage 8
1317887 [Thread-3] INFO  com.fs.starfarer.campaign.save.CampaignGameManager  - Finished saving
1317925 [Thread-3] INFO  starpocalypse.StarpocalypseMod  - Re-enabling submarket swapper
Time quicksaving: -2,550 ms

Unfortunately, the save still have the "regulated_" submarkets! I need to save once more to fully remove it. Even though those regulated_ are attached to Jangala they are not used...

Having written so much, I THINK I am missing some other reference that holds submarket or submarket plugin, transactions or some interaction (which gets dereferenced after save). Found the culprit - submarketTradeData.

[Alex]

... let me add a PlayerTradeDataForSubmarket.setSubmarket(); if I'm reading this right, that should let you do whatever you need to do to avoid this?

[Jaghaimo]

Congrats on 20000 posts!

I don't think this is right or needed*. This is a bug in my opinion. Let's pause the game. Hit save, got file A. Hit save again - since nothing has changed game-wise (pause state), I should get file A again. But I get a different file**

* or I might be missing something; I'd probably need `purgeForSubmarket()` if anything.
** the bug being here: the PlayerTradeData gets cleaned from non-existing submarkets on a second save.


After pointing PlayerTradeDataForSubmarket out, I now belive I just need to call `PlayerActivityTracker`s `advance() ` so the offending submarket is removed:

Code

		List<SubmarketAPI> remove = new ArrayList<SubmarketAPI>();
		for (PlayerTradeDataForSubmarket data : submarketTradeData.values()) {
			data.advance(days);
			
			MarketAPI market = Global.getSector().getEconomy().getMarket(data.getMarket().getId());
			if (market == null || market.getSubmarket(data.getSubmarket().getSpecId()) == null ||
					market.getSubmarket(data.getSubmarket().getSpecId()) != data.getSubmarket()) {
				remove.add(data.getSubmarket());
			}
		}

Perhaps the "bug" is the lack of automatic call of such after all plugins' `beforeSave` was processed?

Edit: Indeed, this did the trick!

Code

    @Override
    public void beforeGameSave() {
        log.info("Replacing regulated submarkets with vanilla variants");
        SubmarketSwapper.uninstall();// this was the hero I got
        SharedData.getData().getPlayerActivityTracker().advance(0);// this is the hero we needed
    }

[Alex]

I don't think this is a bug. It's more, you're doing some tricky stuff and it doesn't play nice with some of the game's expectations.

I mean, I understand that it seems like "saving twice should produce the same file" but there's nothing that really guarantees that, and it's pretty trivial to write a writeReplace() method that would make this not be the case, so while this is an understandable expectation, I don't think it's a valid one.

As for why this might be happening: PlayerActivityTracker.advance() cleans up "orphaned" data. And, actually - without looking into this too much, so I might be mistaken - it seems like this is going to pose a problem to your approach, right? Since  it seems like valid data about what the player's done is getting cleaned out.

[Jaghaimo]

Indeed, you are correct regarding save expectations, I totally forgot about writeReplace() method.

Orphaning it is the intention, as I want to be able to disable the mod at any time. What I will need it to transfer back and forth PlayerTradeDataForSubmarket between vanilla market and my custom market, and for that additional methods will be needed.

In PlayerActivityTracker there is:

Code

	public PlayerTradeDataForSubmarket getPlayerTradeData(SubmarketAPI submarket) {
		PlayerTradeDataForSubmarket data = submarketTradeData.get(submarket);
		if (data == null) {
			data = new PlayerTradeDataForSubmarket(submarket);
			submarketTradeData.put(submarket, data);
		}
		return data;
	}

So I could use PlayerTradeDataForSubmarket returned and modify in-place. Which is a pain, and also incomplete (setter wise).

Just adding PlayerTradeDataForSubmarket.setSubmarket() will still cause the orphan to be discarded since it is stored as a map Map<SubmarketAPI, PlayerTradeDataForSubmarket> in PlayerActivityTracker and the check compares map submarket with player trade data's submarket.

So the only thing that would help me would be addition of said `setSubmarket` as well as addition to PlayerActivityTracker:

Code

	public void transferPlayerTradeData(SubmarketAPI oldSubmarket, SubmarketAPI newSubmarket) {
		PlayerTradeDataForSubmarket data = submarketTradeData.get(oldSubmarket);
		if (data == null) {
			data = new PlayerTradeDataForSubmarket(oldSubmarket);
		}
                data.setSubmarket(newSubmarket);
		submarketTradeData.put(newSubmarket, data);
	}

What is PlayerActivityTracker used for exactly? As in, what am I loosing by not persisting it between saves?

[Alex]

Let me add PlayerActivityTracker.getSubmarketTradeData() - that seems like that should do it, right?

What is PlayerActivityTracker used for exactly? As in, what am I loosing by not persisting it between saves?

Trade-related reputation changes, smuggling suspicion, and probably some other stuff. It's only used in starfarer.api so doing a "find occurrences" or whatever your specific IDE calls it should show you the complete picture of how it's used.

[Jaghaimo]

Yes, PlayerActivityTracker.getSubmarketTradeData() [which will return the map, not ideal since it will expose the implementation] along with PlayerTradeDataForSubmarket.setSubmarket(SubmarketAPI) will sort it out.

But perhaps cleaner would be PlayerActivityTracker.setSubmarketTradeData(SubmarketAPI, PlayerTradeDataForSubmarket) and PlayerTradeDataForSubmarket.setSubmarket(SubmarketAPI) instead? That way I can:

Code

PlayerActivityTracker tracker;
PlayerTradeDataForSubmarket oldData = tracker.getSubmarketTradeData(oldSubmarket);
oldData.setSubmarket(newSubmarket);/* existing map still holds old submarket -> old data which now points to wrong submarket */
tracker.setSubmarketTradeData(newSubmarket, oldData);/* existing map holds above but also new submarket and old-new data */
tracker.advance(0);/* old submarket data is pruned (offending key and its data removed) */

[Alex]

Ok, cool! I think I'll just leave it at the getter - marginally simpler on my end (I mean, we're talking a minute or two, but still) - and "hiding the implementation" seems like a bit of a fig leaf anyway, given the kind of surgery you're doing on the underlying data

[Jaghaimo]

... let me add a PlayerTradeDataForSubmarket.setSubmarket(); if I'm reading this right, that should let you do whatever you need to do to avoid this?

Sounds good. Just to confirm, I also need the initially mentioned setSubmarket() to reassign actual PlayerTradeDataForSubmarket (since PlayerActivityTracker.advance() checks both map's submarket and data's submarket for validity).

[Alex]

Just to confirm, I also need the initially mentioned setSubmarket() to reassign actual PlayerTradeDataForSubmarket (since PlayerActivityTracker.advance() checks both map's submarket and data's submarket for validity).

Yep, added that one too!