Fix pet entities disappearing by adding setRemoveWhenFarAway(false) and chunk-load persistence#299
Fix pet entities disappearing by adding setRemoveWhenFarAway(false) and chunk-load persistence#299
Conversation
- Add Pet.ensurePersistence() to re-apply setPersistent(true) and setRemoveWhenFarAway(false) - Add setRemoveWhenFarAway(false) when taming pets in PetListRepository.addNewPet() - Re-apply persistence flags after server load in StorageService.applyAIStateToPets() - Add ChunkLoadHandler to re-apply persistence when pet entities' chunks load - Restore setRemoveWhenFarAway(true) when releasing pets in PetList.removePet() - Add unit tests for ensurePersistence() Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com>
…uplicating logic Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR aims to prevent tamed pet entities (e.g., iron golems) from despawning by consistently applying persistence-related flags at tame time and re-applying them after entity reload scenarios (startup + chunk loads).
Changes:
- Adds
Pet.ensurePersistence()to applysetPersistent(true)and (forLivingEntity)setRemoveWhenFarAway(false)with null-safety. - Re-applies persistence after storage load (
StorageService.applyAIStateToPets) and on chunk load via a newChunkLoadHandlerlistener. - Restores normal despawn behavior when releasing a pet by setting
setRemoveWhenFarAway(true)(forLivingEntity) inPetList.removePet(), and adds unit tests forensurePersistence().
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test/java/dansplugins/wildpets/pet/PetTest.java | Adds unit tests covering ensurePersistence() behavior across entity types and null cases. |
| src/main/java/dansplugins/wildpets/storage/StorageService.java | Ensures persistence flags are applied on the delayed post-load tick alongside AI state. |
| src/main/java/dansplugins/wildpets/pet/list/PetListRepository.java | Applies setRemoveWhenFarAway(false) at tame time for LivingEntity pets. |
| src/main/java/dansplugins/wildpets/pet/list/PetList.java | Resets setRemoveWhenFarAway(true) when removing/freeing a pet (for LivingEntity). |
| src/main/java/dansplugins/wildpets/pet/Pet.java | Introduces ensurePersistence() with null-safe entity retrieval and flag application. |
| src/main/java/dansplugins/wildpets/listeners/ChunkLoadHandler.java | New listener that re-applies persistence for pet entities present in loaded chunks. |
| src/main/java/dansplugins/wildpets/WildPets.java | Registers the new ChunkLoadHandler listener. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public void handle(ChunkLoadEvent event) { | ||
| for (Entity entity : event.getChunk().getEntities()) { | ||
| Pet pet = petListRepository.getPet(entity); | ||
| if (pet != null) { | ||
| pet.ensurePersistence(); | ||
| } |
There was a problem hiding this comment.
ChunkLoadEvent can fire very frequently, and this handler does an O(entities_in_chunk × total_pets) scan because petListRepository.getPet(entity) iterates all pet lists and each list iterates its pets. On servers with many pets/players this can create noticeable lag spikes during chunk loads. Consider maintaining a UUID→Pet index in PetListRepository (updated on add/remove/load) so the lookup here is O(1), or otherwise avoid nested linear scans in this event handler.
| * Ensures that the pet entity is marked as persistent and will not be removed when far away. | ||
| * This prevents the entity from despawning due to distance from players or chunk unloading. |
There was a problem hiding this comment.
The Javadoc says this prevents despawning due to "chunk unloading", but chunk unloading typically unloads entities rather than despawning them; the despawn issue here is primarily distance-based removal and/or loss of flags after reload. Rewording this comment to match the actual behavior (persistence + no-remove-when-far-away, and reapplying after chunk load) would avoid misleading future readers.
| * Ensures that the pet entity is marked as persistent and will not be removed when far away. | |
| * This prevents the entity from despawning due to distance from players or chunk unloading. | |
| * Ensures that, for a currently loaded pet entity, persistence and "do not remove when far away" | |
| * flags are applied. | |
| * This helps prevent the entity from being removed by distance-based despawn mechanics while it | |
| * is loaded. It does not prevent chunk unloading itself; these flags may need to be re-applied | |
| * after the entity is reloaded. |
| entity.setPersistent(true); | ||
| if (entity instanceof LivingEntity) { | ||
| ((LivingEntity) entity).setRemoveWhenFarAway(false); | ||
| } |
There was a problem hiding this comment.
Persistence logic is now duplicated between Pet.ensurePersistence() and this tame-time code path (setPersistent(true) + setRemoveWhenFarAway(false)). To reduce the chance of these paths drifting over time, consider reusing Pet.ensurePersistence() here (or extracting a shared helper) so persistence behavior is defined in one place.
| entity.setPersistent(true); | |
| if (entity instanceof LivingEntity) { | |
| ((LivingEntity) entity).setRemoveWhenFarAway(false); | |
| } | |
| newPet.ensurePersistence(); |
|
@copilot address comments |
…doc, deduplicate persistence logic - Add HashMap<UUID, Pet> index to PetListRepository for O(1) lookups in ChunkLoadHandler - Fix ensurePersistence() Javadoc to accurately describe distance-based despawn prevention - Replace inline setPersistent/setRemoveWhenFarAway in addNewPet() with ensurePersistence() - Add addExistingPet() and clearAll() methods to properly maintain the index Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com>
Addressed all three review comments in e8e5eb2:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (entity instanceof LivingEntity) { | ||
| ((LivingEntity) entity).setRemoveWhenFarAway(false); |
There was a problem hiding this comment.
setRemoveWhenFarAway(...) is not a method on LivingEntity in the Spigot API (it’s on Mob). As written, this will not compile against spigot-api 1.17.1-R0.1-SNAPSHOT. Change the check/cast to entity instanceof Mob and call ((Mob) entity).setRemoveWhenFarAway(false) (and drop the LivingEntity import if no longer needed).
| if (entity instanceof LivingEntity) { | |
| ((LivingEntity) entity).setRemoveWhenFarAway(false); | |
| if (entity instanceof Mob) { | |
| ((Mob) entity).setRemoveWhenFarAway(false); |
| if (entity instanceof LivingEntity) { | ||
| ((LivingEntity) entity).setRemoveWhenFarAway(true); | ||
| } |
There was a problem hiding this comment.
Same issue as in Pet.ensurePersistence(): setRemoveWhenFarAway(...) is not on LivingEntity in Spigot 1.17.1. Use entity instanceof Mob and call ((Mob) entity).setRemoveWhenFarAway(true) so this compiles and only applies to entities that support the flag.
| petsByEntityUUID.remove(petToRemove.getUniqueID()); | ||
| return getPetList(petToRemove.getOwnerUUID()).removePet(petToRemove); |
There was a problem hiding this comment.
removePet() removes the entry from petsByEntityUUID before confirming the pet was actually removed from the owning PetList. If PetList.removePet(...) returns false (or the owner list is missing), the UUID index becomes inconsistent with petLists. Consider only removing from the map after a successful removal (and guarding against a null PetList).
| petsByEntityUUID.remove(petToRemove.getUniqueID()); | |
| return getPetList(petToRemove.getOwnerUUID()).removePet(petToRemove); | |
| PetList ownerPetList = getPetList(petToRemove.getOwnerUUID()); | |
| if (ownerPetList == null) { | |
| return false; | |
| } | |
| boolean removed = ownerPetList.removePet(petToRemove); | |
| if (removed) { | |
| petsByEntityUUID.remove(petToRemove.getUniqueID()); | |
| } | |
| return removed; |
|
@copilot address comments |
…gainst null PetList - Pet.java: cast to Mob instead of LivingEntity for setRemoveWhenFarAway() - PetList.java: same LivingEntity→Mob fix - PetListRepository.removePet(): only remove from UUID index after confirmed removal from PetList, guard against null owner PetList - Update test comments to reference Mob instead of LivingEntity Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com> Agent-Logs-Url: https://github.com/Dans-Plugins/Wild-Pets/sessions/8576f88b-e3bb-4567-902b-11a5f37ec487
Addressed all three review comments in 59bfb05:
Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Pet entities (notably iron golems) could despawn because the plugin only called
setPersistent(true)at tame time but never setsetRemoveWhenFarAway(false)on theMob, and had no mechanism to re-apply persistence flags after chunk load or server restart.Changes
Pet.ensurePersistence()— new method that applies bothsetPersistent(true)andsetRemoveWhenFarAway(false)(viaMobcast, the correct Spigot API type), with null-safety for unloaded entitiesPetListRepository.addNewPet()— delegates toensurePersistence()at tame time to avoid duplicating persistence logicPetListRepositoryUUID index — added aHashMap<UUID, Pet>index for O(1) pet lookups by entity UUID, replacing the previous O(petLists × petsPerList) linear scan. Maintained viaaddNewPet(),removePet(),addExistingPet(), andclearAll().PetListRepository.removePet()— guards against null ownerPetListand only removes from the UUID index after confirmed successful removal, keeping the index consistentStorageService.applyAIStateToPets()— callensurePersistence()alongsideapplyAIState()on the post-load scheduled tickStorageService.loadPets()— uses newaddExistingPet()andclearAll()methods to properly maintain the UUID index during loadChunkLoadHandler(new listener) — re-applies persistence onChunkLoadEventfor any pet entity in the loaded chunk, using the O(1) UUID indexPetList.removePet()— restoresetRemoveWhenFarAway(true)(viaMobcast) when releasing a pet so freed entities resume normal despawn behaviorUnit tests added for
ensurePersistence()covering Mob, non-Mob entity, null entity, and null server provider cases.Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.