From 250da8c9aa6ec1c188c4b8f4fef0d642518dcb7d Mon Sep 17 00:00:00 2001 From: Malte Langkabel Date: Thu, 6 Aug 2026 17:37:57 +0200 Subject: [PATCH] make re-applying an unchanged ship layout a no-op setRecipe has always returned early when the recipe is unchanged, so a redundant selection does not reset buffers. setShipLayout had no such guard: it cancelled the production cycle and wiped every buffer on any call, including one that set the layout the shipyard already had. That already contradicted REQ-BLD-SHIPYARD, which cancels a cycle only when the player "confirms a layout change", and it becomes load-bearing for the blueprint configuration transfer of REQ-UI-BLUEPRINT-TRANSFER, which is meant to be clicked repeatedly onto matching shipyards. REQ-MAT-INPUT-BUFFER now states the rule generally: setting a configuration to the value it already holds changes nothing, whatever path applies it. An unset layout counts as an empty one. The two are equivalent for buffers, production, and the spawned ship -- the spawn path already converts nullopt to an empty layout deliberately -- so clearing an already-unconfigured shipyard is a no-op too. That is the case a transfer from a layout-less source produces. The only place the two differ is the has_value() bit in StateChecksum, and record and replay take the same branch, so determinism is unaffected. Layout equality is order-sensitive by choice: reporting a change that is not one costs a buffer reset, while missing a real change would leave a shipyard building the wrong ship. BuildingConfigTest's shipyard case used an empty layout as shorthand for "a layout is set", which is now a no-op and left nothing to read back. It configures a real module instead and checks it round-trips. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01JcReq7hVk4KUPhTDKWAG7K --- src/lib/sim/BuildingSystem.cpp | 15 ++++ src/lib/sim/ShipLayout.h | 15 ++++ src/test/BuildingConfigTest.cpp | 15 +++- src/test/ShipModuleTest.cpp | 117 ++++++++++++++++++++++++++++++++ 4 files changed, 160 insertions(+), 2 deletions(-) diff --git a/src/lib/sim/BuildingSystem.cpp b/src/lib/sim/BuildingSystem.cpp index 09348c3..294fc59 100644 --- a/src/lib/sim/BuildingSystem.cpp +++ b/src/lib/sim/BuildingSystem.cpp @@ -284,6 +284,21 @@ void BuildingSystem::setShipLayout(FactoryState& state, BuildingId id, const Shi { if (building.id == id) { + // No-op if the layout is unchanged, so re-applying the layout a shipyard + // already has does not cancel its production cycle or wipe its buffers + // (REQ-MAT-INPUT-BUFFER, REQ-BLD-SHIPYARD). Confirming the layout dialog + // without editing anything, and a blueprint configuration transfer onto an + // already-matching shipyard (REQ-UI-BLUEPRINT-TRANSFER), both land here. + // An unset layout counts as an empty one: the two are equivalent for + // buffers, production, and the spawned ship (see the spawn path below), + // so an empty layout arriving at an unconfigured shipyard changes nothing. + const bool unchanged = building.shipLayout.has_value() + ? *building.shipLayout == layout + : layout.placedModules.empty(); + if (unchanged) + { + return; + } if (building.production.has_value()) { building.production = std::nullopt; diff --git a/src/lib/sim/ShipLayout.h b/src/lib/sim/ShipLayout.h index f616f5a..4d9159d 100644 --- a/src/lib/sim/ShipLayout.h +++ b/src/lib/sim/ShipLayout.h @@ -15,8 +15,23 @@ struct PlacedModule Rotation rotation; }; +inline bool operator==(const PlacedModule& a, const PlacedModule& b) +{ + return a.moduleId == b.moduleId && a.position == b.position && a.rotation == b.rotation; +} + // The complete module configuration for a shipyard's current ship (REQ-MOD-CONFIG). struct ShipLayoutConfig { std::vector placedModules; }; + +// Deliberately order-sensitive: two layouts holding the same modules in a different +// vector order compare unequal. This only decides whether applying a layout is a no-op +// (REQ-MAT-INPUT-BUFFER), so the conservative answer is the safe one -- reporting a +// change that is not one costs a buffer reset, reporting no change when there is one +// would leave the shipyard stale. +inline bool operator==(const ShipLayoutConfig& a, const ShipLayoutConfig& b) +{ + return a.placedModules == b.placedModules; +} diff --git a/src/test/BuildingConfigTest.cpp b/src/test/BuildingConfigTest.cpp index 0b6c8a1..9aae77d 100644 --- a/src/test/BuildingConfigTest.cpp +++ b/src/test/BuildingConfigTest.cpp @@ -103,14 +103,25 @@ TEST_CASE("readBuildingConfig returns a shipyard's schematic and layout", const BuildingId id = placeOperational(sim, cfg, BuildingType::Shipyard, QPoint(0, 0)); SimulationTestAccess::buildings(sim).setRecipe(SimulationTestAccess::state(sim), id, schematic->id); - SimulationTestAccess::buildings(sim).setShipLayout(SimulationTestAccess::state(sim), id, ShipLayoutConfig{}); + + // A real layout, not an empty one: applying an empty layout to a shipyard that has + // none is a no-op (REQ-MAT-INPUT-BUFFER), so it would leave nothing to read back. + ShipLayoutConfig layout; + PlacedModule placed; + placed.moduleId = "armor_plate"; + placed.position = QPoint(0, 0); + placed.rotation = Rotation::East; + layout.placedModules.push_back(placed); + SimulationTestAccess::buildings(sim).setShipLayout(SimulationTestAccess::state(sim), id, layout); const std::optional config = readBuildingConfig(sim, id); REQUIRE(config.has_value()); CHECK(config->type == BuildingType::Shipyard); REQUIRE(config->recipeId.has_value()); CHECK(*config->recipeId == schematic->id); - CHECK(config->shipLayout.has_value()); + REQUIRE(config->shipLayout.has_value()); + REQUIRE(config->shipLayout->placedModules.size() == 1); + CHECK(config->shipLayout->placedModules[0].moduleId == "armor_plate"); } TEST_CASE("readBuildingConfig reads a queued construction site", "[blueprint]") diff --git a/src/test/ShipModuleTest.cpp b/src/test/ShipModuleTest.cpp index b24dc35..ee0caae 100644 --- a/src/test/ShipModuleTest.cpp +++ b/src/test/ShipModuleTest.cpp @@ -262,6 +262,123 @@ TEST_CASE("Shipyard: setShipLayout cancels in-progress production", CHECK_FALSE(b2->production.has_value()); } +// Applying a configuration a building already has is a no-op (REQ-MAT-INPUT-BUFFER). +// setRecipe has always worked this way; setShipLayout did not, and wiped the shipyard +// on every re-apply. That matters now that a blueprint configuration transfer +// (REQ-UI-BLUEPRINT-TRANSFER) can be clicked repeatedly onto matching shipyards. + +TEST_CASE("Shipyard: re-applying the same layout keeps production and buffers", + "[modules][shipyard]") +{ + Simulation sim(loadTestConfig(), 42); + const ShipDef* def = findSchematic(sim.getConfig(), "interceptor"); + REQUIRE(def != nullptr); + const BuildingDef* yardDef = findShipyardDef(sim.getConfig()); + REQUIRE(yardDef != nullptr); + + const BuildingId yardId = placeShipyard(sim, *yardDef); + SimulationTestAccess::buildings(sim).setRecipe(SimulationTestAccess::state(sim), yardId, "interceptor"); + + ShipLayoutConfig layout; + PlacedModule pm; + pm.moduleId = "armor_plate"; + pm.position = QPoint(0, 0); + pm.rotation = Rotation::East; + layout.placedModules.push_back(pm); + + SimulationTestAccess::buildings(sim).setShipLayout(SimulationTestAccess::state(sim), yardId, layout); + fillMaterials(sim, yardId, *def, layout); + sim.tick(); + + const Building* before = findBuilding(sim.getFactoryState(), yardId); + REQUIRE(before != nullptr); + REQUIRE(before->production.has_value()); + const Tick completesAt = before->production->completesAt; + + // A separately built but equal layout: equality is by value, not by identity. + ShipLayoutConfig sameLayout; + sameLayout.placedModules.push_back(pm); + SimulationTestAccess::buildings(sim).setShipLayout(SimulationTestAccess::state(sim), yardId, sameLayout); + + const Building* after = findBuilding(sim.getFactoryState(), yardId); + REQUIRE(after != nullptr); + REQUIRE(after->production.has_value()); + CHECK(after->production->completesAt == completesAt); + CHECK(after->inputBuffer.caps.at(ItemType{"iron_ingot"}) == 10); +} + +TEST_CASE("Shipyard: an empty layout on an unconfigured shipyard changes nothing", + "[modules][shipyard]") +{ + // An unset layout and an empty one are the same state everywhere that matters, so + // clearing an already-unconfigured shipyard must not cancel its cycle. This is the + // case a transfer from a layout-less source shipyard produces. + Simulation sim(loadTestConfig(), 42); + const ShipDef* def = findSchematic(sim.getConfig(), "interceptor"); + REQUIRE(def != nullptr); + const BuildingDef* yardDef = findShipyardDef(sim.getConfig()); + REQUIRE(yardDef != nullptr); + + const BuildingId yardId = placeShipyard(sim, *yardDef); + SimulationTestAccess::buildings(sim).setRecipe(SimulationTestAccess::state(sim), yardId, "interceptor"); + + ShipLayoutConfig emptyLayout; + fillMaterials(sim, yardId, *def, emptyLayout); + sim.tick(); + + const Building* before = findBuilding(sim.getFactoryState(), yardId); + REQUIRE(before != nullptr); + REQUIRE(before->production.has_value()); + REQUIRE_FALSE(before->shipLayout.has_value()); + + SimulationTestAccess::buildings(sim).setShipLayout(SimulationTestAccess::state(sim), yardId, emptyLayout); + + const Building* after = findBuilding(sim.getFactoryState(), yardId); + REQUIRE(after != nullptr); + CHECK(after->production.has_value()); + CHECK_FALSE(after->shipLayout.has_value()); +} + +TEST_CASE("Shipyard: the same modules in a different order count as a change", + "[modules][shipyard]") +{ + // Layout equality is deliberately order-sensitive (see ShipLayout.h): the safe + // answer when in doubt is "changed", which costs a buffer reset rather than + // leaving a shipyard building the wrong ship. + Simulation sim(loadTestConfig(), 42); + const ShipDef* def = findSchematic(sim.getConfig(), "interceptor"); + REQUIRE(def != nullptr); + const BuildingDef* yardDef = findShipyardDef(sim.getConfig()); + REQUIRE(yardDef != nullptr); + + const BuildingId yardId = placeShipyard(sim, *yardDef); + SimulationTestAccess::buildings(sim).setRecipe(SimulationTestAccess::state(sim), yardId, "interceptor"); + + PlacedModule armor; + armor.moduleId = "armor_plate"; + armor.position = QPoint(0, 0); + armor.rotation = Rotation::East; + PlacedModule sensor; + sensor.moduleId = "sensor_booster"; + sensor.position = QPoint(1, 0); + sensor.rotation = Rotation::East; + + ShipLayoutConfig layout; + layout.placedModules.push_back(armor); + layout.placedModules.push_back(sensor); + SimulationTestAccess::buildings(sim).setShipLayout(SimulationTestAccess::state(sim), yardId, layout); + fillMaterials(sim, yardId, *def, layout); + sim.tick(); + REQUIRE(findBuilding(sim.getFactoryState(), yardId)->production.has_value()); + + ShipLayoutConfig reordered; + reordered.placedModules.push_back(sensor); + reordered.placedModules.push_back(armor); + SimulationTestAccess::buildings(sim).setShipLayout(SimulationTestAccess::state(sim), yardId, reordered); + + CHECK_FALSE(findBuilding(sim.getFactoryState(), yardId)->production.has_value()); +} + TEST_CASE("Shipyard: builds a bare hull when no layout is configured", "[modules][shipyard]") {