fixes and stuff #34
fixes and stuff #34
Conversation
…available) fix unnecessary chunk loads/unloads being checked fix item displays when creating or breaking double chests fix cancelling item despawns creating a new item without metadata (dupe issue for plugins that check for this) minor random performance improvements
| import org.bukkit.ChunkSnapshot; | ||
| import org.bukkit.Location; | ||
| import org.bukkit.World; | ||
| import org.bukkit.*; |
There was a problem hiding this comment.
Any possibility you can tell your IDE not to do this? Otherwise I can just fix it before I merge then.
| @@ -82,7 +80,7 @@ private void saveCache() | |||
| } | |||
| } | |||
|
|
|||
| @EventHandler | |||
| @EventHandler(ignoreCancelled = true, priority = EventPriority.MONITOR) | |||
There was a problem hiding this comment.
ChunkLoadEvent isn't cancelable so the first param is useless. Have you been able to determine what's going on here? And since the plugin is doing stuff to the chunk (albeit asynchronously) it really should be at highest priority at the max instead of monitor.
There was a problem hiding this comment.
you can change if you want
| @@ -157,7 +157,7 @@ private void onChunkUnload(ChunkUnloadEvent event) | |||
| while (locations.hasNext()) //can optimize later via mapping chunks if needed | |||
| { | |||
| Location location = locations.next(); | |||
| if (location.getChunk() == event.getChunk()) | |||
| if (location.getBlockX() >> 4 == event.getChunk().getX() && location.getBlockZ() >> 4 == event.getChunk().getZ()) | |||
There was a problem hiding this comment.
What's up here? Is it possible for there to be two different Chunks to exist on getChunk which correspond to the same world/chunk coordinate?
There was a problem hiding this comment.
might just be leftover, was just making sure
| } | ||
| }.runTask(plugin); | ||
| }.runTaskLater(plugin, 1L); |
There was a problem hiding this comment.
What's up with the runnable here, are doublechests not being properly handled anymore?
Also, runTask(plugin) is equivalent to runTaskLater(plugin, 1)
There was a problem hiding this comment.
didnt seem to be ever handled for when they were made or broke
There was a problem hiding this comment.
Ok, so what you actually mean is you respawn the shop item when the doublechest is created instead of just having it despawn (I guess I was under the impression that I had another method cover this - that, or it's that I wasn't able to determine if the new doublechest is supposed to be a shop or not. The left side overrides, so the resulting doublechest may not be a shop, and thus the newly-spawned item won't be able to be despawned until the chunk is unloaded.)
| public void run() { | ||
| spawnItem(new ShopInfo(shopAPI.getLocation(container), shopAPI.getItemStack(container), shopAPI.getPrice(container))); | ||
| } | ||
| }.runTaskLater(plugin, 1L); |
There was a problem hiding this comment.
Um, isn't the idea to despawn the item, not attempt to spawn another??
There was a problem hiding this comment.
Um, then I'm not following why you made this change then.
There was a problem hiding this comment.
if you just cancel the despawn event (which is what was done before this change) it just creates a new item without the metadata tag
There was a problem hiding this comment.
Ok, I don't think I'm following what's going on - I guess I'll have to review the code.
Perhaps this may help me better understand: what did you change, and why did you make this change?
|
There's a lot of stuff going on in this PR, would you mind if I broke it up into separate chunks? I'll probably try cherry-picking onto different branches here. |
|
Forgot about this PR, lol. Anyways, the inventory rows issue has been fixed by performing "round-up" integer division. Unsure what's going on with the rest. I believe one of them is avoiding use of location#getChunk to avoid loading chunks, which I'm not sure if I recall has also been resolved at some point. |
|
there also were issues with items getting spawned that were missing the no pickup meta so they could be duped |
|
Ah I see. Interesting that canceling despawnitemevent would result in that behavior... |
fix error when trying to create a inv with over 54 slots (buying max available)
fix unnecessary chunk loads/unloads being checked
fix item displays when creating or breaking double chests
fix cancelling item despawns creating a new item without metadata (dupe issue for plugins that check for this)
minor random performance improvements
all the work for this PR stemmed from a dupe issue from a plugin that picks up item drops somehow able to still pick up shop items despite checking for the metadata tag, tested for a few days now think all is good