diff --git a/src/simutrans/simhalt.cc b/src/simutrans/simhalt.cc index 93f9ac9..3922506 100644 --- a/src/simutrans/simhalt.cc +++ b/src/simutrans/simhalt.cc @@ -2672,27 +2672,39 @@ sint64 haltestelle_t::calc_maintenance() const // changes this to a public transfer exchange stop -void haltestelle_t::change_owner( player_t *player ) +void haltestelle_t::change_owner( player_t *player, player_t *old_owner ) { + // "player" is the new owner (the public player); "old_owner" is the player who + // triggered the action. On a shared halt we must only convert the tiles that + // belong to old_owner, otherwise a co-owner's property would silently become + // public. As a special case the public player may convert every connected tile. + // NOTE: the test must key off old_owner, not player: player is always the public + // player here, so testing player would make the condition always true. + player_t *const public_player = welt->get_public_player(); + const bool convert_all = (old_owner == public_player); + // process every tile of stop slist_tpl joining; for(tile_t const& i : tiles) { grund_t* const gr = i.grund; if( gebaeude_t* gb = gr->find() ) { - // change ownership - player_t *gbplayer =gb->get_owner(); - gb->set_owner(player); - gb->set_flag(obj_t::dirty); - sint64 const monthly_costs = welt->get_settings().maint_building * gb->get_tile()->get_desc()->get_level(); - waytype_t const costs_type = gb->get_waytype(); - player_t::add_maintenance(gbplayer, -monthly_costs, costs_type); - player_t::add_maintenance(player, monthly_costs, costs_type); + player_t *gbplayer = gb->get_owner(); + // only convert buildings owned by the initiating player + if( convert_all || gbplayer == old_owner ) { + // change ownership + gb->set_owner(player); + gb->set_flag(obj_t::dirty); + sint64 const monthly_costs = welt->get_settings().maint_building * gb->get_tile()->get_desc()->get_level(); + waytype_t const costs_type = gb->get_waytype(); + player_t::add_maintenance(gbplayer, -monthly_costs, costs_type); + player_t::add_maintenance(player, monthly_costs, costs_type); - // cost is computed as cst_make_public_months - sint64 const cost = -welt->scale_with_month_length(monthly_costs * welt->get_settings().cst_make_public_months); - player_t::book_construction_costs(gbplayer, cost, get_basis_pos(), costs_type); - player_t::book_construction_costs(player, -cost, koord::invalid, costs_type); + // cost is computed as cst_make_public_months + sint64 const cost = -welt->scale_with_month_length(monthly_costs * welt->get_settings().cst_make_public_months); + player_t::book_construction_costs(gbplayer, cost, get_basis_pos(), costs_type); + player_t::book_construction_costs(player, -cost, koord::invalid, costs_type); + } } // change way ownership @@ -2701,7 +2713,8 @@ void haltestelle_t::change_owner( player_t *player ) if( weg_t *w=gr->get_weg_nr(j) ) { // change ownership of way... player_t *wplayer = w->get_owner(); - if( wplayer!=player) { + // only convert ways owned by the initiating player (and not already public) + if( wplayer!=player && (convert_all || wplayer == old_owner) ) { w->set_owner( player ); w->set_flag(obj_t::dirty); sint64 cost = w->get_desc()->get_maintenance(); @@ -2744,7 +2757,17 @@ void haltestelle_t::change_owner( player_t *player ) } } } - owners = 1 << player->get_player_nr(); + // Recompute the owner mask from the actual tile owners. Only the initiator's + // tiles became public, so any remaining co-owner of a shared halt must be kept + // in the mask (the old code overwrote it with the public player alone). + owners = 0; + for(tile_t const& i : tiles) { + if( gebaeude_t const* gb = i.grund->find() ) { + if( gb->get_owner_nr() != PLAYER_UNOWNED ) { + owners |= 1 << gb->get_owner_nr(); + } + } + } if (permissions & (permissions - 1)) { // multiple permissions set_permissions(permissions); diff --git a/src/simutrans/simhalt.h b/src/simutrans/simhalt.h index 82e8539..a17bb14 100644 --- a/src/simutrans/simhalt.h +++ b/src/simutrans/simhalt.h @@ -430,7 +430,11 @@ public: void merge_halt( halthandle_t halt_to_join ); - void change_owner( player_t *player ); + // Transfer this stop to new_owner (the public player). Only tiles belonging to + // old_owner (the player who initiated the action) are converted, so co-owners of + // a shared halt keep their property. If old_owner is the public player itself, + // every connected tile is converted. + void change_owner( player_t *new_owner, player_t *old_owner ); vector_tpl const& get_pax_connections() const { return all_links[goods_manager_t::INDEX_PAS].connections; } vector_tpl const& get_mail_connections() const { return all_links[goods_manager_t::INDEX_MAIL].connections; } diff --git a/src/simutrans/tool/simtool.cc b/src/simutrans/tool/simtool.cc index ec411ab..dbc86f0 100644 --- a/src/simutrans/tool/simtool.cc +++ b/src/simutrans/tool/simtool.cc @@ -7125,8 +7125,8 @@ const char *tool_make_stop_public_t::work( player_t *player, koord3d p ) } } - // change ownership - halt->change_owner( welt->get_public_player() ); + // change ownership: only the initiating player's tiles become public + halt->change_owner( welt->get_public_player(), player ); halt->merge_halt( merge_to ); return NULL; diff --git a/tests/all_tests.nut b/tests/all_tests.nut index 301e39b..545840c 100644 --- a/tests/all_tests.nut +++ b/tests/all_tests.nut @@ -128,6 +128,7 @@ all_tests <- [ test_halt_build_station_extension, test_halt_upgrade_downgrade, test_halt_make_public_single, + test_halt_make_public_shared, test_halt_make_public_multi_tile, test_halt_make_public_underground, test_halt_move_stop_invalid_param, diff --git a/tests/tests/test_halt.nut b/tests/tests/test_halt.nut index f999e0e..dde105a 100644 --- a/tests/tests/test_halt.nut +++ b/tests/tests/test_halt.nut @@ -1182,6 +1182,63 @@ function test_halt_make_public_single() } +// Regression test for the shared-halt ownership bug (forum topic 23970): +// making a stop public must only convert the initiating player's tiles, never the +// tiles of a co-owner of the same (public) halt. +function test_halt_make_public_shared() +{ + local pl = player_x(0) + local public_pl = player_x(1) + local pax_halt = building_desc_x.get_available_stations(building_desc_x.station, wt_road, good_desc_x.passenger)[0] + local road_desc = way_desc_x.get_available_ways(wt_road, st_flat)[0] + local makepublic = command_x(tool_make_stop_public) + local wayremover = command_x(tool_remove_way) + + // second company that will co-own the halt + world.create_player(2, 1) + local pl2 = player_x(2) + ASSERT_TRUE(pl2.is_valid()) + SET_PLAYER_FUNDS(pl2, 200000 * 100) + + // a public road, so that stations of different players may be built along it + ASSERT_EQUAL(command_x.build_way(public_pl, coord3d(4, 1, 0), coord3d(4, 5, 0), road_desc, true), null) + + // pl0 builds a station and turns the halt public, so other players may join it + ASSERT_EQUAL(command_x.build_station(pl, coord3d(4, 3, 0), pax_halt), null) + ASSERT_EQUAL(makepublic.work(pl, coord3d(4, 3, 0)), null) + + // pl0 and pl2 each add a private station tile that joins the now public halt + ASSERT_EQUAL(command_x.build_station(pl, coord3d(4, 2, 0), pax_halt), null) + ASSERT_EQUAL(command_x.build_station(pl2, coord3d(4, 4, 0), pax_halt), null) + + // verify the setup: one shared halt with three different tile owners + ASSERT_TRUE(halt_x.get_halt(coord3d(4, 3, 0), public_pl) != null) + ASSERT_EQUAL(building_x(4, 3, 0).get_owner().nr, public_pl.nr) + ASSERT_EQUAL(building_x(4, 2, 0).get_owner().nr, pl.nr) + ASSERT_EQUAL(building_x(4, 4, 0).get_owner().nr, pl2.nr) + + // pl0 makes the shared halt public again via its own tile + { + local pl2_maint = pl2.get_current_maintenance() + + ASSERT_EQUAL(makepublic.work(pl, coord3d(4, 2, 0)), null) + + // pl0's own tile is now public ... + ASSERT_EQUAL(building_x(4, 2, 0).get_owner().nr, public_pl.nr) + // ... but pl2's tile must be left untouched (this is the bug being fixed) + ASSERT_EQUAL(building_x(4, 4, 0).get_owner().nr, pl2.nr) + ASSERT_EQUAL(pl2.get_current_maintenance(), pl2_maint) + } + + // cleanup + world.remove_player(pl2) + ASSERT_EQUAL(command_x(tool_remover).work(public_pl, coord3d(4, 2, 0)), null) + ASSERT_EQUAL(command_x(tool_remover).work(public_pl, coord3d(4, 3, 0)), null) + ASSERT_EQUAL(wayremover.work(public_pl, coord3d(4, 1, 0), coord3d(4, 5, 0), "" + wt_road), null) + RESET_ALL_PLAYER_FUNDS() +} + + function test_halt_make_public_multi_tile() { local pl = player_x(0)