electronicboy
commented
1 year ago I'll have to look when I get back to my desk, but, the spawn reason of the entity being transported should be retained, last I looked the logic for persisting the spawn reason was especially fragile around stuff like entity teleports and spawners, I had ideas for a better fix around this stuff but have completely forgot what all I said
On Sun, 5 Feb 2023, 15:50 elmital, @.***> wrote:
Expected behavior
Some getters in Entity, LivingEntity, CreatureSpawnEvent,... for the SpawnReason of an entity are annoted as @NotNull and should return not null results, by default it seems that if a SpawnReason wasn't set it should return CreatureSpawnEvent.SpawnReason.DEFAULT value. Observed/Actual behavior
Actually sometimes these methods return null results like in the logs here:
java.lang.NullPointerException: Cannot invoke "org.bukkit.event.entity.CreatureSpawnEvent$SpawnReason.equals(Object)" because the return value of "org.bukkit.entity.LivingEntity.getEntitySpawnReason()" is null at me.elmital.customplugin.Listeners.onEntityKilled(Listeners.java:405) ~[customplugin-1.0.0.jar:?] at com.destroystokyo.paper.event.executor.asm.generated.GeneratedEventExecutor202.execute(Unknown Source) ~[?:?] at org.bukkit.plugin.EventExecutor$2.execute(EventExecutor.java:77) ~[paper-api-1.19.3-R0.1-SNAPSHOT.jar:?] at co.aikar.timings.TimedEventExecutor.execute(TimedEventExecutor.java:80) ~[paper-api-1.19.3-R0.1-SNAPSHOT.jar:git-Paper-380] at org.bukkit.plugin.RegisteredListener.callEvent(RegisteredListener.java:70) ~[paper-api-1.19.3-R0.1-SNAPSHOT.jar:?] at org.bukkit.plugin.SimplePluginManager.callEvent(SimplePluginManager.java:672) ~[paper-api-1.19.3-R0.1-SNAPSHOT.jar:?] at org.bukkit.craftbukkit.v1_19_R2.event.CraftEventFactory.callPlayerDeathEvent(CraftEventFactory.java:907) ~[paper-1.19.3.jar:git-Paper-380] at net.minecraft.server.level.ServerPlayer.die(ServerPlayer.java:908) ~[?:?] at net.minecraft.world.entity.LivingEntity.hurt(LivingEntity.java:1491) ~[?:?] at net.minecraft.world.entity.player.Player.hurt(Player.java:961) ~[?:?] at net.minecraft.server.level.ServerPlayer.hurt(ServerPlayer.java:1071) ~[?:?] at net.minecraft.world.entity.projectile.AbstractArrow.onHitEntity(AbstractArrow.java:402) ~[?:?] at net.minecraft.world.entity.projectile.Projectile.onHit(Projectile.java:182) ~[?:?] at net.minecraft.world.entity.projectile.Projectile.preOnHit(Projectile.java:173) ~[?:?] at net.minecraft.world.entity.projectile.AbstractArrow.preOnHit(AbstractArrow.java:296) ~[?:?] at net.minecraft.world.entity.projectile.AbstractArrow.tick(AbstractArrow.java:232) ~[?:?] at net.minecraft.world.entity.projectile.Arrow.tick(Arrow.java:112) ~[?:?] at net.minecraft.server.level.ServerLevel.tickNonPassenger(ServerLevel.java:1211) ~[?:?] at net.minecraft.world.level.Level.guardEntityTick(Level.java:918) ~[?:?] at net.minecraft.server.level.ServerLevel.lambda$tick$6(ServerLevel.java:731) ~[?:?] at net.minecraft.world.level.entity.EntityTickList.forEach(EntityTickList.java:42) ~[paper-1.19.3.jar:git-Paper-380] at net.minecraft.server.level.ServerLevel.tick(ServerLevel.java:711) ~[?:?] at net.minecraft.server.MinecraftServer.tickChildren(MinecraftServer.java:1535) ~[paper-1.19.3.jar:git-Paper-380] at net.minecraft.server.dedicated.DedicatedServer.tickChildren(DedicatedServer.java:440) ~[paper-1.19.3.jar:git-Paper-380] at net.minecraft.server.MinecraftServer.tickServer(MinecraftServer.java:1397) ~[paper-1.19.3.jar:git-Paper-380] at net.minecraft.server.MinecraftServer.runServer(MinecraftServer.java:1173) ~[paper-1.19.3.jar:git-Paper-380] at net.minecraft.server.MinecraftServer.lambda$spin$0(MinecraftServer.java:316) ~[paper-1.19.3.jar:git-Paper-380] at java.lang.Thread.run(Thread.java:833) ~[?:?]
### Steps/models to reproduce I experienced these bugs when players are killed and a PlayerDeathEvent listener try to get the org.bukkit.entity.LivingEntity.getEntitySpawnReason(), sometimes a NPE is thrown. ``` Java @EventHandler(priority = EventPriority.HIGH, ignoreCancelled = true) @SuppressWarnings("unused") public void onEntityKilled(EntityDeathEvent event) { if (!event.getEntity().getEntitySpawnReason().equals(CreatureSpawnEvent.SpawnReason.CUSTOM)) return; var id = event.getEntity().getPersistentDataContainer().get(idKey(),PersistentDataType.INTEGER); if (id != null) { if (event.getEntity() instanceof Mob mob) { ... } } } Plugin and Datapack List I don't think it's related to the bug but I've a Custom Core Plugin the one where logs above came from, and other publics plugins : Plugins (8): Chunky, FreedomChat, LuckPerms, PL-Hide, MyCustomPlugin, spark, WorldEdit, WorldGuard Paper version This server is running Paper version git-Paper-380 (MC: 1.19.3) (Implementing API version 1.19.3-R0.1-SNAPSHOT) (Git: 0ed4b91 <https://github.com/PaperMC/Paper/commit/0ed4b9148b72a7fb702333cf1221be6aeecff7ca> ) Code bellow use the last paper version Other It seems that since the Spigot fix https://hub.spigotmc.org/jira/browse/SPIGOT-6415, in the ServerLevel class, for entities which are teleported. The SpawnReason is passed as null while before it was with the CreatureSpawnEvent.SpawnReason.DEFAULT value. public void addDuringTeleport(Entity entity) { // CraftBukkit start // SPIGOT-6415: Don't call spawn event for entities which travel trough worlds, // since it is only an implementation detail, that a new entity is created when // they are traveling between worlds. this.addDuringTeleport(entity, null); <-- Here is the Spigot change } public void addDuringTeleport(Entity entity, CreatureSpawnEvent.SpawnReason reason) { this.addEntity(entity, reason); // CraftBukkit end } When we look at the ServerLevel#addEntity(Entity entity, CreatureSpawnEvent.SpawnReason spawnReason) a check is done for the Nullability of the SpawnReason: if (entity.spawnReason == null) entity.spawnReason = spawnReason; // Paper But the spawnReason could be null since the fix. // CraftBukkit start private boolean addEntity(Entity entity, CreatureSpawnEvent.SpawnReason spawnReason) { org.spigotmc.AsyncCatcher.catchOp("entity add"); // Spigot // Paper start if (entity.valid) { MinecraftServer.LOGGER.error("Attempted Double World add on " + entity, new Throwable()); if (DEBUG_ENTITIES) { Throwable thr = entity.addedToWorldStack; if (thr == null) { MinecraftServer.LOGGER.error("Double add entity has no add stacktrace"); } else { MinecraftServer.LOGGER.error("Double add stacktrace: ", thr); } } return true; } // Paper end if (entity.spawnReason == null) entity.spawnReason = spawnReason; // Paper if (entity.isRemoved()) { // Paper start if (DEBUG_ENTITIES) { io.papermc.paper.util.TraceUtil.dumpTraceForThread("Tried to add entity " + entity + " but it was marked as removed already"); // CraftBukkit getAddToWorldStackTrace(entity).printStackTrace(); } // Paper end // WorldServer.LOGGER.warn("Tried to add entity {} but it was marked as removed already", EntityTypes.getKey(entity.getType())); // CraftBukkit return false; } else { if (entity instanceof net.minecraft.world.entity.item.ItemEntity itemEntity && itemEntity.getItem().isEmpty()) return false; // Paper - Prevent empty items from being added // Paper start - capture all item additions to the world if (captureDrops != null && entity instanceof net.minecraft.world.entity.item.ItemEntity) { captureDrops.add((net.minecraft.world.entity.item.ItemEntity) entity); return true; } // Paper end // SPIGOT-6415: Don't call spawn event when reason is null. For example when an entity teleports to a new world. if (spawnReason != null && !CraftEventFactory.doEntityAddEventCalling(this, entity, spawnReason)) { return false; } // CraftBukkit end return this.entityLookup.addNewEntity(entity); // Paper - rewrite chunk system } } From what I've found it look to be the only place where a null value is passed, and that's a breaking change for plugin developper. I think two solutions can be applied : - All the concerned methods annoted as @NotNull should be annoted as @Nullable - Or adapt the fix with something like that maybe ? // CraftBukkit start private boolean addEntity(Entity entity, CreatureSpawnEvent.SpawnReason spawnReason) { org.spigotmc.AsyncCatcher.catchOp("entity add"); // Spigot // Paper start if (entity.valid) { MinecraftServer.LOGGER.error("Attempted Double World add on " + entity, new Throwable()); if (DEBUG_ENTITIES) { Throwable thr = entity.addedToWorldStack; if (thr == null) { MinecraftServer.LOGGER.error("Double add entity has no add stacktrace"); } else { MinecraftServer.LOGGER.error("Double add stacktrace: ", thr); } } return true; } // Paper end // Paper start - Since SPIGOT-6415 fix spawnReason may be null boolean callSpawnEvent = spawnReason != null; if (entity.spawnReason == null) { if (spawnReason == null) entity.spawnReason = CreatureSpawnEvent.SpawnReason.DEFAULT; else entity.spawnReason = spawnReason; } // Paper end if (entity.isRemoved()) { // Paper start if (DEBUG_ENTITIES) { io.papermc.paper.util.TraceUtil.dumpTraceForThread("Tried to add entity " + entity + " but it was marked as removed already"); // CraftBukkit getAddToWorldStackTrace(entity).printStackTrace(); } // Paper end // WorldServer.LOGGER.warn("Tried to add entity {} but it was marked as removed already", EntityTypes.getKey(entity.getType())); // CraftBukkit return false; } else { if (entity instanceof net.minecraft.world.entity.item.ItemEntity itemEntity && itemEntity.getItem().isEmpty()) return false; // Paper - Prevent empty items from being added // Paper start - capture all item additions to the world if (captureDrops != null && entity instanceof net.minecraft.world.entity.item.ItemEntity) { captureDrops.add((net.minecraft.world.entity.item.ItemEntity) entity); return true; } // Paper end // SPIGOT-6415: Don't call spawn event when reason is null. For example when an entity teleports to a new world. if (callSpawnEvent && !CraftEventFactory.doEntityAddEventCalling(this, entity, spawnReason)) { // Paper return false; } // CraftBukkit end return this.entityLookup.addNewEntity(entity); // Paper - rewrite chunk system } } — Reply to this email directly, view it on GitHub <https://github.com/PaperMC/Paper/issues/8828>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJMAZH7OWHNFX52W7RQO5TWV7D57ANCNFSM6AAAAAAUR25GWI> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>
elmital
Machine-Maker
Expected behavior
CreatureSpawnEvent should always return a non null value, by default CreatureSpawnEvent.SpawnReason.DEFAULT value.
Observed/Actual behavior
The new players which login for the first time on a server have a null CreatureSpawnEvent.SpawnReason while they haven't disconnected one time.
Steps/models to reproduce
Create a new server with a plugin that listen PlayerJoinEvent and PlayerQuitEvent.
Connect to the server the output is null then disconnect the output will be null too. Re-connect to the server the outputs will be DEFAULT value.
Plugin and Datapack List
Test plugin
Paper version
This server is running Paper version null (MC: 1.19.3) (Implementing API version 1.19.3-R0.1-SNAPSHOT) (Git: 4d7269e)
Other
My first assemption about the issue was totally wrong so to avoid to open a new PR for nothing I let here a fix that I've tested. If and it's okay for you I can open a PR.
Output for step model after the fix :
Here is my proposal in PlayerList class
Before :
After :