From 840f9aa9a0bbd623d4bfe46f17ec7ba5574dfe16 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 9 Sep 2026 12:57:28 +0200 Subject: [PATCH] #224: refuse to build a structure on top of another BuildingField.place() dropped a structure at the fleet centroid with no overlap check of any kind, so they could be stacked. It reads worse than it looks. BuildingField decides which structure the fleet is standing on by testing the centroid against interact_radius and returning the FIRST match, so two structures inside one another make that a question of list order rather than of where the player flew - you could fly to your workshop and open the crafter. The separation is interact_radius itself (150) rather than a new number, because that is exactly the circle the ambiguity comes from. Buildings draw to a radius of roughly 60, so this leaves a visible gap rather than letting them touch. Gated in three places: the info popup (the button greys out and says which structure is in the way), _confirm_build, and _try_place - the preview goes stale as the fleet keeps moving. Ordering matters in one case. A unique structure replaces itself by demolishing the old one BEFORE placing the new one, so a placement refused afterwards would leave the player with neither. The check runs before the demolition and excludes the structure being replaced. Case D pins it. Two existing tests encoded the old permissive behaviour and now fly clear first; neither is about placement: test_build_info_popup builds every menu option from one position, so each structure blocked the next test_payments_use_fleet_hold builds a mothership on the ship lab that an earlier case injects at the origin Also fixes a bug from 37e2e89: Sfx.warm() called add_child() from _ready(), which Godot refuses outright - it printed an error and never pre-built the pool, defeating the point of warming it. Deferred, like Music.ensure(). 173/173. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019AJMdB7iuHuLFZM1aBu3GD --- audio/Sfx.gd | 8 +- controllers/BuildController.gd | 36 ++++++- core/GameState.gd | 22 +++++ tests/test_build_info_popup.gd | 11 ++- tests/test_building_separation.gd | 135 ++++++++++++++++++++++++++ tests/test_payments_use_fleet_hold.gd | 6 ++ 6 files changed, 214 insertions(+), 4 deletions(-) create mode 100644 tests/test_building_separation.gd diff --git a/audio/Sfx.gd b/audio/Sfx.gd index a748599..e8dac10 100644 --- a/audio/Sfx.gd +++ b/audio/Sfx.gd @@ -94,7 +94,13 @@ const PITCH_SPREAD: Dictionary = { static func warm(tree: SceneTree) -> void: if tree == null: return - _pool(tree) + # Deferred, like Music.ensure(): this is called from _ready(), where the + # parent is still setting up children and a direct add_child() is + # refused outright. The lazy path in play() runs during an ordinary + # frame and does not need it. + var build: Callable = func() -> void: + _pool(tree) + build.call_deferred() static func _pool(tree: SceneTree) -> Node: diff --git a/controllers/BuildController.gd b/controllers/BuildController.gd index 915fe12..c765963 100644 --- a/controllers/BuildController.gd +++ b/controllers/BuildController.gd @@ -426,8 +426,17 @@ func _show_building_info(type: String) -> void: else: lines.append("Will mine %s from %s." % [ GameState.resource_name(planet.resource_type), planet.planet_name]) - elif can_build and definition.unique and GameState.has_building_of_type(type): - lines.append("WARNING: only one may exist - this destroys your current one.") + elif can_build: + if definition.unique and GameState.has_building_of_type(type): + lines.append("WARNING: only one may exist - this destroys your current one.") + # A unique structure replaces itself, so the one being demolished is + # not in the way. + var blocker: Dictionary = GameState.building_blocking(fleet.get_centroid(), + type if definition.unique else "") + if not blocker.is_empty(): + lines.append("Too close to your %s - fly clear and try again." \ + % _building_name(String(blocker.get("type", "")))) + can_build = false if can_build and not GameState.can_afford_resources(costs, fleet.get_centroid()): can_build = false pending_build_type = type if can_build else "" @@ -468,6 +477,16 @@ func _confirm_build() -> void: planet.resource_type, planet.planet_id) build_menu.close_menu() return + # BEFORE the demolition below, not after: a unique structure replaces + # itself by removing the old one first, so a blocked placement discovered + # afterwards would leave the player with neither. The old one is excluded + # because it is the thing being replaced. + var blocker: Dictionary = GameState.building_blocking(fleet.get_centroid(), + type if definition.unique else "") + if not blocker.is_empty(): + build_menu.set_status("Too close to your %s" \ + % _building_name(String(blocker.get("type", "")))) + return if definition.unique and GameState.has_building_of_type(type): if not GameState.can_afford_resources(GameState.building_costs(type), fleet.get_centroid()): build_menu.set_status(_cannot_afford(type)) @@ -477,6 +496,11 @@ func _confirm_build() -> void: _try_place(type) +func _building_name(type: String) -> String: + var definition: Resource = BUILDING_DEFINITIONS.get_definition(type) + return String(definition.display_name) if definition != null else type.capitalize() + + func _cannot_afford(type: String) -> String: var missing: Array = [] for resource_type in GameState.building_costs(type): @@ -489,6 +513,14 @@ func _cannot_afford(type: String) -> String: func _try_place(type: String) -> void: + # Re-checked here and not only in the preview: the fleet keeps moving + # between opening the menu and confirming, and this is the one place + # every space structure is actually placed. + var blocker: Dictionary = GameState.building_blocking(fleet.get_centroid()) + if not blocker.is_empty(): + build_menu.set_status("Too close to your %s" \ + % _building_name(String(blocker.get("type", "")))) + return if not GameState.spend_resources(GameState.building_costs(type), fleet.get_centroid()): build_menu.set_status(_cannot_afford(type)) return diff --git a/core/GameState.gd b/core/GameState.gd index fcab54e..b8a8e5a 100644 --- a/core/GameState.gd +++ b/core/GameState.gd @@ -2707,6 +2707,28 @@ func has_mothership() -> bool: return get_mothership_position() != null +# Space structures may not be stacked (#224). The distance is Building's own +# interact_radius, rather than a new number: that is the circle BuildingField +# tests the fleet centroid against to decide which structure you are standing +# on, and it returns the FIRST match - so two structures inside one another +# make that a question of list order rather than of where you flew. +const MIN_BUILDING_SEPARATION: float = 150.0 + + +# The structure a new one at this position would sit on top of, or an empty +# dictionary when the spot is clear. ignore_type skips a type that is about +# to be demolished, which is how the unique structures replace themselves - +# without it a mothership could never be rebuilt where the old one stood. +func building_blocking(at_position: Vector2, ignore_type: String = "") -> Dictionary: + for building in buildings: + if ignore_type != "" and String(building.get("type", "")) == ignore_type: + continue + var at: Vector2 = building.get("position", Vector2.ZERO) + if at.distance_to(at_position) < MIN_BUILDING_SEPARATION: + return building + return {} + + func has_building_of_type(type: String) -> bool: for building in buildings: if building.type == type: diff --git a/tests/test_build_info_popup.gd b/tests/test_build_info_popup.gd index 43988be..2ea2e7b 100644 --- a/tests/test_build_info_popup.gd +++ b/tests/test_build_info_popup.gd @@ -17,6 +17,7 @@ extends SceneTree var scene: Node var game_state: Node var controller: Node +var fleet: Node2D var popup: Control var title: Label var message: Label @@ -55,6 +56,7 @@ func _initialize() -> void: scene = load("res://world/Main.tscn").instantiate() root.add_child(scene) controller = scene.get_node("BuildController") + fleet = scene.get_node("Fleet") popup = scene.get_node("UI/BuildMenu/InfoPopup") title = popup.get_node("Panel/TitleLabel") message = popup.get_node("Panel/MessageLabel") @@ -83,8 +85,15 @@ func _process(delta: float) -> bool: if elapsed < 0.2: return false - for option in OPTIONS: + for i in range(OPTIONS.size()): + var option: Dictionary = OPTIONS[i] var type: String = option.type + # #224: structures may not be stacked, and this loop BUILDS + # each option in turn. Without moving between them the second + # one is refused for sitting on the first, which is the rule + # working - but this test is about what the popup says, not + # about where things may go. + fleet.teleport_to(Vector2(float(i + 2) * GameState.MIN_BUILDING_SEPARATION * 2.0, 0.0)) var expected_cost: int = controller.cost_of(type) var rocks_before: int = game_state.rocks var built_before: int = _count(type) diff --git a/tests/test_building_separation.gd b/tests/test_building_separation.gd new file mode 100644 index 0000000..91cddce --- /dev/null +++ b/tests/test_building_separation.gd @@ -0,0 +1,135 @@ +extends SceneTree + +# Forgejo #224: a structure may not be built on top of one that is already +# there. +# +# It matters beyond the look of it. BuildingField decides which structure the +# fleet is standing on by testing the centroid against interact_radius and +# returning the FIRST match, so two structures inside one another make that a +# question of list order rather than of where the player flew - you would fly +# to your workshop and open the crafter. +# +# Covers: +# A. the spot a structure already occupies is refused +# B. clear of it, the same build goes through +# C. a unique structure can still be rebuilt where the old one stands, +# because the old one is what is being replaced +# D. and a REFUSED rebuild does not demolish the existing one first + +var game_state: Node +var scene: Node +var controller: Node +var fleet: Node2D +var elapsed: float = 0.0 +var stage: int = 0 + + +func _initialize() -> void: + game_state = root.get_node("GameState") + game_state.buildings.clear() + game_state.extractors.clear() + game_state.reset_progression() + game_state.reset_new_game() + game_state.intro_active = false + game_state.reset_quests() + game_state.rocks = 100000 + game_state.bank_inventory.set_amount(GameState.RESOURCE_GAS, 100000) + game_state.bank_inventory.set_amount(GameState.RESOURCE_ENERGY, 100000) + scene = load("res://world/Main.tscn").instantiate() + root.add_child(scene) + current_scene = scene + controller = scene.get_node("BuildController") + fleet = scene.get_node("Fleet") + + +func _process(delta: float) -> bool: + elapsed += delta + if elapsed < 0.4: + return false + + var sep: float = GameState.MIN_BUILDING_SEPARATION + + # _confirm_build() returns SILENTLY on a locked structure, which would + # make every refusal below look like the separation rule working when it + # was only the unlock gate. Unlock what this test builds, and assert it. + for type in [GameState.BUILDING_COMMAND_CENTER, GameState.BUILDING_MOTHERSHIP, + GameState.BUILDING_SHIP_CRAFTER]: + var node_id: String = game_state.research_node_for_building(type) + if node_id != "" and not game_state.is_unlocked(node_id): + game_state.unlocked.append(node_id) + if not game_state.is_building_unlocked(type): + return _fail("'%s' is still locked, so a refusal would prove nothing" % type) + + # --- A. the occupied spot is refused -------------------------------- + game_state.buildings.clear() + game_state.add_building(GameState.BUILDING_SHIP_CRAFTER, Vector2.ZERO) + + if game_state.building_blocking(Vector2.ZERO).is_empty(): + return _fail("a structure's own position reads as a clear spot") + # Just inside the separation, and just outside it. + if game_state.building_blocking(Vector2(sep - 1.0, 0.0)).is_empty(): + return _fail("a spot %.0f units away is not blocked, but the rule is %.0f" % [sep - 1.0, sep]) + if not game_state.building_blocking(Vector2(sep + 1.0, 0.0)).is_empty(): + return _fail("a spot %.0f units away is blocked, but the rule is %.0f" % [sep + 1.0, sep]) + var blocker: Dictionary = game_state.building_blocking(Vector2.ZERO) + if String(blocker.get("type", "")) != GameState.BUILDING_SHIP_CRAFTER: + return _fail("the blocking structure is reported as '%s'" % blocker.get("type", "")) + print("a spot inside %.0f units of a structure is refused, outside it is clear" % sep) + + # --- B. and the whole build path agrees ----------------------------- + # On top of the crafter: refused, and nothing is built or spent. + fleet.teleport_to(Vector2.ZERO) + var before: int = game_state.buildings.size() + controller.pending_build_type = GameState.BUILDING_COMMAND_CENTER + controller._confirm_build() + if game_state.buildings.size() != before: + return _fail("a structure was built on top of the crafter") + print("the build path refuses it too: %s" % controller.build_menu.status.text) + + # Clear of it: the same build goes through. + fleet.teleport_to(Vector2(sep * 3.0, 0.0)) + controller.pending_build_type = GameState.BUILDING_COMMAND_CENTER + controller._confirm_build() + if game_state.buildings.size() != before + 1: + return _fail("the same build was refused on a clear spot") + print("clear of it, the same build goes through") + + # --- C. a unique structure replaces itself in place ----------------- + game_state.buildings.clear() + game_state.add_building(GameState.BUILDING_MOTHERSHIP, Vector2.ZERO) + fleet.teleport_to(Vector2.ZERO) + if not game_state.building_blocking(Vector2.ZERO, GameState.BUILDING_MOTHERSHIP).is_empty(): + return _fail("a unique structure blocks its own replacement") + controller.pending_build_type = GameState.BUILDING_MOTHERSHIP + controller._confirm_build() + if game_state.buildings.size() != 1: + return _fail("rebuilding the mothership in place left %d structures" % game_state.buildings.size()) + if String(game_state.buildings[0].type) != GameState.BUILDING_MOTHERSHIP: + return _fail("the rebuilt structure is not a mothership") + print("a unique structure can still be rebuilt where the old one stood") + + # --- D. a refused rebuild demolishes nothing ------------------------ + # The mothership is unique and is removed before the new one is placed. + # Something ELSE in the way has to be caught before that demolition, or + # the player is left with neither. + game_state.buildings.clear() + var keep: int = game_state.add_building(GameState.BUILDING_MOTHERSHIP, Vector2.ZERO) + game_state.add_building(GameState.BUILDING_SHIP_CRAFTER, Vector2(sep * 4.0, 0.0)) + fleet.teleport_to(Vector2(sep * 4.0, 0.0)) + controller.pending_build_type = GameState.BUILDING_MOTHERSHIP + controller._confirm_build() + if game_state.find_building(keep).is_empty(): + return _fail("a refused rebuild demolished the existing mothership - the player is left with neither") + if game_state.buildings.size() != 2: + return _fail("a refused rebuild changed the structure count to %d" % game_state.buildings.size()) + print("a refused rebuild leaves the existing structure standing") + + print("TEST_BUILDING_SEPARATION: PASS") + quit(0) + return true + + +func _fail(message: String) -> bool: + print("TEST_BUILDING_SEPARATION: FAIL (%s)" % message) + quit(1) + return true diff --git a/tests/test_payments_use_fleet_hold.gd b/tests/test_payments_use_fleet_hold.gd index 690c9ad..24fcaf5 100644 --- a/tests/test_payments_use_fleet_hold.gd +++ b/tests/test_payments_use_fleet_hold.gd @@ -270,6 +270,12 @@ func _process(delta: float) -> bool: game_state.fleet_inventory.set_amount(GameState.RESOURCE_ORE, HOLD_ORE) var before: int = game_state.buildings.size() + # #224: structures may not be stacked, and _check_ship_lab left a + # lab standing at the origin as a prerequisite for an earlier + # case. Fly clear of it - the subject here is which purse the ore + # comes out of, not where a mothership may stand. + scene.get_node("Fleet").teleport_to( + Vector2(GameState.MIN_BUILDING_SEPARATION * 4.0, 0.0)) build_controller.pending_build_type = GameState.BUILDING_MOTHERSHIP build_controller._confirm_build()