Fix encoding components on chunk unload or server restart

MineInAbyss · Dec 25, 2023 · 6dcb5dd · 6dcb5dd
1 parent 7899828
commit 6dcb5dd
Show file tree

Hide file tree

Showing 4 changed files with 34 additions and 24 deletions.
diff --git a/geary-papermc-plugin/src/main/kotlin/com/mineinabyss/geary/papermc/plugin/GearyPluginImpl.kt b/geary-papermc-plugin/src/main/kotlin/com/mineinabyss/geary/papermc/plugin/GearyPluginImpl.kt
@@ -9,6 +9,7 @@ import com.mineinabyss.geary.papermc.GearyPlugin
 import com.mineinabyss.geary.papermc.GearyProductionPaperConfigModule
 import com.mineinabyss.geary.papermc.bridge.PaperBridge
 import com.mineinabyss.geary.papermc.configlang.ConfigLang
+import com.mineinabyss.geary.papermc.datastore.encodeComponentsTo
 import com.mineinabyss.geary.papermc.datastore.withUUIDSerializer
 import com.mineinabyss.geary.papermc.tracking.blocks.BlockTracking
 import com.mineinabyss.geary.papermc.tracking.blocks.gearyBlocks
@@ -29,7 +30,6 @@ import com.mineinabyss.idofront.serialization.SerializablePrefabItemService
 import com.mineinabyss.serialization.formats.YamlFormat
 import okio.FileSystem
 import okio.Path.Companion.toOkioPath
-import org.bukkit.entity.Player
 import org.bukkit.event.Listener
 import org.bukkit.inventory.ItemStack
 import kotlin.io.path.isDirectory
