Implemented LivingKnockBackEvent event and hook #4503

MercurialPony · 2017-11-07T12:15:52Z

Changes

Added LivingKnockBackEvent event, added onLivingKnockBack method to ForgeHooks, added onLivingKnockBack check and values into EntityLivingBase knockBack method. Added KnockBackHookTest test mod.

Why?

This event fixes the problems with hard-coded vanilla knock back. Modders can now cancel, change or add their own knock back to entities with quite a few parameters at their disposal. I've seen a few threads asking for this type of event before.
This addition is absolutely necessary to me as currently I'm creating a mod that adds a few new attributes for items including a knock back attribute. Using events like LivingAttackEvent to cancel knock back is very inefficient, sometimes inconsistent and potentially unstable.

How does it work?

It's as simple as any other event in forge. The LivingKnockBackEvent contains a few handy parameters like the attacker, strength and ratios which can be changed resulting in a highly flexible event.
I added a test mod KnockBackHookTest for anyone doubting the hook's functionality.

Added LivingKnockBackEvent event, added onLivingKnockBack method to ForgeHooks, added onLivingKnockBack check into EntityLivingBase knockBack method. Added KnockBackHookTest test mod.

bs2609

The hook looks ok, but there are a few style points.

Make sure to use spaces for indentation for consistency with the rest of the codebase.

Also try to keep changes in patches strictly to what you need to implement your hook: don't touch imports and only commit the patch files you actually worked on.

bs2609 · 2017-11-07T12:42:24Z

patches/minecraft/net/minecraft/entity/EntityLivingBase.java.patch

+ import net.minecraftforge.fml.relauncher.Side;
+ import net.minecraftforge.fml.relauncher.SideOnly;
+-import org.apache.logging.log4j.LogManager;
+-import org.apache.logging.log4j.Logger;


Don't change imports in patches.

bs2609 · 2017-11-07T12:43:01Z

patches/minecraft/net/minecraft/entity/EntityLivingBase.java.patch

+
+     public void func_70653_a(Entity p_70653_1_, float p_70653_2_, double p_70653_3_, double p_70653_5_)
+     {
+    	if (!net.minecraftforge.common.ForgeHooks.onLivingKnockBack(this, p_70653_1_, p_70653_2_, p_70653_3_, p_70653_5_)) return;


The formatting here doesn't match, check spaces are being used.

bs2609 · 2017-11-07T12:43:48Z

patches/minecraft/net/minecraft/item/Item.java.patch

@@ -68,7 +68,7 @@
         CreativeTabs creativetabs = this.func_77640_w();
         return creativetabs != null && (p_194125_1_ == CreativeTabs.field_78027_g || p_194125_1_ == creativetabs);
     }
-@@ -435,11 +445,704 @@
+@@ -435,11 +441,704 @@


You aren't really changing the contents of this file, so leave it out of the PR.

bs2609 · 2017-11-07T12:45:24Z

src/main/java/net/minecraftforge/common/ForgeHooks.java

@@ -559,6 +560,11 @@ public static boolean onLivingAttack(EntityLivingBase entity, DamageSource src,
        return !MinecraftForge.EVENT_BUS.post(new LivingAttackEvent(entity, src, amount));
    }

