Trial Spawner and Vaults 1.21 additions#2790
Trial Spawner and Vaults 1.21 additions#2790MC-Samuel wants to merge 11 commits intoDenizenScript:devfrom
Conversation
| // | ||
| // @Location true | ||
| // | ||
| // @Triggers when a vault block state changes |
There was a problem hiding this comment.
Needs to be block's or block changes state.
Also . at the end.
paper/src/main/java/com/denizenscript/denizen/paper/events/VaultChangesStateScriptEvent.java
Show resolved
Hide resolved
| public BlockDispenseLootScriptEvent() { | ||
| registerCouldMatcher("loot dispenses from <block>"); | ||
| this.<BlockDispenseLootScriptEvent, ListTag>registerDetermination(null, ListTag.class, (evt, context, input) -> { | ||
| List<ItemStack> items = new ArrayList<>(); |
There was a problem hiding this comment.
Nitpick, but can initialize to input.size().
| @Override | ||
| public ObjectTag getContext(String name) { | ||
| return switch (name) { | ||
| case "item" -> item; |
There was a problem hiding this comment.
This has the same issue with the context potentially becoming outdated due to another plugin accessing it (e.g. if you - wait a bit in a script and then read it) - need to dynamically get the item here, but can keep the field for matches optimization.
|
|
||
| public BlockDispenseLootScriptEvent() { | ||
| registerCouldMatcher("loot dispenses from <block>"); | ||
| this.<BlockDispenseLootScriptEvent, ListTag>registerDetermination(null, ListTag.class, (evt, context, input) -> { |
There was a problem hiding this comment.
Just in general, prefixed determinations (to match their context counterparts) are usually preferred as they're easier to understand in scripts & look cleaner if the event ever gets more than one determination.
| || data instanceof Tripwire | ||
| || (NMSHandler.getVersion().isAtLeast(NMSVersion.v1_19) && (data instanceof SculkCatalyst | ||
| || data instanceof SculkShrieker)); | ||
| || data instanceof SculkShrieker)) |
| else if (isSculkCatalyst()) { | ||
| return ((SculkCatalyst) material.getModernData()).isBloom() ? "BLOOM" : "NORMAL"; // TODO: 1.19 | ||
| else if (NMSHandler.getVersion().isAtLeast(NMSVersion.v1_21) && getBlockData() instanceof TrialSpawner trialSpawner) { | ||
| return new ElementTag(trialSpawner.getTrialSpawnerState().name(), true); |
There was a problem hiding this comment.
Can use the enum constructor for these
| // | ||
| // @Location true | ||
| // | ||
| // @Triggers when a vault block's state changes. A list of states can be found at <@link url https://hub.spigotmc.org/javadocs/spigot/org/bukkit/block/data/type/TrialSpawner.State.html>. |
| import org.bukkit.event.EventHandler; | ||
| import org.bukkit.event.Listener; | ||
|
|
||
| public class VaultChangeStateScriptEvent extends BukkitScriptEvent implements Listener { |
There was a problem hiding this comment.
Nitpick, but the event is called changes and this says change.
| if (!runInCheck(path, location)) { | ||
| return false; | ||
| } | ||
| if (!path.tryArgObject(2, new ItemTag(event.getDisplayItem()))) { |
There was a problem hiding this comment.
This could be put in a field for matches optimization (getContext should still access it directly though).
plugin/src/main/java/com/denizenscript/denizen/objects/properties/entity/EntityItem.java
Show resolved
Hide resolved
| this.<VaultDisplayItemScriptEvent, ItemTag>registerDetermination(null, ItemTag.class, (evt, context, input) -> { | ||
| evt.event.setDisplayItem(input.getItemStack()); | ||
| }); |
There was a problem hiding this comment.
Generally prefixed determinations are preferred - easier to read/work with in code, and easier to manage for us when more get added.
| // @Triggers when a block dispenses loot containing multiple items. | ||
| // | ||
| // @Context | ||
| // <context.loot> returns a ListTag(ItemTag) of outcome items. |
There was a problem hiding this comment.
outcome here sounds a little weird imo? I'd just say loot items maybe, or something like items being dispensed if you want to be more explicit.
| import java.util.ArrayList; | ||
| import java.util.List; | ||
|
|
||
| public class BlockDispenseLootScriptEvent extends BukkitScriptEvent implements Listener { |
There was a problem hiding this comment.
I believe the class name usually matches the Denizen name over the bukkit name, so that it's easier to spot in code for us.
| } | ||
| else if (isEnderSignal()) { | ||
| return new ItemTag(getEnderSignal().getItem()); | ||
| else if (NMSHandler.getVersion().isAtLeast(NMSVersion.v1_19) && getEntity() instanceof ItemDisplay itemDisplay) { // TODO: 1.19: when 1.19 is minimum, make a 'getDisplay' |
| if (item.asType(EntityTag.class, mechanism.context).isCitizensNPC()) { | ||
| item.asType(EntityTag.class, mechanism.context).getDenizenNPC().getCitizen().data().setPersistent(NPC.Metadata.ITEM_ID, item.getItemStack().getType().name()); | ||
| } |
There was a problem hiding this comment.
Uh, did you test this? You're referencing item because the old code had the entity under item, but in this case I'm pretty sure this would throw an NPE right away - you'd want to use object here (I.e. the property classes' entity object).
There was a problem hiding this comment.
Also can just use ItemTag#getBukkitMaterial here I think
| import java.util.ArrayList; | ||
| import java.util.List; | ||
|
|
||
| public class BlockDispensesLootScriptEvent extends BukkitScriptEvent implements Listener { |
There was a problem hiding this comment.
Event class name should match Denizen name.
Replaces #2771
Additions:
Updates:
Some of this was copied from @MrMaleficus's earlier PR, so credit to them for the events.