diff --git a/Server/src/main/content/global/skill/construction/HouseManager.java b/Server/src/main/content/global/skill/construction/HouseManager.java index 4db8e8191..4f63bf279 100644 --- a/Server/src/main/content/global/skill/construction/HouseManager.java +++ b/Server/src/main/content/global/skill/construction/HouseManager.java @@ -43,6 +43,11 @@ public final class HouseManager { */ private DynamicRegion dungeonRegion; + /** + * If the current regions have been handed off to delayed teardown. + */ + private boolean teardownPending; + /** * The house location. */ @@ -368,9 +373,24 @@ public final class HouseManager { } ZoneBuilder.configure(zone); + teardownPending = false; return houseRegion; } + /** + * Marks the current regions as unavailable while their delayed teardown is pending. + * Stale regions from a house rebuild must not change the lifecycle of the replacement + * regions now referenced by this manager. + * + * @param houseToRemove The house region being left. + * @param dungeonToRemove The dungeon region paired with the house, if any. + */ + void beginTeardown(Region houseToRemove, Region dungeonToRemove) { + if (houseRegion == houseToRemove && dungeonRegion == dungeonToRemove) { + teardownPending = true; + } + } + private DynamicRegion getPreparedRegion() { ZoneBorders borders = DynamicRegion.reserveArea(8,8); DynamicRegion region = new DynamicRegion(-1, borders.getSouthWestX() >> 6, borders.getSouthWestY() >> 6); @@ -708,7 +728,7 @@ public final class HouseManager { // return (houseRegion != null) || (dungeonRegion != null); //} public boolean isLoaded() { - return (houseRegion != null && houseRegion.isActive()) || (dungeonRegion != null && dungeonRegion.isActive()); + return !teardownPending && ((houseRegion != null && houseRegion.isActive()) || (dungeonRegion != null && dungeonRegion.isActive())); } /** diff --git a/Server/src/main/content/global/skill/construction/HouseZone.java b/Server/src/main/content/global/skill/construction/HouseZone.java index 31a57f6f8..647e8d3ff 100644 --- a/Server/src/main/content/global/skill/construction/HouseZone.java +++ b/Server/src/main/content/global/skill/construction/HouseZone.java @@ -2,7 +2,6 @@ package content.global.skill.construction; import core.api.Container; -import core.game.world.map.Location; import org.rs09.consts.Items; import core.game.node.entity.Entity; import core.game.node.entity.player.Player; @@ -27,14 +26,14 @@ public final class HouseZone extends MapZone { private HouseManager house; /** - * The previous house region id. + * The house region occupied before the latest transition. */ - private int previousRegion = -1; + private Region previousRegion; /** - * The previous dungeon region id. + * The dungeon region paired with the previous house region. */ - private int previousDungeon = -1; + private Region previousDungeon; /** * Constructs the house zone object. @@ -55,11 +54,11 @@ public final class HouseZone extends MapZone { } private void unregisterOldRegions() { - if (previousRegion != -1) { - unregisterRegion(previousRegion); + if (previousRegion != null && RegionManager.getRegionCache().get(previousRegion.getId()) == previousRegion) { + unregisterRegion(previousRegion.getId()); } - if (previousDungeon != -1) { - unregisterRegion(previousDungeon); + if (previousDungeon != null && RegionManager.getRegionCache().get(previousDungeon.getId()) == previousDungeon) { + unregisterRegion(previousDungeon.getId()); } } @@ -68,9 +67,8 @@ public final class HouseZone extends MapZone { if (e instanceof Player) { Player pl = (Player) e; if (house == pl.getHouseManager()) { - previousRegion = house.getHouseRegion().getId(); - if (house.getDungeonRegion() != null) - previousDungeon = house.getDungeonRegion().getId(); + previousRegion = house.getHouseRegion(); + previousDungeon = house.getDungeonRegion(); } registerLogoutListener(pl, "houselogout", (p) -> { p.setLocation(house.getLocation().getExitLocation()); @@ -95,48 +93,29 @@ public final class HouseZone extends MapZone { if (e instanceof Player) { Player p = (Player) e; - // if the player is moving between the dungeon and main house, don't trigger the leave sequence - if (house.getDungeonRegion() != null && !logout) { - // current loc - Location dest = p.getProperties().getTeleportLocation(); - int currentRegionId = p.getLocation().getRegionId(); - int houseId = house.getHouseRegion().getId(); - int dungeonId = house.getDungeonRegion().getId(); - - // check if still in house - boolean currentlyInHouse = (currentRegionId == houseId || currentRegionId == dungeonId); - - // check if moving between house regions - int destRegionId = (dest != null) ? dest.getRegionId() : -1; - boolean movingToHouse = (destRegionId == houseId || destRegionId == dungeonId); - - // if yes, return true and stop the leave sequence - if (currentlyInHouse || movingToHouse) { - return true; + // RegionManager moves the player into the destination region before notifying the old + // zone. Remaining in this manager's current regions is therefore an internal move, not + // a real house exit. When the region instances changed, the move is a house rebuild + // (such as a building-mode toggle), so only the stale instances should be torn down. + if (!logout && (p.getViewport().getRegion() == house.getHouseRegion() + || p.getViewport().getRegion() == house.getDungeonRegion())) { + if (previousRegion != house.getHouseRegion() || previousDungeon != house.getDungeonRegion()) { + callFamiliar(p); + submitRegionRemoval(previousRegion, previousDungeon); } + return true; + } + + if (!logout) { + callFamiliar(p); } remove_items(p); // The below tears down the house if the owner was the one who left if (house == p.getHouseManager()) { house.expelGuests(p); - int toRemove = previousRegion; - int dungRemove = previousDungeon; - submitWorldPulse(new Pulse(1) { - public boolean pulse() { - Region r = RegionManager.forId(toRemove); - Region dr = dungRemove != -1 ? RegionManager.forId(dungRemove) : null; - RegionManager.removeRegion(toRemove); - unregisterRegion(toRemove); - r.flagInactive(); - if (dungRemove != -1) { - RegionManager.removeRegion(dungRemove); - unregisterRegion(dungRemove); - dr.flagInactive(); - } - return true; - } - }); + house.beginTeardown(previousRegion, previousDungeon); + submitRegionRemoval(previousRegion, previousDungeon); } // Clear logout listener and original-loc (if appropriate) clearLogoutListener(p, "houselogout"); @@ -148,6 +127,36 @@ public final class HouseZone extends MapZone { return true; } + /** + * Takes the player's familiar along before a stale house region is cleared. + */ + private void callFamiliar(Player player) { + if (player.getFamiliarManager().hasFamiliar()) { + player.getFamiliarManager().getFamiliar().call(); + } + } + + /** + * Removes only the exact region instances captured while leaving. This prevents delayed + * cleanup from evicting a replacement region that has since reused the same coordinates. + */ + private void submitRegionRemoval(Region houseToRemove, Region dungeonToRemove) { + if (houseToRemove == null && dungeonToRemove == null) { + return; + } + submitWorldPulse(new Pulse(1) { + public boolean pulse() { + if (houseToRemove != null) { + RegionManager.removeRegion(houseToRemove); + } + if (dungeonToRemove != null) { + RegionManager.removeRegion(dungeonToRemove); + } + return true; + } + }); + } + private void remove_items(Player p) { for (int item = Items.KETTLE_7688; item <= Items.CHEFS_DELIGHT_7755; item++) {//Removes all PoH versions of tea and beer barrel-related items removeAll(p, item, Container.INVENTORY); diff --git a/Server/src/main/content/global/skill/summoning/familiar/Familiar.java b/Server/src/main/content/global/skill/summoning/familiar/Familiar.java index 089db9352..851c36ceb 100644 --- a/Server/src/main/content/global/skill/summoning/familiar/Familiar.java +++ b/Server/src/main/content/global/skill/summoning/familiar/Familiar.java @@ -393,10 +393,17 @@ public abstract class Familiar extends NPC implements Plugin { return false; } - + @Override public void onRegionInactivity() { call(); + // NPC.tick() short-circuits into this method while the region is inactive, so Entity.tick() + // never runs to consume a teleport that call() just set; apply it manually here or the + // familiar re-calls forever without ever moving (GL #2313). + if (getProperties().getTeleportLocation() != null && owner.isActive()) { + getWalkingQueue().update(); + getUpdateMasks().prepare(this); + } } @Override @@ -703,10 +710,20 @@ public abstract class Familiar extends NPC implements Plugin { //owner.getPacketDispatch().sendMessage("Your familiar is too big to fit here. Try calling it again when you are standing"); //owner.getPacketDispatch().sendMessage("somewhere with more space."); //spamTimer = 50; + // No room to materialize, so wait invisibly at the owner's side (like init() does when + // there is no room at summon time). Waiting in place instead means being left behind: + // once the abandoned region unloads, the familiar either gets wiped with it + // (DynamicRegion teardown) or gets stuck endlessly re-calling from an inactive region + // that no longer ticks it normally (GL #2313). + setInvisible(true); + stayWithOwner(); return false; } setInvisible(owner.getZoneMonitor().isRestricted(ZoneRestriction.FOLLOWERS) && !owner.getLocks().isLocked("enable_summoning")); - if (isInvisible()) return true; + if (isInvisible()) { + stayWithOwner(); + return true; + } getProperties().setTeleportLocation(destination); if (!(this instanceof Pet)) { if (firstCall) { @@ -727,13 +744,33 @@ public abstract class Familiar extends NPC implements Plugin { } else { face(owner); } - if (!isRenderable() && owner.isActive()) { + // A familiar pulled in from another region must move immediately rather than on its next + // tick: NPCs tick before players, so when this call runs from the owner's movement (e.g. + // leaving a POH), the familiar's tick for this cycle has already passed - and a region + // teardown pulse (POH exit portal, building-mode rebuild) runs at the start of the next + // cycle, wiping any NPC still inside before it can consume a pending teleport (GL #2313). + boolean leftInAnotherRegion = getLocation().getRegionId() != owner.getLocation().getRegionId(); + if ((!isRenderable() || leftInAnotherRegion) && owner.isActive()) { // log(this.getClass(), Log.ERR, "Familiar in inactive region!"); getWalkingQueue().update(); getUpdateMasks().prepare(this); } return true; } + + /** + * Moves a familiar that cannot materialize to its owner's location, so it is never left + * behind in a region that may unload. The teleport is applied immediately because this can + * run from an inactive region, where Entity.tick() (the normal consumer) no longer runs. + */ + private void stayWithOwner() { + if (!owner.isActive() || getLocation().equals(owner.getLocation())) { + return; + } + getProperties().setTeleportLocation(owner.getLocation()); + getWalkingQueue().update(); + getUpdateMasks().prepare(this); + } /** * Gets the spawning location of the familiar. diff --git a/Server/src/main/core/game/world/map/RegionManager.kt b/Server/src/main/core/game/world/map/RegionManager.kt index 9fb7b848b..8bc290b6e 100644 --- a/Server/src/main/core/game/world/map/RegionManager.kt +++ b/Server/src/main/core/game/world/map/RegionManager.kt @@ -332,10 +332,14 @@ object RegionManager { } } val l = owner.location.transform(stepX, stepY, 0) - // Check if ALL target tiles are unclipped + // Check if the NPC can stand on ALL target tiles. This must not use isClipped(), + // which rejects any tile that has a wall along ANY of its sides: every tile of a + // railed bridge deck has one (e.g. the whole Lumbridge bridge), which made familiars + // and pets unable to materialize anywhere on it (GL #1471). Materializing is the + // same question as teleporting, so ask the teleport predicate. for (x in 0 until node.size()) { for (y in 0 until node.size()) { - if (isClipped(l.transform(x, y, 0))) { + if (!isTeleportPermitted(l.transform(x, y, 0))) { continue@outer } } @@ -903,6 +907,25 @@ object RegionManager { } } + /** + * Removes a region only if it is still the cached instance for its id. + * + * Delayed cleanup tasks must use this overload so they cannot remove a newer + * dynamic region that has since been assigned the same coordinates. + */ + @JvmStatic + fun removeRegion(region: Region) { + if (lock.tryLock() || LOCK.tryLock(10000, TimeUnit.MILLISECONDS)) { + if (REGION_CACHE[region.id] === region) { + REGION_CACHE.remove(region.id) + } + if (region.isActive) { + region.flagInactive(true) + } + LOCK.unlock() + } + } + /** * Gets the regionCache. * @return The regionCache. diff --git a/Server/src/test/kotlin/content/global/skill/construction/HouseRegionLifecycleTests.kt b/Server/src/test/kotlin/content/global/skill/construction/HouseRegionLifecycleTests.kt new file mode 100644 index 000000000..eadc48042 --- /dev/null +++ b/Server/src/test/kotlin/content/global/skill/construction/HouseRegionLifecycleTests.kt @@ -0,0 +1,114 @@ +package content.global.skill.construction + +import TestUtils +import content.global.skill.summoning.familiar.SteelTitanNPC +import core.game.world.map.Region +import core.game.world.map.RegionManager +import org.junit.jupiter.api.Assertions.assertFalse +import org.junit.jupiter.api.Assertions.assertNotSame +import org.junit.jupiter.api.Assertions.assertSame +import org.junit.jupiter.api.Assertions.assertTrue +import org.junit.jupiter.api.Test + +class HouseRegionLifecycleTests { + companion object { + init { + TestUtils.preTestSetup() + } + } + + @Test + fun buildingModeRebuildKeepsReplacementHouseLoaded() { + TestUtils.getMockPlayer("house-build-rebuild").use { player -> + val house = player.houseManager + val familiar = SteelTitanNPC(player, 7343) + player.familiarManager.familiar = familiar + familiar.init() + house.createNewHouseAt(HouseLocation.RIMMINGTON) + house.enter(player, false) + player.walkingQueue.update() + familiar.call() + + val normalRegion = house.houseRegion + assertTrue(house.isInHouse(player)) + assertTrue(familiar.isActive) + assertSame(normalRegion, familiar.viewport.region) + + house.toggleBuildingMode(player, true) + val buildRegion = house.houseRegion + assertNotSame(normalRegion, buildRegion) + player.walkingQueue.update() + TestUtils.advanceTicks(2, false) + + assertTrue(house.isBuildingMode) + assertTrue(house.isLoaded) + assertTrue(house.isInHouse(player)) + assertFalse(normalRegion.isActive) + assertSame(buildRegion, RegionManager.regionCache[buildRegion.id]) + assertTrue(familiar.isActive) + assertSame(buildRegion, familiar.viewport.region) + + house.toggleBuildingMode(player, false) + val replacementNormalRegion = house.houseRegion + assertNotSame(buildRegion, replacementNormalRegion) + player.walkingQueue.update() + TestUtils.advanceTicks(2, false) + + assertFalse(house.isBuildingMode) + assertTrue(house.isLoaded) + assertTrue(house.isInHouse(player)) + assertFalse(buildRegion.isActive) + assertSame(replacementNormalRegion, RegionManager.regionCache[replacementNormalRegion.id]) + assertTrue(familiar.isActive) + assertSame(replacementNormalRegion, familiar.viewport.region) + + HouseManager.leave(player) + player.walkingQueue.update() + TestUtils.advanceTicks(2, false) + } + } + + @Test + fun regionPendingTeardownIsNotReusedByImmediateReentry() { + TestUtils.getMockPlayer("house-region-reentry").use { player -> + val house = player.houseManager + house.createNewHouseAt(HouseLocation.RIMMINGTON) + house.enter(player, false) + player.walkingQueue.update() + val oldRegion = house.houseRegion + + HouseManager.leave(player) + player.walkingQueue.update() + assertFalse(house.isLoaded) + + house.enter(player, false) + val replacement = house.houseRegion + assertNotSame(oldRegion, replacement) + player.walkingQueue.update() + TestUtils.advanceTicks(2, false) + + assertTrue(house.isLoaded) + assertTrue(house.isInHouse(player)) + assertFalse(oldRegion.isActive) + assertSame(replacement, RegionManager.regionCache[replacement.id]) + + HouseManager.leave(player) + player.walkingQueue.update() + TestUtils.advanceTicks(2, false) + } + } + + @Test + fun delayedRemovalDoesNotEvictReplacementAtSameRegionId() { + val house = HouseManager() + house.createNewHouseAt(HouseLocation.RIMMINGTON) + val oldRegion = house.construct() + val replacement = Region(oldRegion.x, oldRegion.y) + RegionManager.addRegion(replacement.id, replacement) + + RegionManager.removeRegion(oldRegion) + + assertSame(replacement, RegionManager.regionCache[replacement.id]) + RegionManager.removeRegion(replacement.id) + } +}