Skip to content

Commit

Permalink
bug fix and optimizations in NPC skins
Browse files Browse the repository at this point in the history 
fixed client sometimes disconnects for: Internal Exception: io.netty.handler.codec.DecoderException: java.io.IOException: Packet 0/56 (ki) was larger than I expected, found 917 bytes extra whilst reading packet 56;
changes in Skin#setNPCTexture

Allow retrieving Skin instance by skin name instead of entity so it can be retrieved without a spawned entity instance. Changes in Skin and SkinPacketTracker.

add SkinPacketTracker#onSpawnNPC

remove redundant scheduled skin update tasks in CitizensNPC and EventListen; provide no benefit

reset current SkinUpdateTracker instance if exists instead of creating a new one for all players in EventListen#onCitizensReload

reset rotation count in EventListen when player teleports/changes world/etc

reset update trackers for players nearby NPC's that spawn in case they are not looking at the NPC.
  • Loading branch information
@JCThePants
JCThePants committed Sep 4, 2015
1 parent 20816de commit 80ee5f1
Show file tree
Hide file tree
Showing 4 changed files with 140 additions and 53 deletions.
103 changes: 78 additions & 25 deletions src/main/java/net/citizensnpcs/EventListen.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
import net.citizensnpcs.api.event.NPCDespawnEvent;
import net.citizensnpcs.api.event.NPCLeftClickEvent;
import net.citizensnpcs.api.event.NPCRightClickEvent;
import net.citizensnpcs.api.event.NPCSpawnEvent;
import net.citizensnpcs.api.event.PlayerCreateNPCEvent;
import net.citizensnpcs.api.npc.NPC;
import net.citizensnpcs.api.npc.NPCRegistry;
Expand Down Expand Up @@ -81,6 +82,7 @@
import org.bukkit.event.world.WorldUnloadEvent;
import org.bukkit.inventory.meta.SkullMeta;
import org.bukkit.scheduler.BukkitRunnable;
import org.bukkit.scheduler.BukkitTask;
import org.bukkit.scoreboard.Team;

