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

Add PlayerLandEvent #10859

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

StillLutto
Copy link

Description

This is a very simple event. This event is triggered whenever a player lands on a block. The event is mostly useful for when you want to do a specific action whenever a player lands on a specific block/location.

Event Details

Event Name: PlayerLandEvent
Trigger: When a player lands on the ground
Purpose: To allow players to do specific actions when a player lands on a specific block

Key Components

Player: The player that landed.
From: The location the player landed from.
To: The location the player landed to.
Main Supporting Block: The main block supporting the player, the block that the player landed on.

@StillLutto StillLutto requested a review from a team as a code owner June 8, 2024 19:31

+ // Paper start - Add PlayerLandEvent
+ if (flag2 && !(this.player.getY() - d4 >= 0.0)) {
+ this.player.setOnGround(true);
Copy link
Member

Choose a reason for hiding this comment

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

What is up with setting the player's ground value here? As this is technically changing behavior?

Copy link
Author

Choose a reason for hiding this comment

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

The reason I set the ground value here is because that method runs checkSupportingBlock() which updates Entity#mainSupportingBlockPos. The reason I'm not just taking advantage of this.player.setOnGroundWithKnownMovement() which happens just a few lines below is because I previously made this event cancellable, But after further testing, it had pretty buggy behaviour... I'm going to quickly just move this below the this.player.setOnGroundWithKnownMovement() in the next commit.

import net.minecraft.world.phys.shapes.BooleanOp;
import net.minecraft.world.phys.shapes.Shapes;
import net.minecraft.world.phys.shapes.VoxelShape;
+import org.bukkit.craftbukkit.block.CraftBlock;
Copy link
Contributor

Choose a reason for hiding this comment

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

use fully qualified imports when using a class not very often.

Copy link
Author

Choose a reason for hiding this comment

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

Fair, will change.

@NonSwag
Copy link
Sponsor Contributor

NonSwag commented Jun 9, 2024

Change my mind, but isn't this just the player toggle flight event with extra conditions
Wouldn't exposing the main supporting block method from NMS be smarter?
image

@StillLutto
Copy link
Author

@NonSwag Whilst first looking how to do this in my own plugin, I saw that the best to check when a player lands, is to use a PlayerMoveEvent. The onGround boolean is, from what I understand, pretty unreliable. Thus, you will have to use a bit of NMS there to make it work. Furthermore, you will probably also need the mainSupportingBlock, which is added nms, and if you want the from and to location... Well, you get the point. This event is basically here to avoid this overhead. Now I'm going to be honest, I don't know if your method works, but if it does, all this nms will still be needed. It's definitely not that big of a problem, but the main idea is to avoid this. So make your own informed decision off of this.

@Brokkonaut
Copy link
Contributor

Brokkonaut commented Jun 11, 2024

isOnGround is exactly as reliable as your new event, because that is everything the server knows about the players on ground state. And adding api to get the mainSupportingBlock for the player should be easy and unproblematic.

@notTamion
Copy link
Contributor

there is already an pr open which exposes that iirc

@lynxplay
Copy link
Contributor

mainSupportingBlock iirc was dropped because of how funky movement is as, at the time PlayerMoveEvent e.g. is called, the supporting block is temporarily AIR until the server recomputes it right after.

@notTamion
Copy link
Contributor

yeah. if someone finds a solution for it in #9298 then that won‘t be a problem anymore

@StillLutto
Copy link
Author

StillLutto commented Jun 11, 2024

If you think this doesn't need to be added, you can close this post. I think onGround is not set right when 'PlayerMoveEvent` gets called, thus some nms there is still needed. Anyway, feel free to close this now.

@NonSwag
Copy link
Sponsor Contributor

NonSwag commented Jun 12, 2024

isOnGround is sent with the movement packet so in theory it could be added to the movement event
It is unreliable but still a part of the protocol so having it added would be a benefit
For example anti cheats could make use of it instead of using nms
Also Player#isOnGround is not required to be called which is not even up to date within the event
Maybe an annotation to warn about it's unreliability and not deprecating it would be appropriate
Same with the isOnGround method

@StillLutto
Copy link
Author

Again, feel free to close this if you think it's not worth adding

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Awaiting review
Development

Successfully merging this pull request may close these issues.

6 participants