10 block event modernizations #2774
Conversation
plugin/src/main/java/com/denizenscript/denizen/events/block/CauldronLevelChangeScriptEvent.java
Show resolved
Hide resolved
| this.<CauldronLevelChangeScriptEvent, ElementTag>registerOptionalDetermination(null, ElementTag.class, (evt, context, level) -> { | ||
| if (level.isInt()) { | ||
| evt.newLevel = level.asInt(); | ||
| ((Levelled) evt.event.getNewState().getBlockData()).setLevel(newLevel); |
There was a problem hiding this comment.
Have you tested this with e.g. an empty cauldron getting a level or a cauldron emptying? I believe the point of the deprecation was that while lava/water_cauldron are levelled, the base cauldron isn't.
| String changeType = path.eventArgLowerAt(2); | ||
| if (changeType.equals("raises")) { | ||
| if (event.getNewLevel() <= event.getOldLevel()) { | ||
| if (path.eventArgLowerAt(2).equals("raises")) { |
There was a problem hiding this comment.
Is there a reason you replaced the variable with calling eventArgLowerAt for every check?
| public FurnaceBurnsItemScriptEvent() { | ||
| registerCouldMatcher("furnace burns <item>"); | ||
| this.<FurnaceBurnsItemScriptEvent, DurationTag>registerDetermination(null, DurationTag.class, (evt, context, time) -> { | ||
| evt.event.setBurnTime(time.getTicksAsInt()); |
There was a problem hiding this comment.
I believe this is a breaking change? Previously inputting just a number would go through the ElementTag handling and be set as ticks, now it'll be converted into a DurationTag and be set as seconds (unless setBurnTime is in seconds instead of ticks for some reason).
| if (level.asInt() > 3) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
You call asInt several times here, should go in a variable right after the int check.
Also this is an early return, I'd put it before all the logic & right after the int check as well.
| } | ||
| Material cauldronType = cauldronState.getType(); | ||
| if (cauldronType != Material.WATER_CAULDRON && cauldronType != Material.LAVA_CAULDRON) { | ||
| cauldronState.setType(event.getBlock().getType()); |
There was a problem hiding this comment.
I'd store the type once the event fires instead of getting it dynamically here (like I did in the original example iirc) - otherwise it could lead to weird situations where some script runs a determination after the fact and the block state instance gets set to something weird (and maybe breaks another plugin that's using it or whatever).
Not a very likely scenario, but still probably better to make sure the types we set here are the correct ones & also avoid reading the block type every determination performance wise.
| return switch (name) { | ||
| case "location" -> location; | ||
| case "cause" -> new ElementTag(event.getReason()); | ||
| case "old_level" -> new ElementTag(event.getBlock().getBlockData() instanceof Levelled levelled ? levelled.getLevel() : 0); |
There was a problem hiding this comment.
Same here, need to store the old level otherwise it'd keep reading the current level from the world and the context would change as the block changes (in e.g. an after event).
Can probably just store the old BlockData and use it for both this and the determination? Or storing an int and a Material separately would be fine as well.
| public FurnaceBurnsItemScriptEvent() { | ||
| registerCouldMatcher("furnace burns <item>"); | ||
| this.<FurnaceBurnsItemScriptEvent, ObjectTag>registerOptionalDetermination(null, ObjectTag.class, (evt, context, time) -> { | ||
| if (time instanceof ElementTag elementTag && elementTag.isInt()) { |
There was a problem hiding this comment.
Include a small comment here because it's a bit unclear why this special case exists (// Backwards compatibility for non-duration tick input or something)
|
fixed |
| if (cauldronType != Material.WATER_CAULDRON && cauldronType != Material.LAVA_CAULDRON) { | ||
| cauldronState.setType(cauldronType); | ||
| } |
There was a problem hiding this comment.
Have you tested this after the changes? I believe it's meant to be checking the new cauldron type, not the old type from before the event.
3 participants
#getOldLeveland#getNewLevel, modernized determination format, minor getContext format change