public class EventListen implements Listener {
Expand Down Expand Up @@ -322,6 +324,16 @@ public void onNeedsRespawn(NPCNeedsRespawnEvent event) {
toRespawn.put(coord, event.getNPC());
}

@EventHandler
public void onNPCSpawn(NPCSpawnEvent event) {
SkinnableEntity skinnable = NMS.getSkinnable(event.getNPC().getEntity());
if (skinnable == null)
return;

// reset nearby players in case they are not looking at the NPC when it spawns.
resetNearbyPlayers(skinnable);
}

@EventHandler
public void onNPCDespawn(NPCDespawnEvent event) {
if (event.getReason() == DespawnReason.PLUGIN || event.getReason() == DespawnReason.REMOVAL) {
Expand Down Expand Up @@ -441,7 +453,6 @@ public void onWorldUnload(WorldUnloadEvent event) {
// a player moves a certain distance from their last position.
@EventHandler(priority = EventPriority.MONITOR, ignoreCancelled = true)
public void onPlayerMove(final PlayerMoveEvent event) {

SkinUpdateTracker updateTracker = skinUpdateTrackers.get(event.getPlayer().getUniqueId());
if (updateTracker == null)
return;
Expand All @@ -460,17 +471,25 @@ public void onCitizensReload(CitizensReloadEvent event) {
if (player.hasMetadata("NPC"))
continue;

skinUpdateTrackers.put(player.getUniqueId(), new SkinUpdateTracker(player));
SkinUpdateTracker tracker = skinUpdateTrackers.get(player.getUniqueId());
if (tracker == null)
continue;

tracker.hardReset(player);
}
}

public void recalculatePlayer(final Player player, long delay, final boolean isInitial) {

public void recalculatePlayer(final Player player, long delay, boolean reset) {
if (player.hasMetadata("NPC"))
return;

if (isInitial) {
skinUpdateTrackers.put(player.getUniqueId(), new SkinUpdateTracker(player));
SkinUpdateTracker tracker = skinUpdateTrackers.get(player.getUniqueId());
if (tracker == null) {
tracker = new SkinUpdateTracker(player);
skinUpdateTrackers.put(player.getUniqueId(), tracker);
}
else if (reset) {
tracker.hardReset(player);
}

new BukkitRunnable() {
Expand All @@ -480,20 +499,42 @@ public void run() {

List<SkinnableEntity> nearbyNPCs = getNearbySkinnableNPCs(player);
for (SkinnableEntity npc : nearbyNPCs) {
npc.getSkinTracker().updateViewer(player);
}

if (!nearbyNPCs.isEmpty() && isInitial) {
// one more time to help when resource pack load times
// prevent immediate skin loading
recalculatePlayer(player, 40, false);
npc.getSkinTracker().updateViewer(player);
}
}
}.runTaskLater(CitizensAPI.getPlugin(), delay);
}

private List<SkinnableEntity> getNearbySkinnableNPCs(Player player) {
// hard reset skin update trackers for players near a skinnable NPC
private void resetNearbyPlayers(SkinnableEntity skinnable) {
Entity entity = skinnable.getBukkitEntity();
if (entity == null || !entity.isValid())
return;

double viewDistance = Setting.NPC_SKIN_VIEW_DISTANCE.asDouble();
viewDistance *= viewDistance;
Location location = entity.getLocation(NPC_LOCATION);
List<Player> players = entity.getWorld().getPlayers();
for (Player player : players) {
if (player.hasMetadata("NPC"))
continue;

double distanceSquared = player.getLocation(CACHE_LOCATION).distanceSquared(location);
if (distanceSquared > viewDistance)
continue;

SkinUpdateTracker tracker = skinUpdateTrackers.get(player.getUniqueId());
if (tracker == null) {
tracker = new SkinUpdateTracker(player);
skinUpdateTrackers.put(player.getUniqueId(), tracker);
}
else {
tracker.hardReset(player);
}
}
}

private List<SkinnableEntity> getNearbySkinnableNPCs(Player player) {
List<SkinnableEntity> results = new ArrayList<SkinnableEntity>();

double viewDistance = Setting.NPC_SKIN_VIEW_DISTANCE.asDouble();
Expand All @@ -506,7 +547,7 @@ private List<SkinnableEntity> getNearbySkinnableNPCs(Player player) {
&& player.canSee((Player) npcEntity)
&& player.getWorld().equals(npcEntity.getWorld())
&& player.getLocation(CACHE_LOCATION)
.distanceSquared(npc.getStoredLocation()) < viewDistance) {
.distanceSquared(npc.getStoredLocation()) < viewDistance) {

SkinnableEntity skinnable = NMS.getSkinnable(npcEntity);

Expand Down Expand Up @@ -600,21 +641,25 @@ public int hashCode() {
}

private class SkinUpdateTracker {
float initialYaw;
final Location location = new Location(null, 0, 0, 0);
int rotationCount;
boolean hasMoved;
float upperBound;
float lowerBound;

SkinUpdateTracker(Player player) {
reset(player);
hardReset(player);
}

boolean shouldUpdate(Player player) {
Location currentLoc = player.getLocation(CACHE_LOCATION);

Location currentLoc = player.getLocation(YAW_LOCATION);
if (!hasMoved) {
hasMoved = true;
return true;
}

if (rotationCount < 2) {
if (rotationCount < 3) {
float yaw = NMS.clampYaw(currentLoc.getYaw());
boolean hasRotated = upperBound < lowerBound
? yaw > upperBound && yaw < lowerBound
Expand Down Expand Up @@ -643,15 +688,23 @@ boolean shouldUpdate(Player player) {
// resets initial yaw and location to the players
// current location and yaw.
void reset(Player player) {
player.getLocation(location);
this.initialYaw = NMS.clampYaw(location.getYaw());
float rotationDegrees = Setting.NPC_SKIN_ROTATION_UPDATE_DEGREES.asFloat();
this.upperBound = NMS.clampYaw(this.initialYaw + rotationDegrees);
this.lowerBound = NMS.clampYaw(this.initialYaw - rotationDegrees);
player.getLocation(this.location);
if (rotationCount < 3) {
float rotationDegrees = Setting.NPC_SKIN_ROTATION_UPDATE_DEGREES.asFloat();
float yaw = NMS.clampYaw(this.location.getYaw());
this.upperBound = NMS.clampYaw(yaw + rotationDegrees);
this.lowerBound = NMS.clampYaw(yaw - rotationDegrees);
}
}

void hardReset(Player player) {
this.hasMoved = false;
this.rotationCount = 0;
reset(player);
}
}

private static final Location YAW_LOCATION = new Location(null, 0, 0, 0);
private static final Location CACHE_LOCATION = new Location(null, 0, 0, 0);
private static final Location NPC_LOCATION = new Location(null, 0, 0, 0);
private static final int MOVEMENT_SKIN_UPDATE_DISTANCE = 50 * 50;
}
22 changes: 5 additions & 17 deletions src/main/java/net/citizensnpcs/npc/CitizensNPC.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -189,23 +189,11 @@ public boolean spawn(Location at) {
boolean couldSpawn = !Util.isLoaded(at) ? false : mcEntity.world.addEntity(mcEntity, SpawnReason.CUSTOM);

// send skin packets, if applicable, before other NMS packets are sent
SkinnableEntity skinnable = NMS.getSkinnable(getEntity());
if (skinnable != null) {
final double viewDistance = Settings.Setting.NPC_SKIN_VIEW_DISTANCE.asDouble();
//skinnable.getSkinTracker().updateNearbyViewers(viewDistance);
Bukkit.getScheduler().scheduleSyncDelayedTask(CitizensAPI.getPlugin(), new Runnable() {
@Override
public void run() {
if (getEntity() == null || !getEntity().isValid())
return;

SkinnableEntity npc = NMS.getSkinnable(getEntity());
if (npc == null)
return;

npc.getSkinTracker().updateNearbyViewers(viewDistance);
}
}, 20);
if (couldSpawn) {
SkinnableEntity skinnable = NMS.getSkinnable(getEntity());
if (skinnable != null) {
skinnable.getSkinTracker().onSpawnNPC();
}
}

mcEntity.setPositionRotation(at.getX(), at.getY(), at.getZ(), at.getYaw(), at.getPitch());
Expand Down
45 changes: 36 additions & 9 deletions src/main/java/net/citizensnpcs/npc/skin/Skin.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,6 @@ private void setData(@Nullable GameProfile profile) {
}

private void fetch() {

final int maxRetries = Setting.MAX_NPC_SKIN_RETRIES.asInt();
if (maxRetries > -1 && fetchRetries >= maxRetries) {
if (Messaging.isDebugging()) {
Expand Down Expand Up @@ -215,7 +214,7 @@ public void run() {
}, delay);

if (Messaging.isDebugging()) {
Messaging.debug("Retrying skin fetch for '" + skinName + "' in " + delay + "ticks.");
Messaging.debug("Retrying skin fetch for '" + skinName + "' in " + delay + " ticks.");
}
break;
case SUCCESS:
Expand All @@ -228,19 +227,19 @@ public void run() {
}

/**
* Get a skin for a skinnable entity.
* Get a player skin.
*
* <p>
* If a Skin instance does not exist, a new one is created and the skin data is automatically fetched.
* </p>
*
* @param entity
* The skinnable entity.
* @param skinName
* The name of the skin.
*/
public static Skin get(SkinnableEntity entity) {
Preconditions.checkNotNull(entity);
public static Skin get(String skinName) {
Preconditions.checkNotNull(skinName);

String skinName = entity.getSkinName().toLowerCase();
skinName = skinName.toLowerCase();

Skin skin;
synchronized (CACHE) {
Expand All @@ -254,6 +253,23 @@ public static Skin get(SkinnableEntity entity) {
return skin;
}

/**
* Get a skin for a skinnable entity.
*
* <p>
* If a Skin instance does not exist, a new one is created and the skin data is automatically fetched.
* </p>
*
* @param entity
* The skinnable entity.
*/
public static Skin get(SkinnableEntity entity) {
Preconditions.checkNotNull(entity);

String skinName = entity.getSkinName().toLowerCase();
return get(skinName);
}

/**
* Clear all cached skins.
*/
Expand All @@ -272,7 +288,8 @@ public static void clearCache() {
private static void setNPCSkinData(SkinnableEntity entity, String skinName, UUID skinId, Property skinProperty) {
NPC npc = entity.getNPC();

// cache skins for faster initial skin availability
// cache skins for faster initial skin availability and
// for use when the latest skin is not required.
npc.data().setPersistent(CACHED_SKIN_UUID_NAME_METADATA, skinName);
npc.data().setPersistent(CACHED_SKIN_UUID_METADATA, skinId.toString());
if (skinProperty.getValue() != null) {
Expand All @@ -287,6 +304,16 @@ private static void setNPCSkinData(SkinnableEntity entity, String skinName, UUID

private static void setNPCTexture(SkinnableEntity entity, Property skinProperty) {
GameProfile profile = entity.getProfile();

// don't set property if already set since this sometimes causes
// packet errors that disconnect the client.
Property current = Iterables.getFirst(profile.getProperties().get("textures"), null);
if (current != null
&& current.getValue().equals(skinProperty.getValue())
&& current.getSignature().equals(skinProperty.getSignature())) {
return;
}

profile.getProperties().removeAll("textures"); // ensure client does not crash due to duplicate properties.
profile.getProperties().put("textures", skinProperty);
}
Expand Down
23 changes: 21 additions & 2 deletions src/main/java/net/citizensnpcs/npc/skin/SkinPacketTracker.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,14 @@
import java.util.Map;
import java.util.UUID;

import net.citizensnpcs.npc.CitizensNPC;
import org.bukkit.Bukkit;
import org.bukkit.Location;
import org.bukkit.entity.Player;
import org.bukkit.event.EventHandler;
import org.bukkit.event.Listener;
import org.bukkit.event.player.PlayerQuitEvent;
import org.bukkit.scheduler.BukkitRunnable;
import org.bukkit.scheduler.BukkitTask;

import com.google.common.base.Preconditions;
Expand Down Expand Up @@ -98,6 +100,23 @@ public void notifySkinChange() {
skin.applyAndRespawn(entity);
}

/**
* Invoke when the NPC entity is spawned.
*/
public void onSpawnNPC() {
isRemoved = false;
new BukkitRunnable() {
@Override
public void run() {
if (!entity.getNPC().isSpawned())
return;

double viewDistance = Settings.Setting.NPC_SKIN_VIEW_DISTANCE.asDouble();
updateNearbyViewers(viewDistance);
}
}.runTaskLater(CitizensAPI.getPlugin(), 20);
}

/**
* Invoke when the NPC entity is removed.
*
Expand Down Expand Up @@ -164,11 +183,11 @@ public void updateNearbyViewers(double radius) {
Player from = entity.getBukkitEntity();
Location location = from.getLocation();

for (Player player : Bukkit.getServer().getOnlinePlayers()) {
for (Player player : world.getPlayers()) {
if (player == null || player.hasMetadata("NPC"))
continue;

if (world != player.getWorld() || !player.canSee(from))
if (!player.canSee(from))
continue;

if (location.distanceSquared(player.getLocation(CACHE_LOCATION)) > radius)
Expand Down

0 comments on commit 80ee5f1

Please sign in to comment.