[1.19.x] Add Cancelable LivingEatEvent & LivingFoodEffectEvent #9134

Adneths · 2022-11-05T10:03:46Z

Parallel's this PR with the same features just in 1.19.x

ChampionAsh5357 · 2022-11-05T12:50:13Z

patches/minecraft/net/minecraft/world/food/FoodData.java.patch

+-   public void m_38707_(int p_38708_, float p_38709_) {
+   public void eatInternal(int p_38708_, float p_38709_) {


We don't remove and change the name of methods. Add a deprecation message to the new method.

Shouldn't the deprecation be on the old method to encourage use of the new method that has a LivingEntity input. Also the method is not removed as m_38707_(int, float) still exists just calling eat(int, float, LivingEntity) which then calls eatInternal(int, float). So it effectively does the same thing just that it also fires the LivingEatEvent.
So I'm not sure why this is an issue.

You are replacing all calls to the basic #eat method, so this can be deprecated in favor of the LivingEntity counterpart. This can just be private in that case.

ChampionAsh5357 · 2022-11-05T12:50:50Z

patches/minecraft/net/minecraft/world/food/FoodData.java.patch

+-
+   @Deprecated
+   public void m_38707_(int p_38708_, float p_38709_) {
+      this.eat(p_38708_, p_38709_, null);
+   }


Unnecessary

Why is this unnecessary, it is still an eat event and should cause the LivingEatEvent to fire.

If that's the case, this should be moved on top of your #eatInternal method to reduce the patch size.

ChampionAsh5357 · 2022-11-05T12:51:47Z

patches/minecraft/net/minecraft/world/food/FoodData.java.patch

+   public void eat(int p_38708_, float p_38709_, @javax.annotation.Nullable net.minecraft.world.entity.LivingEntity entity) {
+      float[] ret = net.minecraftforge.common.ForgeHooks.onLivingEat(entity, p_38708_, p_38709_);
+      if(ret != null)
+         this.eatInternal((int)ret[0], ret[1]);
+   }


This could be an extension method to reduce patch size.

I don't understand what you mean with extension

Scratch this, the private change I mentioned above would cause this to fail.

patches/minecraft/net/minecraft/world/food/FoodData.java.patch

src/test/java/net/minecraftforge/debug/entity/living/LivingFoodEventTest.java

ChampionAsh5357 · 2022-11-05T13:06:36Z

patches/minecraft/net/minecraft/world/entity/LivingEntity.java.patch

+-            if (!p_21065_.f_46443_ && pair.getFirst() != null && p_21065_.f_46441_.m_188501_() < pair.getSecond()) {
+-               p_21066_.m_7292_(new MobEffectInstance(pair.getFirst()));
+         List<Pair<MobEffectInstance, Float>> ret = net.minecraftforge.common.ForgeHooks.onLivingFoodEffect(p_21066_, p_21064_);
+         if(ret != null)
+         {
+            for(Pair<MobEffectInstance, Float> pair : ret) {
+               if (!p_21065_.f_46443_ && pair.getFirst() != null && p_21065_.f_46441_.m_188501_() < pair.getSecond()) {
+                  p_21066_.m_7292_(new MobEffectInstance(pair.getFirst()));
+               }


Make patch smaller. Can unindent everything and have the if statement without braces.

But the rest of the patch uses indent? Do I column align the code?

It doesn't indent more than previous lines have. If column align is matching the previous code indent with the new one, then yes.

patches/minecraft/net/minecraft/world/entity/LivingEntity.java.patch

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

ChampionAsh5357

LivingEatEvent and LivingFoodEffectEvent is essentially a redundancy issue. The only cases that is not covered in the latter event are cakes and effects. So, there is a massive overlap between the two events otherwise, which should be addressed in some capacity as otherwise we are just firing the same event twice. Though I do not know the best way to go about fixing this.

ChampionAsh5357 · 2022-11-08T12:45:27Z

patches/minecraft/net/minecraft/world/entity/LivingEntity.java.patch

+-            if (!p_21065_.f_46443_ && pair.getFirst() != null && p_21065_.f_46441_.m_188501_() < pair.getSecond()) {
+-               p_21066_.m_7292_(new MobEffectInstance(pair.getFirst()));
+         List<Pair<MobEffectInstance, Float>> ret = net.minecraftforge.common.ForgeHooks.onLivingFoodEffect(p_21066_, p_21064_);
+         if(ret != null)
+         {
+            for(Pair<MobEffectInstance, Float> pair : ret) {
+               if (!p_21065_.f_46443_ && pair.getFirst() != null && p_21065_.f_46441_.m_188501_() < pair.getSecond()) {
+                  p_21066_.m_7292_(new MobEffectInstance(pair.getFirst()));
+               }


It doesn't indent more than previous lines have. If column align is matching the previous code indent with the new one, then yes.

ChampionAsh5357 · 2022-11-08T12:46:06Z

patches/minecraft/net/minecraft/world/food/FoodData.java.patch

+-   public void m_38707_(int p_38708_, float p_38709_) {
+   public void eatInternal(int p_38708_, float p_38709_) {


You are replacing all calls to the basic #eat method, so this can be deprecated in favor of the LivingEntity counterpart. This can just be private in that case.

ChampionAsh5357 · 2022-11-08T12:48:27Z

patches/minecraft/net/minecraft/world/food/FoodData.java.patch

+-
+   @Deprecated
+   public void m_38707_(int p_38708_, float p_38709_) {
+      this.eat(p_38708_, p_38709_, null);
+   }


If that's the case, this should be moved on top of your #eatInternal method to reduce the patch size.

ChampionAsh5357 · 2022-11-08T12:49:30Z

patches/minecraft/net/minecraft/world/food/FoodData.java.patch

+   public void eat(int p_38708_, float p_38709_, @javax.annotation.Nullable net.minecraft.world.entity.LivingEntity entity) {
+      float[] ret = net.minecraftforge.common.ForgeHooks.onLivingEat(entity, p_38708_, p_38709_);
+      if(ret != null)
+         this.eatInternal((int)ret[0], ret[1]);
+   }


Scratch this, the private change I mentioned above would cause this to fail.

ChampionAsh5357 · 2022-11-08T12:51:04Z

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

+ * LivingEatEvent is fire when an LivingEntity eats food (item, cake, etc.).<br>
+ * Specifically it is fired for the method call {@link FoodData#eat()}.<br>
+ * <br>
+ * This event is fired via the {@link ForgeHooks#onLivingEat()} .<br>


Why would it matter? People have the ability to backtrace through the code, so they could just easily look it up where the event is initialized from.

ChampionAsh5357 · 2022-11-08T12:54:22Z

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

+ * LivingFoodEffectEvent is fire when an LivingEntity eats food item.<br>
+ * Specifically it is fired for the method call {@link LivingEntity#addEatEffect()}.<br>
+ * <br>
+ * This event is fired via the {@link ForgeHooks#onLivingFoodEffect()} .<br>


Why would it matter? People have the ability to backtrace through the code, so they could just easily look it up where the event is initialized from.

ChampionAsh5357 · 2022-11-08T12:57:40Z

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

+		this.saturaionAmount = saturationAmount;
+	}
+
+	public LivingEatEvent(@javax.annotation.Nullable LivingEntity entity, ItemStack foodItem, FoodProperties prop) {


This only needs to be one constructor which takes in the nullable entity, food and saturation, and the nullable item stack. FoodProperties is outside the scope as the event does not take it in.

ChampionAsh5357 · 2022-11-08T12:58:35Z

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

+	public ItemStack getFoodItem() {
+		return foodItem;
+	}
+
+	public List<Pair<MobEffectInstance, Float>> getEffects() {
+		return effectPairs;
+	}


Document things on a public API level. Class documents are never good enough.

ChampionAsh5357 · 2022-11-08T13:03:40Z

patches/minecraft/net/minecraft/world/food/FoodData.java.patch

 -         FoodProperties foodproperties = p_38713_.m_41473_();
-+         FoodProperties foodproperties = p_38714_.getFoodProperties(entity);
-          this.m_38707_(foodproperties.m_38744_(), foodproperties.m_38745_());
+-         this.m_38707_(foodproperties.m_38744_(), foodproperties.m_38745_());
+         float[] ret = net.minecraftforge.common.ForgeHooks.onLivingEat(entity, p_38714_, p_38714_.getFoodProperties(entity));
+         if(ret != null)
+            this.eatInternal((int)ret[0], ret[1]);


It would be two removal two add now, but essentially it boils down to putting the condition and the execution statement on the same line (if and #eatInternal call)

ChampionAsh5357 · 2022-11-08T13:08:09Z

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

+import net.minecraftforge.eventbus.api.Cancelable;
+
+/**
+ * LivingFoodEffectEvent is fire when an LivingEntity eats food item.<br>


LivingApplyEffectOnEatEvent or probably LivingApplyFoodEffectEvent? It's just a thought that describes the action being taken.

As for your sentence, 'the event is fired whenever a LivingEntity receives an effect from food regardless of if the food applies any effects'. Though this brings a redundancy issue.

Adneths · 2022-11-19T14:47:02Z

LivingEatEvent and LivingFoodEffectEvent is essentially a redundancy issue. The only cases that is not covered in the latter event are cakes and effects. So, there is a massive overlap between the two events otherwise, which should be addressed in some capacity as otherwise we are just firing the same event twice. Though I do not know the best way to go about fixing this.

The way the code is written makes it hard to combine into one event. Because FoodData is changed at the start of Player#eat() while MobEffectInstance is applied in LivingEntity#eat() which is called at the end of Player#eat()

One solution I can think of is when FoodData#eat() with ItemStack is called it returns List<Pair<MobEffectInstance, Float>> after being modified. Then replace super.eat() with code copied from LivingEntity#eat() and change the way it processes food item effects. This choice does not change the method signatures but does mean Player#eat() does not really extend upon LivingEntity#eat() any more.

Another solution I came up with is to overload to get LivingEntity#eat(Level, ItemStack, List<Pair<MobEffectInstance, Float>>) so the list of effects can be specified outside of the method. This would allow the inheritance connection to remain.

I've implemented the second option into my code as of now.

Adneths · 2022-11-30T02:22:04Z

@James103
So what do I do now, I'm unfamiliar with the process as a whole.

CLAassistant · 2023-07-17T23:58:27Z

All committers have signed the CLA.

Implemented LivingEatEvent & LivingFoodEffectEvent

7cdc173

Matyrobbrt requested a review from a team November 5, 2022 11:07

Matyrobbrt added Feature This request implements a new feature. Triage This request requires the active attention of the Triage Team. Requires labelling or reviews. 1.19 New Event This request implements a new event. labels Nov 5, 2022

ChampionAsh5357 suggested changes Nov 5, 2022

View reviewed changes

Matyrobbrt suggested changes Nov 5, 2022

View reviewed changes

src/main/java/net/minecraftforge/event/entity/living/LivingFoodEffectEvent.java Outdated Show resolved Hide resolved

ChampionAsh5357 suggested changes Nov 8, 2022

View reviewed changes

Adneths added 5 commits November 17, 2022 08:16

Reduce patch size & fix some style

9df5210

Change Event documentation

01cec5b

Update LivingEatEvent.java

b5b954b

Combine LivingFoodEvent & LivingFoodEffectEvent

49a4614

Fix NullPointer crash

85be89a

Adneths requested review from ChampionAsh5357 and Matyrobbrt and removed request for ChampionAsh5357 November 20, 2022 07:47

James103 mentioned this pull request Nov 28, 2022

[1.18.x] Added a Hunger-System API #8405

Closed

sciwhiz12 added the Needs Rebase This PR requires a rebase to implement upstream changes (usually to fix merge conflicts). label Dec 16, 2022

Jonathing mentioned this pull request Jul 16, 2023

[1.18.x] Add Cancelable LivingEatEvent & LivingFoodEffectEvent #9125

Closed

sciwhiz12 mentioned this pull request Oct 21, 2023

Event/s for handling Hunger, Saturation, and Exhaustion neoforged/NeoForge#182

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[1.19.x] Add Cancelable LivingEatEvent & LivingFoodEffectEvent #9134

[1.19.x] Add Cancelable LivingEatEvent & LivingFoodEffectEvent #9134

Adneths commented Nov 5, 2022 •

edited

ChampionAsh5357 Nov 5, 2022

Adneths Nov 8, 2022

ChampionAsh5357 Nov 8, 2022

ChampionAsh5357 Nov 5, 2022

Adneths Nov 8, 2022

ChampionAsh5357 Nov 8, 2022

ChampionAsh5357 Nov 5, 2022

Adneths Nov 8, 2022

ChampionAsh5357 Nov 8, 2022

ChampionAsh5357 Nov 5, 2022

Adneths Nov 8, 2022

ChampionAsh5357 Nov 8, 2022

ChampionAsh5357 left a comment

ChampionAsh5357 Nov 8, 2022

ChampionAsh5357 Nov 8, 2022

ChampionAsh5357 Nov 8, 2022

ChampionAsh5357 Nov 8, 2022

ChampionAsh5357 Nov 8, 2022

ChampionAsh5357 Nov 8, 2022

ChampionAsh5357 Nov 8, 2022

ChampionAsh5357 Nov 8, 2022

ChampionAsh5357 Nov 8, 2022

ChampionAsh5357 Nov 8, 2022

Adneths commented Nov 19, 2022

Adneths commented Nov 30, 2022

CLAassistant commented Jul 17, 2023 •

edited

		- public void m_38707_(int p_38708_, float p_38709_) {
		+ public void eatInternal(int p_38708_, float p_38709_) {

[1.19.x] Add Cancelable LivingEatEvent & LivingFoodEffectEvent #9134

Are you sure you want to change the base?

[1.19.x] Add Cancelable LivingEatEvent & LivingFoodEffectEvent #9134

Conversation

Adneths commented Nov 5, 2022 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChampionAsh5357 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

Choose a reason for hiding this comment

Adneths commented Nov 19, 2022

Adneths commented Nov 30, 2022

CLAassistant commented Jul 17, 2023 • edited

Adneths commented Nov 5, 2022 •

edited

CLAassistant commented Jul 17, 2023 •

edited