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

Added entity middle-click feature #5397

Merged
merged 18 commits into from
Nov 15, 2024

Conversation

JavierLeon9966
Copy link
Contributor

@JavierLeon9966 JavierLeon9966 commented Nov 7, 2022

Introduction

Added entity middle-click feature, this is a vanilla feature that it hasn't been implemented for so long.

Relevant issues

Changes

API changes

Added the following methods in Entity:

  • Entity->getCleanedNBT(): Returns a cleaned NBT from the entity, removing tags such as position, motion, etc.
  • Entity->copyDataFromItem(): Copies the entity NBT from the item into the entity (internal).
  • Entity->getSpawnItem(): Returns the item that can be used to spawn the entity, by default it returns air if there isn't one.
  • Entity->getPickedItem(): Returns an item that is able to spawn said entity, with a parameter $addUserData to include the entity's NBT.

Added the following methods in Item:

  • Item->clearCustomEntityData(): Clears the NBT from the picked (middle-clicked) entity.
  • Item->setCustomEntityData(): Sets the NBT of the picked (middle-clicked) entity.
  • Item->getCustomEntityData(): Returns the NBT of the picked (middle-clicked) entity.

Added the following method in Player:

  • Player->pickEntity(): Equips the middle-clicked (picked) entity in the player's hand with added NBT if requested.

Added the following event:

  • PlayerEntityPickEvent

Tests

https://clipchamp.com/watch/nrP09ZXooey

@dktapps
Copy link
Member

dktapps commented Nov 7, 2022

I'd prefer this wasn't dependent on NBT. Bad enough that we have to deal with that for tiles already...

Copy link
Member

@dktapps dktapps left a comment

Choose a reason for hiding this comment

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

I don't like that this involves so much in the way of legacy hacks.

src/entity/Entity.php Outdated Show resolved Hide resolved
src/player/Player.php Show resolved Hide resolved
@dktapps dktapps added Category: API Related to the plugin API Category: Gameplay Related to Minecraft gameplay experience Type: Contribution labels Nov 7, 2022
@dktapps dktapps added Type: Enhancement Contributes features or other improvements to PocketMine-MP and removed Type: Contribution labels Nov 26, 2022
Copy link
Member

@dktapps dktapps left a comment

Choose a reason for hiding this comment

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

I'm hesitant to merge this because entities currently predominantly use the Java entity NBT format, but items use the Bedrock format. This is already a disaster for tiles.

I'm OK with having the basic entity picking, but I think the NBT stuff should be removed for now until the internals handling can be cleaned up (and preferably find a way to deal with extra data that doesn't require NBT at all).

src/entity/Entity.php Outdated Show resolved Hide resolved
src/entity/Entity.php Outdated Show resolved Hide resolved
src/item/Item.php Outdated Show resolved Hide resolved
Copy link
Member

@dktapps dktapps left a comment

Choose a reason for hiding this comment

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

The separation between "spawn item" and "picked item" seems redundant.

src/entity/Entity.php Outdated Show resolved Hide resolved
src/entity/Entity.php Outdated Show resolved Hide resolved
@jasonw4331
Copy link
Contributor

@JavierLeon9966 any updates for this PR? If not it will be closed when API 5 releases

@JavierLeon9966
Copy link
Contributor Author

Oh, whoops, I forgot about it, but I think I have the changes in my local

Joshy3282 added a commit to Joshy3282/PocketMine-MP that referenced this pull request Oct 2, 2024
@dktapps dktapps added the Status: Blocked Depends on other changes which are yet to be completed label Nov 14, 2024
src/entity/Entity.php Outdated Show resolved Hide resolved
@dktapps dktapps removed the Status: Blocked Depends on other changes which are yet to be completed label Nov 14, 2024
@dktapps
Copy link
Member

dktapps commented Nov 14, 2024

This looks basically fine as it is right now, minus a few nits.

I will mention that we'll probably want to apply #4771 to PlayerEntityPickEvent too, but that's a problem for another time. The most important thing is for the events to be consistent.

@JavierLeon9966 JavierLeon9966 requested a review from a team as a code owner November 15, 2024 16:53
dktapps
dktapps previously approved these changes Nov 15, 2024
dktapps
dktapps previously approved these changes Nov 15, 2024
@dktapps dktapps merged commit d3add78 into pmmp:minor-next Nov 15, 2024
15 checks passed
@JavierLeon9966 JavierLeon9966 deleted the entity-pick-request branch November 15, 2024 21:33
@dktapps dktapps mentioned this pull request Nov 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: API Related to the plugin API Category: Gameplay Related to Minecraft gameplay experience Type: Enhancement Contributes features or other improvements to PocketMine-MP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants