Fix undo corruption when merged house becomes vacant

[csfreitas]

Hi Bianca, one thing I should have made clearer: this builds on Crudelios's earlier fix, which you linked in #775. It adds the same Undo protection for the case where a merged 2x2 house becomes four vacant lots.

You were right about the ID protection. The problem I reproduced was Undo bringing back the old house's map data after it had split. Later deletion and ID reuse then linked the old tile to a distant building.

I've removed the artificial test and corrected the description and code comment. The steps and saves are in my previous comment.

The fix prevented this sequence in my master test, but I can't confirm it covers every case of #775 or the reporter's exact sequence. It won't repair an already corrupted save.

[crudelios]

Good catch! I did disable undo on house evolution merge/split, but missed this one.

[bvschaik]

Could you detail the steps required to reproduce the bug so I can check whether the fix works?

I tried to reproduce but I could not. I built some statues (to be deleted later), then some houses. I caused the people to emigrate with high taxes and 0 wages. I deleted the statues just before a 2x2 merged house got vacated. The 3 vacant lots chose building ids outside the deleted statue range due to this bit of code in src/building/building.c:

julius/src/building/building.c

Lines 54 to 62 in 3c7d865

	building *building_create(building_type type, int x, int y)
	{
	building *b = 0;
	for (int i = 1; i < MAX_BUILDINGS; i++) {
	if (all_buildings[i].state == BUILDING_STATE_UNUSED && !game_undo_contains_building(i)) {
	b = &all_buildings[i];
	break;
	}
	}

The code for creating a building explicitly checks the undo list, so they are not reused.

Also, please don't bother with writing a test for this - the tests are only for integration testing the game logic, not these kind of bugs.

[csfreitas]

Thanks for checking this, Bianca. You are right about the building-ID protection, and I need to correct my earlier explanation.

My original diagnostic forced the house to become vacant between game_undo_start_build() and game_undo_finish_build(). During that interval, undo is not ready, so game_undo_contains_building() returns false. However, normal gameplay does not advance the simulation while building_construction_in_progress() is true. The diagnostic bypassed that restriction, so it was not a valid gameplay reproduction.

I then ran a separate local, headless diagnostic without this PR's fix, using the normal construction APIs and game_run(), with maximum taxes and zero wages. Across three existing saves, I observed 38 merged-to-vacant transitions. Four occurred while undo was available. In all four, the new lots avoided the protected statue ID, and undo restored the statue without duplicate map references to that ID. This was not a manual GUI reproduction and does not rule out other causes of #775.

The test in this PR only checks that undo gets disabled; it does not demonstrate the reported corruption. Understood about the integration suite: I will not pursue adding this diagnostic there.

I have moved this PR to draft. I do not currently have valid reproduction steps for the proposed cause, and the existing fix should not be treated as established. Sorry for presenting the earlier result too confidently, and thank you for catching this. The next step is to identify a sequence reachable through normal gameplay before proposing a correction.

[csfreitas]

Hi Bianca,

You were right about building_create() checking the undo list. It isn't reusing IDs while they're protected by Undo, as I originally thought.

This took me quite a while to pin down. I kept hitting cases where Undo was correctly disabled, so demolishing something and waiting for a house to downgrade wasn't enough. The case I eventually reproduced was a merged 2×2 house becoming four vacant lots while Undo was still available.

I prepared a test save just before that happens. I reproduced the problem on Julius 1.7 and master (5992838), then repeated it on the same master with the proposed guard. The master comparison was on Windows x64 with SDL 2.32.8.

The sequence is:

1. Load 01-start.sav and pause immediately. Demolish the distant small villa, ID 37, at (70,38).
2. Resume until merged house 41 at (71,88) becomes vacant, then pause and use Undo.
3. Demolish the adjacent theater and lots 41 and 476. Let those removals finish, then pause again.
4. Place a medium statue at (70,88), demolish lot 477 at (72,89), and place a vacant house at (59,71), all while paused.
5. Resume without further construction or Undo. In this run, the corruption was visible by May.

The important part happens at step 2. Undo restores the villa, but it also restores the old merged-house image and tile metadata. The house itself isn't merged again: its four separate vacant-lot records remain.

The distant link appears later, after deletion and normal ID reuse. ID 477 became a burning ruin at (32,7), but the old tile at (72,89) still referenced it. Clicking the old location opened information about the distant ruin. The saved file confirms that both locations reference the same ID.

With the guard, Undo becomes unavailable as soon as the merged house becomes vacant. The villa isn't restored, and neither is the stale house-map state. After continuing the remaining steps, 477 was reused at the same ruin location, but this time the old tile was clear and there was only one map reference to that ID.

So the change prevents an Undo operation that is no longer safe; it doesn't repair a save that's already corrupted.

The ZIP includes the starting save, the before/after Undo checkpoints, the corrupted master save, and the fixed-run comparison. The README has the remaining coordinates and checkpoint details. Start from 01 to repeat the sequence: the intermediate saves are for inspection, since loading them doesn't restore the live Undo history.

This reproduces a vacant-lot/burning-ruin link rather than the reporter's exact villa/tent case. The guard prevents the corruption in this sequence, but I can't say it covers every possible cause of #775. Without a save from before the reporter's corruption or the exact steps that led to it, I can't confirm that this is the same path they hit.

julius-775-review-files-20260907.zip

[bvschaik]

Thanks. I tried your reproduction steps and was able to reproduce. I still don't fully understand how exactly it corrupts the data - I tried to reproduce on my own emigration save but failed. However, I think the fix is a good one to have anyway, since it prevents an inconsistent state.

Could you mark the PR as ready so I can merge it? I think we should close the linked issue, since it's highly likely that this inconsistent state lead to the linked houses: all the other cases of houses merging/splitting are covered by disabling undo.

[csfreitas]

Thanks for taking the time to reproduce it! Apparently the planets have to align just right to corrupt this save. I spent quite a while getting Rome into exactly the wrong state 😄

The key seems to be that Undo restores the old map state, but leaves the four separate vacant-lot records in place. Later demolition and ID reuse make that mismatch visible as a link to a distant building. That would explain why simply making a house lose its residents usually isn't enough.

I've marked the PR as ready. Thanks again!

[bvschaik]

Thanks for figuring out how to reproduce in the first place!