@@ -118,8 +118,9 @@ class GearyPluginImpl : GearyPlugin() {
     override fun onDisable() {
         server.worlds.forEach { world ->
             world.entities.forEach entities@{ entity ->
-                if (entity is Player) return@entities
-                entity.toGearyOrNull()?.removeEntity()
+                val gearyEntity = entity.toGearyOrNull() ?: return@entities
+                gearyEntity.encodeComponentsTo(entity)
+                gearyEntity.removeEntity()
             }
         }
         server.scheduler.cancelTasks(this)

diff --git a/...kotlin/com/mineinabyss/geary/papermc/tracking/entities/systems/EntityWorldEventTracker.kt b/...kotlin/com/mineinabyss/geary/papermc/tracking/entities/systems/EntityWorldEventTracker.kt
@@ -2,6 +2,7 @@ package com.mineinabyss.geary.papermc.tracking.entities.systems
 
 import com.destroystokyo.paper.event.entity.EntityAddToWorldEvent
 import com.destroystokyo.paper.event.entity.EntityRemoveFromWorldEvent
+import com.mineinabyss.geary.papermc.datastore.encodeComponentsTo
 import com.mineinabyss.geary.papermc.gearyPaper
 import com.mineinabyss.geary.papermc.tracking.entities.gearyMobs
 import com.mineinabyss.geary.papermc.tracking.entities.toGearyOrNull
@@ -12,6 +13,7 @@ import org.bukkit.event.EventPriority
 import org.bukkit.event.Listener
 import org.bukkit.event.player.PlayerJoinEvent
 import org.bukkit.event.player.PlayerQuitEvent
+import org.bukkit.event.world.EntitiesUnloadEvent
 
 class EntityWorldEventTracker : Listener {
     /** Remove entities from ECS when they are removed from Bukkit for any reason (Uses PaperMC event) */
@@ -27,21 +29,31 @@ class EntityWorldEventTracker : Listener {
     fun EntityRemoveFromWorldEvent.onBukkitEntityRemove() {
         // Only remove player from ECS on disconnect, not death
         if (entity is Player) return
-        // We remove the geary entity one tick after the Bukkit one has been removed to ensure nothing
-        // else that tries to access the geary entity from Bukkit will create a new entity.
+
+        // We remove the geary entity a bit later because paper has a bug where stored entities call load/unload/load
         Bukkit.getScheduler().scheduleSyncDelayedTask(gearyPaper.plugin, {
-            if(entity.isValid) return@scheduleSyncDelayedTask
+            if (entity.isValid) return@scheduleSyncDelayedTask // If the entity is still valid, it's the paper bug
             entity.toGearyOrNull()?.removeEntity()
         }, 10)
     }
 
+    @EventHandler(priority = EventPriority.HIGHEST)
+    fun EntitiesUnloadEvent.onEntitiesUnload() {
+        entities.forEach {
+            val gearyEntity = it.toGearyOrNull() ?: return@forEach
+            gearyEntity.encodeComponentsTo(it)
+        }
+    }
+
     @EventHandler(priority = EventPriority.LOWEST)
     fun PlayerJoinEvent.onPlayerLogin() {
         gearyMobs.bukkit2Geary.getOrCreate(player)
     }
 
     @EventHandler(priority = EventPriority.HIGHEST)
     fun PlayerQuitEvent.onPlayerLogout() {
-        player.toGearyOrNull()?.removeEntity()
+        val gearyEntity = player.toGearyOrNull() ?: return
+        gearyEntity.encodeComponentsTo(player)
+        gearyEntity.removeEntity()
     }
 }
diff --git a/...com/mineinabyss/geary/papermc/tracking/entities/systems/UntrackOnRemoveBukkitComponent.kt b/...com/mineinabyss/geary/papermc/tracking/entities/systems/UntrackOnRemoveBukkitComponent.kt
@@ -3,7 +3,6 @@ package com.mineinabyss.geary.papermc.tracking.entities.systems
 import com.mineinabyss.geary.annotations.optin.UnsafeAccessors
 import com.mineinabyss.geary.components.events.EntityRemoved
 import com.mineinabyss.geary.datatypes.family.family
-import com.mineinabyss.geary.papermc.datastore.encodeComponentsTo
 import com.mineinabyss.geary.papermc.tracking.entities.gearyMobs
 import com.mineinabyss.geary.systems.GearyListener
 import com.mineinabyss.geary.systems.accessors.Pointers
@@ -16,6 +15,5 @@ class UntrackOnRemoveBukkitComponent : GearyListener() {
     @OptIn(UnsafeAccessors::class)
     override fun Pointers.handle() {
         gearyMobs.bukkit2Geary.remove(bukkit.entityId)
-        target.entity.encodeComponentsTo(bukkit)
     }
 }
diff --git a/...ts/src/test/kotlin/com/mineinabyss/geary/papermc/tracking/entities/EntityTrackingTests.kt b/...ts/src/test/kotlin/com/mineinabyss/geary/papermc/tracking/entities/EntityTrackingTests.kt
@@ -17,6 +17,7 @@ import io.kotest.matchers.nulls.shouldNotBeNull
 import io.kotest.matchers.shouldBe
 import org.bukkit.entity.Pig
 import org.bukkit.entity.Player
+import org.bukkit.event.world.EntitiesUnloadEvent
 import org.bukkit.persistence.PersistentDataContainer
 import org.junit.jupiter.api.Nested
 import org.junit.jupiter.api.Test
@@ -91,7 +92,7 @@ class EntityTrackingTests: MockedServerTest() {
         }
 
         @Test
-        fun `should persist components on player disconnect`() {
+        fun `should persist components when player disconnects`() {
             val player = server.addPlayer()
 
             testPersistence(
@@ -101,19 +102,17 @@ class EntityTrackingTests: MockedServerTest() {
             )
         }
 
-        //TODO this test wouldn't reflect data actually being written to NBT, maybe the event calls too late to save!
-//        @Test
-//        fun `should persist components on entities`() {
-//            val pig = world.spawn(world.spawnLocation, Pig::class.java)
-//            EntityAddToWorldEvent(pig).callEvent()
-//
-//            testPersistence(
-//                pig.toGeary(),
-//                pig.persistentDataContainer
-//            ) {
-//                pig.remove()
-//                EntityRemoveFromWorldEvent(pig).callEvent()
-//            }
-//        }
+        @Test
+        fun `should persist components on entities when chunk unloaded`() {
+            val pig = world.spawn(world.spawnLocation, Pig::class.java)
+            EntityAddToWorldEvent(pig).callEvent()
+
+            testPersistence(
+                pig.toGeary(),
+                pig.persistentDataContainer
+            ) {
+                EntitiesUnloadEvent(pig.location.chunk, listOf(pig)).callEvent()
+            }
+        }
     }
 }