src/simutrans/obj/gebaeude.cc:1098: virtual void gebaeude_t::cleanup(player_t*): Assertion `get_stadt()' failed.
Removing the assert is sufficient to fix the symptom — in release builds the code is functionally correct, the assert is just over-strict.
The deeper issue is that the destructor's opt-out (clearing the city link before tearing down each building, to skip per-tile detach work when the whole city is being freed) only works for single-tile buildings; for multi-tile buildings it only clears the first tile, so the remaining tiles still run the detach path. That's wasted work, not broken behavior — but it's exactly the wasted work the opt-out was added to avoid in the first place.
So this patch does both: relaxes the assert, and extends the opt-out to every tile of the building, which is what the original author thought they were already doing.
From 245b993357a164c3a35a98479e8a387486886cce Mon Sep 17 00:00:00 2001
From: Jan Rydzewski <flegmer@gmail.com>
Date: Thu, 30 Apr 2026 10:23:58 +0000
Subject: [PATCH] simcity: fix stadt_t::~stadt_t() multi-tile building handling
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
The destructor used to pop one entry off `buildings`, set its
stadt to NULL, then call hausbauer_t::remove on it. But
add_gebaeude_to_stadt puts ONE entry per TILE into `buildings`
for a multi-tile building. Pre-popping just the head tile then
called hausbauer_t::remove, which cascades through
gebaeude_t::cleanup -> remove_gebaeude_from_stadt and walks every
tile of the building; that walk's `buildings.remove(remove_gb);
assert(ok)` failed when it reached the already-popped slot.
Walk the building's tile list up front: drop the still-listed
sibling tiles from `buildings`, null every tile's stadt, then
call hausbauer_t::remove. Nulling stadt makes the cascade short-
circuit remove_gebaeude_from_stadt entirely, preserving the
intent of the original 2007 set_stadt(NULL) bypass — its
recalc_city_size() is wasted work on a city that's about to
vanish, and per-tile buildings.remove() inside it is O(n) for an
n-building city.
Relax the city-building-head-tile assert in gebaeude_t::cleanup
that 11703 added. The assert assumes any cleanup of a city-
building head tile happens with stadt linked, which is the right
invariant for renovation paths but is over-broad for the
destructor path that intentionally clears stadt up front.
Surfaced by paksets with multi-tile townhalls (pak64's 02_CITY
through 04_CITY, pak128's 2x2 default townhall) once a city's bev
crosses the threshold for check_bau_townhall to upgrade past the
1x1 starter townhall.
New test test_city_remove_with_multitile_townhall builds a city,
grows it via tool_change_city_size until check_bau_townhall picks
a multi-tile townhall, tears down every non-townhall building so
the destructor only iterates townhall and monument/attraction
entries, then removes the townhall — exercising the destructor
path that was previously broken.
---
src/simutrans/obj/gebaeude.cc | 12 ++++---
src/simutrans/world/simcity.cc | 22 +++++++++++--
tests/all_tests.nut | 3 +-
tests/tests/test_city.nut | 59 ++++++++++++++++++++++++++++++++++
4 files changed, 88 insertions(+), 8 deletions(-)
diff --git a/src/simutrans/obj/gebaeude.cc b/src/simutrans/obj/gebaeude.cc
index a061d5d380..31f6771aee 100644
--- a/src/simutrans/obj/gebaeude.cc
+++ b/src/simutrans/obj/gebaeude.cc
@@ -1094,11 +1094,13 @@ void gebaeude_t::cleanup(player_t *player)
player_t::book_construction_costs(player, cost, get_pos().get_2d(), tile->get_desc()->get_finance_waytype());
- if (is_city_building() && tile->get_index() == 0) {
- assert(get_stadt());
- }
-
-
+ // get_stadt() may legitimately be NULL on the stadt_t::~stadt_t()
+ // path: the destructor pre-clears stadt on every tile of each popped
+ // building so the cascade here short-circuits remove_gebaeude_from_stadt
+ // (its recalc_city_size() is wasted work on a city that's vanishing).
+ // Outside that path callers expect cleanup to remove the building
+ // from its stadt; an absent stadt for a city-building head tile
+ // there means the caller forgot.
if (stadt_t *city=get_stadt()) {
city->remove_gebaeude_from_stadt(this);
}
diff --git a/src/simutrans/world/simcity.cc b/src/simutrans/world/simcity.cc
index 288ab7d29c..00974f1136 100644
--- a/src/simutrans/world/simcity.cc
+++ b/src/simutrans/world/simcity.cc
@@ -957,8 +957,26 @@ stadt_t::~stadt_t()
}
}
else {
- gb->set_stadt( NULL );
- hausbauer_t::remove(welt->get_public_player(),gb);
+ // add_gebaeude_to_stadt put one entry per tile of a
+ // multi-tile building into `buildings`; pop_back only
+ // took out the head. Walk the tiles, drop the still-
+ // listed siblings here, and null every tile's stadt
+ // so the hausbauer_t::remove cascade short-circuits
+ // remove_gebaeude_from_stadt — its recalc_city_size()
+ // is wasted work on a city about to vanish, and its
+ // per-tile buildings.remove() would re-find an
+ // already-popped entry and assert.
+ static vector_tpl<grund_t*> gb_tiles;
+ gb->get_tile_list(gb_tiles);
+ for (grund_t* gr : gb_tiles) {
+ if (gebaeude_t* part = gr->find<gebaeude_t>()) {
+ if (part != gb) {
+ buildings.remove(part);
+ }
+ part->set_stadt(NULL);
+ }
+ }
+ hausbauer_t::remove(welt->get_public_player(), gb);
}
}
// avoid the bookkeeping if world gets destroyed
diff --git a/tests/all_tests.nut b/tests/all_tests.nut
index 0a7f404a9a..e302603761 100644
--- a/tests/all_tests.nut
+++ b/tests/all_tests.nut
@@ -57,7 +57,8 @@ all_tests <- [
test_city_add_on_existing_townhall,
test_city_add_near_map_border,
test_city_change_size_invalid_params,
- test_city_change_size_to_minimum
+ test_city_change_size_to_minimum,
+ test_city_remove_with_multitile_townhall,
test_climate_invalid,
test_climate_flat,
test_climate_cliff,
diff --git a/tests/tests/test_city.nut b/tests/tests/test_city.nut
index 2f16481238..85a404fc3b 100644
--- a/tests/tests/test_city.nut
+++ b/tests/tests/test_city.nut
@@ -145,6 +145,65 @@ function test_city_change_size_invalid_params()
}
+// Exercises the stadt_t::~stadt_t() multi-tile-building path: build a
+// city, grow it until check_bau_townhall picks a multi-tile townhall
+// (pak64 lands on 04_CITY 2x2 around bev=20000), tear down every
+// non-townhall building first to keep the destructor's iteration
+// focused on the multi-tile townhall, then remove the townhall.
+// Without the destructor fix in this commit, the cascade through
+// gebaeude_t::cleanup -> stadt_t::remove_gebaeude_from_stadt asserts
+// when it walks back to the already-popped head tile.
+function test_city_remove_with_multitile_townhall()
+{
+ local pl = player_x(1)
+ local townhall_pos = coord3d(8, 8, 0)
+
+ ASSERT_EQUAL(command_x(tool_add_city).work(pl, townhall_pos, "0"), null)
+ ASSERT_EQUAL(command_x(tool_change_city_size).work(player_x(0), townhall_pos, "20000"), null)
+
+ // Tear down every non-townhall building. Capture the townhall's
+ // position on the way through.
+ local size = world.get_size()
+ local th_corner = null
+ for (local x = 0; x < size.x; x++) {
+ for (local y = 0; y < size.y; y++) {
+ local b = tile_x(x, y, 0).find_object(mo_building)
+ if (b == null) continue
+ if (b.is_townhall()) {
+ if (th_corner == null) th_corner = coord3d(x, y, 0)
+ continue
+ }
+ command_x(tool_remover).work(pl, coord3d(x, y, 0))
+ }
+ }
+
+ // Fail loudly if the pakset never upgraded the townhall to multi-
+ // tile; otherwise the test would silently pass without exercising
+ // the destructor path we care about.
+ local th_size = tile_x(th_corner.x, th_corner.y, 0).find_object(mo_building).get_desc().get_size(0)
+ ASSERT_TRUE(th_size.x > 1 || th_size.y > 1)
+
+ ASSERT_EQUAL(command_x(tool_remover).work(pl, th_corner), null)
+
+ // ~stadt_t() tears down everything in stadt::buildings (the
+ // townhall, plus monuments/attractions added via
+ // add_gebaeude_to_stadt) but not the auto-generated road network;
+ // sweep what's left so RESET_ALL_PLAYER_FUNDS sees zero
+ // maintenance.
+ for (local x = 0; x < size.x; x++) {
+ for (local y = 0; y < size.y; y++) {
+ local tile = tile_x(x, y, 0)
+ if (tile.get_way(wt_road) != null) {
+ local p = coord3d(x, y, 0)
+ command_x(tool_remove_way).work(pl, p, p, "" + wt_road)
+ }
+ }
+ }
+
+ RESET_ALL_PLAYER_FUNDS()
+}
+
+
function test_city_change_size_to_minimum()
{
ASSERT_EQUAL(command_x(tool_add_city).work(player_x(1), coord3d(1, 1, 0)), null)
yes, i did this using AI and I am half proud of it. The not-proud part is that my python brain can't fully wrap around it.
cheers!
Your post contains an email address. You might want to delete that to avoid spam.
Also, I've no idea about the specifics of town hall removal, but the code seems to contains a call to an irrelevant RESET_ALL_PLAYER_FUNDS() function, which would be concerning.... but for the fact that my IDE says no such function exists, nor is it mentioned in the Doxygen or Squirrel-API documentation. Maybe the AI is actually doing something too clever for me to understand, but it looks like at least that line could be a hallucination....
Thanks for caring! But I cant resist collecting the fame after the patch gets merged (if it does). How could all the fans reach me if i don't provide an email? :P
RESET_ALL_PLAYER_FUNDS is in automated tests code and it is already used in the tests/tests/test_city.nut
Thank you. The assert was only asking for the first tile, not all the other tiles. I had put it there to debug the city growth with multitile buildings.
However, the C code is dangerous. The building list contains tiles not buildings! So a multitle building is contained 4 times in the list. Removing it only once would not do, the next attempt at deletion would access a deleted multitle building tile like common to pak128.german for instance. The current code however had another error, a headquarter could belong to two cities afterwards. So I put in the very old code with deleting nothing and let hausbauer_t handle everything.
The general squirrel test code only works with pak64 in English but there are no multitile buildings (apart from the headquarter and the townhall). Hence, in pak64 one can easily find out if it is the multitile town hall from its name. Also, it is build as soon as 1500 citizens are reached and the climate is right. However, due to space constraints, sometimes the townhall replacement can fail.
Quote from: janry on Yesterday at 09:27:42 PMThanks for caring! But I cant resist collecting the fame after the patch gets merged (if it does). How could all the fans reach me if i don't provide an email?
SVN does not record patch addresses, that is a very git thing anyway ...
Submitted a revised vversion in r11943.