Knockback #2094
Knockback #2094
Conversation
| import org.bukkit.event.Listener; | ||
| import org.bukkit.util.Vector; | ||
|
|
||
| public class EntityKnockbackByEntityScriptEvent extends BukkitScriptEvent implements Listener { |
There was a problem hiding this comment.
Stray doublespace ^
You should enable automatic formatting in IntelliJ btw, helps with this stuff
| // @Regex ^on [^\s]+ knocks back [^\s]+$ | ||
| // | ||
| // @Switch in:<area> to only process the event if it occurred within a specified area. | ||
| // |
There was a problem hiding this comment.
Remove the blank line separating two switches
| // | ||
| // @Context | ||
| // <context.entity> returns the EntityTag that was knocked back. | ||
| // <context.attacker> returns the EntityTag of the one who knocked. |
There was a problem hiding this comment.
This should be damager, to align with existing events
| else if (name.equals("attacker")) { | ||
| return hitBy.getDenizenObject(); | ||
| } | ||
| return super.getContext(name); |
There was a problem hiding this comment.
You forgot to implement acceleration context
| (entity.isPlayer() ? entity.getDenizenPlayer() : null), | ||
|
|
||
| hitBy.isCitizensNPC() ? hitBy.getDenizenNPC() : | ||
| (entity.isCitizensNPC() ? EntityTag.getNPCFrom(event.getHitBy()) : null) |
There was a problem hiding this comment.
This was a mistake in existing events you referenced, please apply a matching correction to it a49f134
EDIT: Actually you used the wrong entity anyway, so the line still does contain a mistake on your own part
| public EntityKnockbackByEntityEvent event; | ||
|
|
||
| @Override | ||
| public boolean couldMatch(ScriptContainer scriptContainer, String s) { |
There was a problem hiding this comment.
You referenced existing entity events for this. Existing entity events have not yet been updated to a new method that all other events have. Please use the new (ScriptPath path) method
| public EntityTag entity; | ||
| public EntityTag hitBy; | ||
| public ItemTag held; | ||
| public LocationTag acceleration; |
There was a problem hiding this comment.
This value has no reason to be tracked as a field - reference any of the semi-recent event cleanup commits, such as here: 86682fc#diff-3adfbdf9af2fb8177b5756a6594aafaaL50
| if (!runWithCheck(path, held)) { | ||
| return false; | ||
| } | ||
|
|
There was a problem hiding this comment.
Lots of stray newlines here, should all be removed (within this method)
| // | ||
| // --> | ||
|
|
||
| public EntityKnockbackByEntityScriptEvent() { |
There was a problem hiding this comment.
The class name should probably be closer to the Denizen name, not an odd copy of the underlying Paper name. EntityKnocksbackEntityScriptEvent
|
|
||
| @Override | ||
| public String getName() { | ||
| return "EntityKnocksbackEntityEvent"; |
There was a problem hiding this comment.
The word Event should not appear here
| @Override | ||
| public boolean applyDetermination(ScriptPath path, ObjectTag determinationObj) { | ||
| if (determinationObj instanceof LocationTag) { | ||
| acceleration = new LocationTag(((LocationTag) determinationObj).toVector()); |
There was a problem hiding this comment.
As you mentioned you were aware of on Discord, this line doesn't actually do anything. This will be naturally fixed by the field removal I mentioned above
| public EntityKnockbackByEntityEvent event; | ||
|
|
||
| @Override | ||
| public boolean couldMatch(ScriptContainer scriptContainer, String s) { |
There was a problem hiding this comment.
Original issue here still not corrected
| if (!runWithCheck(path, held)) { | ||
| return false; | ||
| } | ||
|
|
There was a problem hiding this comment.
Original issue here still not resolved
|
|
||
| @Override | ||
| public boolean applyDetermination(ScriptPath path, ObjectTag determinationObj) { | ||
| if (determinationObj instanceof LocationTag) { |
There was a problem hiding this comment.
Not a reliable check, the object is not guaranteed to be of the expected type when fed in, LocationTag.matches + valueOf are required.
Also, !isDefaultDetermination
| public ObjectTag getContext(String name) { | ||
| if (name.equals("entity")) { | ||
| return entity.getDenizenObject(); | ||
| } else if (name.equals("damager")) { |
There was a problem hiding this comment.
newline between } and else, not on the same line.
|
Ok, I think I got it this time! |
| public boolean applyDetermination(ScriptPath path, ObjectTag determinationObj) { | ||
| String determination = determinationObj.toString(); | ||
| if (!isDefaultDetermination(determinationObj)) { | ||
| if (determinationObj instanceof LocationTag && LocationTag.matches(determination)) { |
There was a problem hiding this comment.
Nope. ||. If it's already a LocationTag or it can become a LocationTag.
There was a problem hiding this comment.
Ohhhh! I get it now!
|
|
||
| @Override | ||
| public boolean applyDetermination(ScriptPath path, ObjectTag determinationObj) { | ||
| String determination = determinationObj.toString(); |
There was a problem hiding this comment.
Goes inside the if, not outside it (only used inside)
| String determination = determinationObj.toString(); | ||
| if (!isDefaultDetermination(determinationObj)) { | ||
| if (determinationObj instanceof LocationTag && LocationTag.matches(determination)) { | ||
| event.getAcceleration().copy(((LocationTag) determinationObj).toVector()); |
There was a problem hiding this comment.
need a valueOf, at least if instanceOf failed and matches passed instead.
Alternately you could remove the instanceOf and only do matches + valueOf (would be cleaner tbh)
|
AM fixes applied.... |
| @@ -15,6 +15,7 @@ public static void init() { | |||
| ScriptEvent.registerScriptEvent(new PlayerEquipsArmorScriptEvent()); | |||
| ScriptEvent.registerScriptEvent(new PlayerJumpsPaperScriptEventImpl()); | |||
| ScriptEvent.registerScriptEvent(new PlayerSpectatesEntityScriptEvent()); | |||
| ScriptEvent.registerScriptEvent(new EntityKnocksbackEntityScriptEvent()); | |||
There was a problem hiding this comment.
This list should be alphabetized (just look at the order of events in your IntelliJ class listing)
|
Fixed |
| return new BukkitScriptEntryData( | ||
| hitBy.isPlayer() ? hitBy.getDenizenPlayer() : | ||
| (entity.isPlayer() ? entity.getDenizenPlayer() : null), | ||
|
|
There was a problem hiding this comment.
Stray doublenewline ^
Also the newlines on the individual parts
I'm confused why this isn't formatted exactly the same as the EntityDamaged event, which I think is what you were referencing anyway?
return new BukkitScriptEntryData(damager != null && damager.isPlayer() ? damager.getDenizenPlayer() : entity.isPlayer() ? entity.getDenizenPlayer() : null,
damager != null && damager.isCitizensNPC() ? damager.getDenizenNPC() : entity.isCitizensNPC() ? entity.getDenizenNPC() : null);
|
:D |
Adds Paper only knockback event