+    public static boolean onLivingKnockBack(EntityLivingBase target, Entity attacker, float strength, double ratioX, double ratioZ)
+    {
+    	return !MinecraftForge.EVENT_BUS.post(new LivingKnockBackEvent(target, attacker, strength, ratioX, ratioZ));


As you are negating the result when using this function, it'd be better to avoid the double-negation by just returning the result here directly.

bs2609 · 2017-11-07T12:46:20Z

src/main/java/net/minecraftforge/event/entity/living/LivingKnockBackEvent.java

+import net.minecraftforge.fml.common.eventhandler.Cancelable;
+
+/**
+ * LivingAttackEvent is fired when a living entity is about to be knocked back. <br>


Should be LivingKnockBackEvent?

bs2609 · 2017-11-07T12:48:17Z

src/main/java/net/minecraftforge/event/entity/living/LivingKnockBackEvent.java

+{
+	private final Entity attacker;
+	private float strength;
+	private double ratioX, ratioZ;


These values can be final as well.

bs2609 · 2017-11-07T12:49:17Z

src/test/java/net/minecraftforge/debug/KnockBackHookTest.java

+import net.minecraftforge.fml.common.Mod;
+import net.minecraftforge.fml.common.eventhandler.SubscribeEvent;
+
+@Mod(modid = "kbhtest", name = "Knock Back Hook Test")


Add a version = "1.0" element here, to prevent a warning being logged.

bs2609 · 2017-11-07T12:51:44Z

src/test/java/net/minecraftforge/debug/KnockBackHookTest.java

+	@SubscribeEvent
+	public static void onKnockBack(LivingKnockBackEvent event)
+	{
+		if(event.getEntityLiving() instanceof EntitySheep)


Can you make this mod be disabled by default?

bs2609 · 2017-11-07T12:55:05Z

src/main/java/net/minecraftforge/event/entity/living/LivingKnockBackEvent.java

@@ -0,0 +1,57 @@
+package net.minecraftforge.event.entity.living;


This file needs a license header.

LivingKnockBackEvent supports changing the values.

Generated patches.

Replaced tabs with spaces, restored unused imports, removed Item.java.patch from the PR, added a license header to and fixed LivingKnockBackEvent's description, removed unnecessary negation for the event post, fixed test mod parameters, disabled test mod by default.

bs2609 · 2017-11-07T14:54:27Z

I think a good way to handle allowing changes to the values is to patch in a block of code like this at the start of the knockBack method:

net.minecraftforge.event.entity.living.LivingKnockBackEvent event
    = net.minecraftforge.common.ForgeHooks.onLivingKnockBack(this, entityIn, strength, xRatio, zRatio);
if (event.isCanceled()) return;
strength = event.getStrength(); xRatio = event.getRatioX(); zRatio = event.getRatioZ();

Have the hook method construct and post the event as before, but then return the event itself, rather than a boolean. Then you can overwrite the passed in parameters with the new values from the event.

The reason I'd suggest doing it this way is to keep all the code changes in one block, and to avoid needing to touch the vanilla logic.

Event hook now initializes, posts and returns the event. knockBack method changed accordingly.

Again

MercurialPony · 2017-11-07T15:26:32Z

@bs2609 I fixed all the issues you pointed out. Will you approve now? When will these additions be merged with forge?

Draco18s

Some more cleanup required before it'll be approved. Lex is very strict about the standards: the smaller a patch file is, the better.

Draco18s · 2017-11-07T16:04:44Z

patches/minecraft/net/minecraft/entity/EntityLivingBase.java.patch

+ import javax.annotation.Nullable;
+
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;


You have unused imports

This is a patch, no touchy imports.

So do I have to remove the unused imports or not?

In Minecraft vanilla files do not ever touch imports, ever ever ever. You have added several, REMOVE THEM.

Draco18s · 2017-11-07T16:05:43Z

patches/minecraft/net/minecraft/entity/EntityLivingBase.java.patch

+
+     public void func_70653_a(Entity p_70653_1_, float p_70653_2_, double p_70653_3_, double p_70653_5_)
+     {
+        LivingKnockBackEvent event = net.minecraftforge.common.ForgeHooks.onLivingKnockBack(this, p_70653_1_, p_70653_2_, p_70653_3_, p_70653_5_);


You should use a fully qualified name, rather than an import here; net.minecraftforge.event.entity.living.LivingKnockBackEvent event = ...

Draco18s · 2017-11-07T16:07:09Z

src/main/java/net/minecraftforge/common/ForgeHooks.java

+import javax.annotation.Nullable;
+
+import org.apache.commons.io.FilenameUtils;
+import org.apache.commons.io.IOUtils;


You've reorganized the imports here, these lines have shifted place. Don't do this, it makes the patch files larger for no reason.

This is a forge class. Imports should be added but not organized, its entirely up to the users IDE as to how they are ordered.

So I don't have to organize them?

No, don't organize them. (And you're right @covers1624 it's a Forge class, not a patch, I missed that, but it still needs to be fixed).

Draco18s · 2017-11-07T16:09:55Z

patches/minecraft/net/minecraft/entity/EntityLivingBase.java.patch

+             {
+                 this.field_70181_x /= 2.0D;
+-                this.field_70181_x += (double)p_70653_2_;
+                this.field_70181_x += (double) p_70653_2_;


You have added a space here (double) p_70653_2_ which causes additional lines in the patch file that do precisely nothing.

Removed unused imports (again because I though they weren't mine), used full paths instead of imports in vanilla code, removed unnecessary spaces, restored imports to previous order.

MercurialPony · 2017-11-07T17:34:26Z

@Draco18s I fixed everything, please approve. The question still stands: when will the additions be merged and released?

bs2609 · 2017-11-07T17:36:20Z

You need to wait for mezz to review the PR. If they think it's ok, they'll assign it to Lex for final approval.

Draco18s · 2017-11-07T17:39:11Z

Looks much better now. Inclusion is primarily up to folks like Lex and mezz, who will review it at some point. Getting PRs added can be a lengthy process.

MercurialPony · 2017-11-07T17:40:12Z

Well thanks to everyone for the input. Here's hoping it will get added soon, I really need this for my mod.

jabelar · 2017-11-12T07:14:32Z

This is a useful event. It is a shame that more information about the context of why the knockback is occurring cannot be passed in. I understand that it is cleanest to hook into the knockback() method so you only have the parameters available that that method gets (plus the target entity in which the method is being called). It would be a lot cooler though if the DamageSource could be passed in as well, and also if you could determine if the knockback was due to blocking with shield.

In other words, instead of firing the event inside the knockback() method it would be much more useful if it was fired outside the method and a bit more information passed along.

But it is still pretty cool as is, and could always be extended in an unbreaking way later.

liach · 2017-11-12T17:33:28Z

src/test/java/net/minecraftforge/debug/KnockBackHookTest.java

+@Mod.EventBusSubscriber
+public class KnockBackHookTest
+{
+	private static final boolean ENABLED = false;


Can you put 4 spaces for tabs?

As liach says, please use spaces for indentation.

liach · 2017-11-12T17:36:10Z

src/main/java/net/minecraftforge/event/entity/living/LivingKnockBackEvent.java

+{
+    protected Entity attacker;
+    protected float strength;
+    protected double ratioX, ratioZ;


protected final Entity attacker; protected float strength; protected double ratioX, ratioZ; protected final float originalStrength; protected final double originalRatioX, originalRatioZ; public LivingKnockBackEvent(EntityLivingBase target, Entity attacker, float strength, double ratioX, double ratioZ) { super(target); this.attacker = attacker; this.originalStrength = strength; this.originalRatioX = ratioX; this.originalRatioZ = ratioZ; this.strength = strength; this.ratioX = ratioX; this.ratioZ = ratioZ; }

In case of multiple mods modifying the same event.

This is a good suggestion from liach, please add the "original" values as final values, so mods using the events can have more information about modifications to the values.

mezz

This is looking good. Please address the remaining comments and then I will test this out.
Consider jabelar's comment as well, and see if you can fire this in a location that gives it access to the damage source and if there is a shield. That will make the event useful in more situations.

mezz · 2017-11-18T20:40:43Z

src/test/java/net/minecraftforge/debug/KnockBackHookTest.java

+@Mod.EventBusSubscriber
+public class KnockBackHookTest
+{
+	private static final boolean ENABLED = false;


As liach says, please use spaces for indentation.

mezz · 2017-11-18T20:41:36Z

src/main/java/net/minecraftforge/event/entity/living/LivingKnockBackEvent.java

+{
+    protected Entity attacker;
+    protected float strength;
+    protected double ratioX, ratioZ;


This is a good suggestion from liach, please add the "original" values as final values, so mods using the events can have more information about modifications to the values.

MercurialPony · 2017-11-19T14:36:23Z

@mezz @jabelar I'll get around to fixing the issues. However, I'm not sure how to go about passing more information to the event. I could add the event to every knockBack() method call, but that would require modifying at least 4 different classes and wouldn't work if other modders called the method. I could add more params to the knockBack() method, but I'm not sure that's a good idea for compatibility or similar issues.

mezz · 2017-11-19T22:55:25Z

I guess for now just keep it simple, the way it is.

Added original fields and getters for the event's params, fixed indentation in the test mod.

MercurialPony · 2017-11-26T20:05:06Z

@mezz Alright, I've added everything you pointed out. Will this be added to the api now? Also, why do none of the other events with changeable params have original fields?

liach · 2017-11-26T22:38:06Z

src/main/java/net/minecraftforge/event/entity/living/LivingKnockBackEvent.java

+{
+    protected Entity attacker;
+    protected float strength;
+    protected double ratioX, ratioZ;


mezz · 2017-11-27T01:57:17Z

Tested and works as expected.

Implemented LivingKnockBackEvent

7a04fa5

Added LivingKnockBackEvent event, added onLivingKnockBack method to ForgeHooks, added onLivingKnockBack check into EntityLivingBase knockBack method. Added KnockBackHookTest test mod.

MercurialPony changed the title ~~Implemented LivingKnockBackEvent~~ Implemented LivingKnockBackEvent hook Nov 7, 2017

bs2609 reviewed Nov 7, 2017

View reviewed changes

MercurialPony added 2 commits November 7, 2017 15:59

Updated the LivingKnockBackEvent

e967d43

LivingKnockBackEvent supports changing the values.

Fixed bug with patches.

462e558

Generated patches.

MercurialPony changed the title ~~Implemented LivingKnockBackEvent hook~~ Implemented LivingKnockBackEvent event and hook Nov 7, 2017

MercurialPony added 2 commits November 7, 2017 18:09

Changed event hook

e77c5c0

Event hook now initializes, posts and returns the event. knockBack method changed accordingly.

Restored unused imports

7448711

Again

Fixed minor description issue

483cfd2

Draco18s suggested changes Nov 7, 2017

View reviewed changes

MercurialPony added 2 commits November 7, 2017 20:17

Fixed minor style issues.

dbc1392

Removed unused imports (again because I though they weren't mine), used full paths instead of imports in vanilla code, removed unnecessary spaces, restored imports to previous order.

Restored import order.

8ce55bd

liach reviewed Nov 12, 2017

View reviewed changes

mezz added 1.12 Feature This request implements a new feature. labels Nov 18, 2017

mezz suggested changes Nov 18, 2017

View reviewed changes

Updated event, fixed style issues.

b5c620c

Added original fields and getters for the event's params, fixed indentation in the test mod.

liach approved these changes Nov 26, 2017

View reviewed changes

mezz approved these changes Nov 27, 2017

View reviewed changes

mezz assigned LexManos Nov 27, 2017

mezz added the Assigned This request has been assigned to a core developer. Will not go stale. label Nov 27, 2017

LexManos merged commit bbf1bef into MinecraftForge:1.12.x Dec 17, 2017

		@@ -0,0 +1,57 @@
		package net.minecraftforge.event.entity.living;

Implemented LivingKnockBackEvent event and hook #4503

Implemented LivingKnockBackEvent event and hook #4503

Conversation

MercurialPony commented Nov 7, 2017 • edited

Changes

Why?

How does it work?

bs2609 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bs2609 commented Nov 7, 2017

MercurialPony commented Nov 7, 2017 • edited

Draco18s left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Draco18s Nov 7, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MercurialPony commented Nov 7, 2017

bs2609 commented Nov 7, 2017

Draco18s commented Nov 7, 2017

MercurialPony commented Nov 7, 2017

jabelar commented Nov 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mezz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MercurialPony commented Nov 19, 2017 • edited

mezz commented Nov 19, 2017

MercurialPony commented Nov 26, 2017

Choose a reason for hiding this comment

mezz commented Nov 27, 2017

MercurialPony commented Nov 7, 2017 •

edited

MercurialPony commented Nov 7, 2017 •

edited

Draco18s Nov 7, 2017 •

edited

MercurialPony commented Nov 19, 2017 •

edited