From 4d8a84c459419d0f584bb547d8f6cdad56bfcd42 Mon Sep 17 00:00:00 2001 From: Nassim Jahnke <nassim@njahnke.dev> Date: Sat, 9 Nov 2024 21:17:42 +0100 Subject: [PATCH] Improve CraftEntity and CraftPlayer equals --- patches/server/Fix-CraftEntity-hashCode.patch | 46 -------- ...ity-and-CraftPlayer-equals-and-hashC.patch | 103 ++++++++++++++++++ 2 files changed, 103 insertions(+), 46 deletions(-) delete mode 100644 patches/server/Fix-CraftEntity-hashCode.patch create mode 100644 patches/server/Improve-CraftEntity-and-CraftPlayer-equals-and-hashC.patch diff --git a/patches/server/Fix-CraftEntity-hashCode.patch b/patches/server/Fix-CraftEntity-hashCode.patch deleted file mode 100644 index 7b82d84c7d..0000000000 --- a/patches/server/Fix-CraftEntity-hashCode.patch +++ /dev/null @@ -1,46 +0,0 @@ -From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 -From: Aikar <aikar@aikar.co> -Date: Sun, 10 Jun 2018 20:20:15 -0400 -Subject: [PATCH] Fix CraftEntity hashCode - -hashCodes are not allowed to change, however bukkit used a value -that does change, the entityId. - -When an entity is teleported dimensions, the entity reference is -replaced with a new one with a new entity ID. - -For hashCode, we can simply use the UUID's hashCode to keep -the hashCode from changing. - -equals() is ok to use getEntityId() because equals() should only -be true if both the left and right are the same reference. - -Since entity ids can not duplicate during runtime, this -check is essentially the same as this.getHandle() == other.getHandle() - -However, replaced it too to make it clearer of intent. - -diff --git a/src/main/java/org/bukkit/craftbukkit/entity/CraftEntity.java b/src/main/java/org/bukkit/craftbukkit/entity/CraftEntity.java -index 0000000000000000000000000000000000000000..0000000000000000000000000000000000000000 100644 ---- a/src/main/java/org/bukkit/craftbukkit/entity/CraftEntity.java -+++ b/src/main/java/org/bukkit/craftbukkit/entity/CraftEntity.java -@@ -0,0 +0,0 @@ public abstract class CraftEntity implements org.bukkit.entity.Entity { - return false; - } - final CraftEntity other = (CraftEntity) obj; -- return (this.getEntityId() == other.getEntityId()); -+ return (this.getHandle() == other.getHandle()); // Paper - while logically the same, this is clearer - } - -+ // Paper - Fix hashCode. entity ID's are not static. -+ // A CraftEntity can change reference to a new entity with a new ID, and hash codes should never change - @Override - public int hashCode() { -- int hash = 7; -- hash = 29 * hash + this.getEntityId(); -- return hash; -+ return getUniqueId().hashCode(); -+ // Paper end - } - - @Override diff --git a/patches/server/Improve-CraftEntity-and-CraftPlayer-equals-and-hashC.patch b/patches/server/Improve-CraftEntity-and-CraftPlayer-equals-and-hashC.patch new file mode 100644 index 0000000000..e03fc2823f --- /dev/null +++ b/patches/server/Improve-CraftEntity-and-CraftPlayer-equals-and-hashC.patch @@ -0,0 +1,103 @@ +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 +From: Nassim Jahnke <nassim@njahnke.dev> +Date: Sat, 9 Nov 2024 21:10:45 +0100 +Subject: [PATCH] Improve CraftEntity and CraftPlayer equals and hashCode + +Make sure the hash code does not change and also remove outdated +equals logic from CraftPlayer. Long-term, the override there should +be entirely removed, but this is good enough for now. + +Replacing some getHandle method calls with direct field access will +also reduce overhead from casts that the overridden methods come with, +at least until those are changed later on as well. + +diff --git a/src/main/java/org/bukkit/craftbukkit/entity/CraftEntity.java b/src/main/java/org/bukkit/craftbukkit/entity/CraftEntity.java +index 0000000000000000000000000000000000000000..0000000000000000000000000000000000000000 100644 +--- a/src/main/java/org/bukkit/craftbukkit/entity/CraftEntity.java ++++ b/src/main/java/org/bukkit/craftbukkit/entity/CraftEntity.java +@@ -0,0 +0,0 @@ public abstract class CraftEntity implements org.bukkit.entity.Entity { + + @Override + public UUID getUniqueId() { +- return this.getHandle().getUUID(); ++ return this.entity.getUUID(); + } + + @Override +@@ -0,0 +0,0 @@ public abstract class CraftEntity implements org.bukkit.entity.Entity { + + @Override + public boolean equals(Object obj) { +- if (obj == null) { +- return false; +- } +- if (this.getClass() != obj.getClass()) { +- return false; +- } ++ if (this == obj) return true; ++ if (obj == null || getClass() != obj.getClass()) return false; ++ + final CraftEntity other = (CraftEntity) obj; +- return (this.getEntityId() == other.getEntityId()); ++ return this.entity == other.entity; // There should never be duplicate entities with differing references + } + + @Override + public int hashCode() { +- int hash = 7; +- hash = 29 * hash + this.getEntityId(); +- return hash; ++ // The UUID and thus hash code should never change (unlike the entity id) ++ return this.getUniqueId().hashCode(); + } + + @Override +diff --git a/src/main/java/org/bukkit/craftbukkit/entity/CraftPlayer.java b/src/main/java/org/bukkit/craftbukkit/entity/CraftPlayer.java +index 0000000000000000000000000000000000000000..0000000000000000000000000000000000000000 100644 +--- a/src/main/java/org/bukkit/craftbukkit/entity/CraftPlayer.java ++++ b/src/main/java/org/bukkit/craftbukkit/entity/CraftPlayer.java +@@ -0,0 +0,0 @@ public class CraftPlayer extends CraftHumanEntity implements Player { + + @Override + public boolean equals(Object obj) { +- if (!(obj instanceof OfflinePlayer)) { +- return false; +- } +- OfflinePlayer other = (OfflinePlayer) obj; +- if ((this.getUniqueId() == null) || (other.getUniqueId() == null)) { +- return false; +- } +- +- boolean uuidEquals = this.getUniqueId().equals(other.getUniqueId()); +- boolean idEquals = true; +- +- if (other instanceof CraftPlayer) { +- idEquals = this.getEntityId() == ((CraftPlayer) other).getEntityId(); +- } +- +- return uuidEquals && idEquals; ++ // Long-term, this should just use the super equals... for now, check the UUID ++ if (obj == this) return true; ++ if (!(obj instanceof OfflinePlayer other)) return false; ++ return this.getUniqueId().equals(other.getUniqueId()); + } + + @Override +@@ -0,0 +0,0 @@ public class CraftPlayer extends CraftHumanEntity implements Player { + + @Override + public boolean canSee(org.bukkit.entity.Entity entity) { +- return this.equals(entity) || entity.isVisibleByDefault() ^ this.invertedVisibilityEntities.containsKey(entity.getUniqueId()); // SPIGOT-7312: Can always see self ++ return this.getUniqueId().equals(entity.getUniqueId()) || entity.isVisibleByDefault() ^ this.invertedVisibilityEntities.containsKey(entity.getUniqueId()); // SPIGOT-7312: Can always see self + } + + public boolean canSeePlayer(UUID uuid) { +@@ -0,0 +0,0 @@ public class CraftPlayer extends CraftHumanEntity implements Player { + @Override + public int hashCode() { + if (this.hash == 0 || this.hash == 485) { +- this.hash = 97 * 5 + (this.getUniqueId() != null ? this.getUniqueId().hashCode() : 0); ++ this.hash = 97 * 5 + this.getUniqueId().hashCode(); + } + return this.hash; + }