Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LivingBreatheEvent and LivingDrownEvent #7810

Closed

Conversation

Meldexun
Copy link
Contributor

@Meldexun Meldexun commented Jun 7, 2021

This PR adds three new events to allow modders to control if a living entity can breathe and how air is consumed/refilled.
With the LivingBreatheEvent modders could easily prevent players from breathing in something like a moon dimension or allow players to breathe everywhere while wearing a specific item.
Withe the LivingConsumeAirEvent and LivingRefillAirEvent modders could change how air is consumed/refilled or add an item which can hold air which will be refilled when the players air is full.

Currently a problem could be something like this:
When a modder adds a moon dimension where the player can't breathe then he has to revert the refilling of the air supply and the make the player consume air. This would be a lot cleaner and compatibility between mods would be easier to achieve with the LivingBreatheEvent.

I also added two test mods which each add an item with make usage of the events.

Copy link
Contributor

@ChampionAsh5357 ChampionAsh5357 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question for you: why do you need three events to handle this logic? I'm pretty sure this could be done in a single cancelable event. For example, if the event is canceled, execute Forge logic where it checks if it can or can not breathe and then sets the amount. Calling three events every tick for something that could be done in one seems resource consuming.

Comment on lines 55 to 78
+ boolean flag2 = this.func_208600_a(FluidTags.field_206959_a) && !this.field_70170_p.func_180495_p(new BlockPos(this.func_226277_ct_(), this.func_226280_cw_(), this.func_226281_cx_())).func_203425_a(Blocks.field_203203_C);
+
+ if (flag2 && !this.field_70170_p.field_72995_K && this.func_184218_aH() && this.func_184187_bx() != null && !this.func_184187_bx().canBeRiddenInWater(this)) {
+ this.func_184210_p();
+ }
+
+ boolean canBreathe = !flag2 || this.func_70648_aU() || EffectUtils.func_205133_c(this) || flag1;
+ net.minecraftforge.event.entity.living.LivingBreatheEvent event = new net.minecraftforge.event.entity.living.LivingBreatheEvent(this, canBreathe);
+ net.minecraftforge.common.MinecraftForge.EVENT_BUS.post(event);
+
+ if (!event.isCanBreathe()) {
+ net.minecraftforge.event.entity.living.LivingConsumeAirEvent event1 = new net.minecraftforge.event.entity.living.LivingConsumeAirEvent(this, -this.func_70682_h(0));
+ net.minecraftforge.common.MinecraftForge.EVENT_BUS.post(event1);
+ this.func_70050_g(MathHelper.func_76125_a(this.func_70086_ai() - event1.getAmount(), -20, this.func_205010_bg()));
+
+ if (this.func_70086_ai() == -20) {
+ this.func_70050_g(0);
+ Vector3d vector3d = this.func_213322_ci();
+
+ for (int i = 0; i < 8; ++i) {
+ double d2 = this.field_70146_Z.nextDouble() - this.field_70146_Z.nextDouble();
+ double d3 = this.field_70146_Z.nextDouble() - this.field_70146_Z.nextDouble();
+ double d4 = this.field_70146_Z.nextDouble() - this.field_70146_Z.nextDouble();
+ this.field_70170_p.func_195594_a(ParticleTypes.field_197612_e, this.func_226277_ct_() + d2, this.func_226278_cu_() + d3, this.func_226281_cx_() + d4, vector3d.field_72450_a, vector3d.field_72448_b, vector3d.field_72449_c);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really invasive. You do not need this much of a rewrite to handle this logic. You can simply add the event below #isAlive, post it, and then store the result in the flag to pass into multiple places. You also reverse the logic of when the entity stops riding which in vanilla's case doesn't break behavior but could in other mods. You can also use ForgeHooks to handle the actual event creation bring this down to just three lines added with a few more modified.

Comment on lines 89 to 91
+ net.minecraftforge.event.entity.living.LivingRefillAirEvent event1 = new net.minecraftforge.event.entity.living.LivingRefillAirEvent(this, this.func_207300_l(0));
+ net.minecraftforge.common.MinecraftForge.EVENT_BUS.post(event1);
+ this.func_70050_g(MathHelper.func_76125_a(this.func_70086_ai() + event1.getAmount(), -20, this.func_205010_bg()));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same applies here that this could all go in ForgeHooks to reduce the amount of calls.

} else if (this.func_70086_ai() < this.func_205010_bg()) {
- } else if (this.func_70086_ai() < this.func_205010_bg()) {
- this.func_70050_g(this.func_207300_l(this.func_70086_ai()));
+ } else {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fires event unnecessarily. The point of the event is to refill. If you are not or your air meter is full, you should have no reason to fire the event.

@Meldexun
Copy link
Contributor Author

Meldexun commented Jun 7, 2021

So I formatted the forge classes and added the missing license headers.


This is really invasive. You do not need this much of a rewrite to handle this logic. You can simply add the event below #isAlive, post it, and then store the result in the flag to pass into multiple places.

This is basically the vanilla code:

if (isUnderWater && !notInBubbleColumn) {
    if (!canBreatheUnderwater && !hasWaterBreathing && !damageIsDisabled) {
        // consume air
    }
    // check if vehicle can be ridden underwater
} else if (air < maxAir) {
    // refill air
}

So to insert the events the only way I could think of was to:

  • move the vehicle check out of that statement
  • merge the first two if statements

You are free to help me making this less invasive though I didn't see a way to keep the original structure in tact. Which is the reason why it is how it is now.


Question for you: why do you need three events to handle this logic? I'm pretty sure this could be done in a single cancelable event. For example, if the event is canceled, execute Forge logic where it checks if it can or can not breathe and then sets the amount. Calling three events every tick for something that could be done in one seems resource consuming.

(two events every tick not three)
For example maybe I want to add an item which can store air.
I could use the consume event to first extract air out of my item and only when the item is empty the players air supply is used.
And the refill event could be used to fill the item once the players air supply is full (which is also the reason why the air < maxAir check was removed).

Fires event unnecessarily. The point of the event is to refill. If you are not or your air meter is full, you should have no reason to fire the event.

As described above

@ChampionAsh5357
Copy link
Contributor

You are free to help me making this less invasive though I didn't see a way to keep the original structure in tact. Which is the reason why it is how it is now.

You can just create a method within ForgeHooks to handle the logic for you and then just return what you need. That reduces those three line patches to a single line or even less in some cases.

And the refill event could be used to fill the item once the players air supply is full (which is also the reason why the air < maxAir check was removed).

But...that would never do anything. #setAirSupply is always hardcoded to choose the minimum between the added value and max air. Sure, you could add logic within the event to consume the item if used, but it's quite pointless to remove the check if literally nothing will happen when its called.

This is why I said the three event calls could be reduced down to one. That one event would be cancelable and set if the user can breathe and the amount to add/remove. When canceled, the event will execute the forge data retrieved from the event.

@Meldexun
Copy link
Contributor Author

Meldexun commented Jun 8, 2021

Hm I guess this could work.
Should I rather copy the if (air == -20) then DROWN into the ForgeHooks method or do more changes to the vanilla class?

@ChampionAsh5357
Copy link
Contributor

You should always do the least amount of lines added/modified as possible within the vanilla class.

@Meldexun
Copy link
Contributor Author

So I update this again. Now it is only one event which is fired inside a ForgeHooks method and thus the changes required to the vanilla class are very small now.

@sciwhiz12 sciwhiz12 added 1.16 Feature This request implements a new feature. Needs Rebase This PR requires a rebase to implement upstream changes (usually to fix merge conflicts). New Event This request implements a new event. Triage This request requires the active attention of the Triage Team. Requires labelling or reviews. labels Sep 21, 2021
@sciwhiz12
Copy link
Contributor

Hello! @Meldexun, please leave a comment whether you wish to continue this PR and update it to the latest active version of 1.18 (or leave an explanation why this PR's changes are not applicable to 1.18). If we do not hear back from you after some time, this PR will be closed for inactivity.
If anyone else wishes to continue the work of this PR, please create a PR with a link to this PR for reference.
Thank you for your contributions. 👋

@sciwhiz12 sciwhiz12 added the Stale This request hasn't had an update from the author in a long time. label Dec 25, 2021
@Meldexun
Copy link
Contributor Author

Meldexun commented Jan 3, 2022

Hi, yes I would like to continue this PR. I have created a new branch in my fork for MC 1.18 https://github.com/Meldexun/MinecraftForge/tree/living-breathe-event-1.18. So how should I proceed now? Should I create a new pull request?

@stale stale bot removed the Stale This request hasn't had an update from the author in a long time. label Jan 3, 2022
@sciwhiz12
Copy link
Contributor

@Meldexun, you have two options here:

  • Force-push the commits from that new branch to the branch of this PR, then change the base branch of this PR to the 1.18.x branch. I personally prefer this, as this preserves the history of the PR.
  • Create a new PR with that new branch, and close this PR as superseded by the new pull request.

@Meldexun Meldexun changed the base branch from 1.16.x to 1.18.x January 10, 2022 03:18
@Meldexun Meldexun changed the title LivingBreatheEvent, LivingConsumeAirEvent and LivingRefillAirEvent LivingBreatheEvent and LivingDrownEvent Jan 10, 2022
@Meldexun
Copy link
Contributor Author

I updated everything to MC 1.18 now. Also I moved pretty much all changes into the ForgeHooks#onLivingBreathe(LivingEntity, int, int) method and added a living drown event which allows modders to control how fast an entity takes damage while drowning or cancel the event when the entity is suffocating in a space dimension to not spawn the bubble particles and to not play the underwater drown sound.

@TheCurle
Copy link
Contributor

This may be easier to implement in 1.19 with the new fluid API.

I'm going to close this old one for inactivity (which is entirely our fault!) and we can continue if you'd like in a new PR.

@TheCurle TheCurle closed this Jul 17, 2022
@autoforge autoforge bot removed the Triage This request requires the active attention of the Triage Team. Requires labelling or reviews. label Feb 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.16 Feature This request implements a new feature. Needs Rebase This PR requires a rebase to implement upstream changes (usually to fix merge conflicts). New Event This request implements a new event.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants