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

feat(entity/api): add EntityEvent.IsPushable and implementation #9675

Closed

Conversation

danorris709
Copy link
Contributor

Description

This PR adds the EntityEvent.IsPushable class so that you can prevent entities from being pushed in any given situation, with the main intention being to prevent this interaction with vanilla entities.

Reason

The main use-case I have for making this is for preventing players from being pushed. In lower versions there was a variable in the Entity class mapped to "pushthrough" which was used in Mojang's code for determining how much an Entity would be pushed by. However, in modern versions Mojang removed this and replaced this with the "isPushable" method. Unfortuantely this means that you cannot prevent players from being pushed anymore, and in my specific instance it means that when a player is in a GUI and gets pushed into a nether portal the nether portal GUI effect begins to cause graphical errors. They could also be pushed into other damage sources whilst in this GUI which is something I'd like to prevent without putting them in spectator mode.

Details

I added the event to each of the overrides of the isPushable method such that it would fire consistently for all entities, with the exception of those entities that have a super call. I did this as I assumed it would be much more useful to others if it covered more than just the Player entity.

I did not include the event call in classes such as the Warden where they have a super call, as I figured that if they wanted to prevent the warden from being pushed they could set it to return false and the logical and would end up returning false and as such the event won't fire twice. However, now I am considering that perhaps it is returning false and they want it to return true? Is this something that should be changed?

Code from Warden.java:

   public boolean isPushable() {
      return !this.isDiggingOrEmerging() && super.isPushable();
   }

@CLAassistant
Copy link

CLAassistant commented Jul 21, 2023

CLA assistant check
All committers have signed the CLA.

@autoforge autoforge bot added Triage This request requires the active attention of the Triage Team. Requires labelling or reviews. 1.20 labels Jul 21, 2023
@autoforge autoforge bot requested a review from a team July 21, 2023 13:16
Copy link
Member

@Jonathing Jonathing left a comment

Choose a reason for hiding this comment

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

A test mod would go great with this. Please make one. 😊

@Jonathing Jonathing added Feature This request implements a new feature. New Event This request implements a new event. labels Jul 21, 2023
@danorris709
Copy link
Contributor Author

A test mod would go great with this. Please make one. 😊

A test mod as in a Forge test case? Or just an example mod where the event is used?

If the former, I wasn't really sure how to emulate two entities pushing eachother in the world using the test framework

@Jonathing
Copy link
Member

Just a test mod that listens for the new event and cancels it on a specific mob. Something just to demonstrate that the event works.

@danorris709
Copy link
Contributor Author

danorris709 commented Jul 21, 2023

Just a test mod that listens for the new event and cancels it on a specific mob. Something just to demonstrate that the event works.

I added the test and here is a GIF of with and without it enabled

Test mod enabled

Test mod disabled

(Removed the embeded gifs as they looked terrible)

Copy link
Member

Choose a reason for hiding this comment

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

Please include the license header at the top of this file.

/*
 * Copyright (c) Forge Development LLC and contributors
 * SPDX-License-Identifier: LGPL-2.1-only
 */

Copy link
Member

Choose a reason for hiding this comment

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

Resolved in 97a32f0.

Copy link
Member

@Jonathing Jonathing left a comment

Choose a reason for hiding this comment

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

Looks good! I appreciate the minimally-invasive changes.

About your comment on the Warden: I think it would be good to keep this implementation as-is. This event seems like it would be useful to exclusively cancel the motion of entity cramming and pushing, rather than to enable it. Although I can leave additional thoughts to @LexManos.

@autoforge autoforge bot added Assigned This request has been assigned to a core developer. Will not go stale. and removed Triage This request requires the active attention of the Triage Team. Requires labelling or reviews. labels Jul 21, 2023
@danorris709
Copy link
Contributor Author

Looks good! I appreciate the minimally-invasive changes.

About your comment on the Warden: I think it would be good to keep this implementation as-is. This event seems like it would be useful to exclusively cancel the motion of entity cramming and pushing, rather than to enable it. Although I can leave additional thoughts to @LexManos.

Hmm perhaps to make it less exclusive, and further it's useful-ness, I could change it to a "Push" event where it includes the push motion and then allows for modifying how far an entity is pushed, and as such the equivalent to preventing them from being pushed would just be setting that motion to all 0s.

Something like this perhaps:

(From Entity.java:)

   public void push(double p_20286_, double p_20287_, double p_20288_) {
      var result = ForgeHooks.onPush(this, p_20286_, p_20287_, p_20288_);
      this.setDeltaMovement(this.getDeltaMovement().add(result.x(), result.y(), result.z());
      this.hasImpulse = true;
   }

Thus allowing for more creativity as potentially it could be used for increasing the push reaction under given circumstances

@Jonathing
Copy link
Member

If you think that would be better, you can make a successor PR and close this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.20 Assigned This request has been assigned to a core developer. Will not go stale. Feature This request implements a new feature. New Event This request implements a new event.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants