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

Items that have contents now drops them on destroy #4768

Open
wants to merge 21 commits into
base: minor-next
Choose a base branch
from

Conversation

JavierLeon9966
Copy link
Contributor

@JavierLeon9966 JavierLeon9966 commented Jan 24, 2022

Introduction

Currently when you destroy an item with contents (ex: shulker box) it doesn’t drop them.

Changes

API changes

Added the following methods in Item:

  • Item->getContainedItems(): Returns the contents of such item, ex: shulker boxes, chests, brewing stands, hoppers, etc.
  • Item->setContainedItems(): Sets the contents of the item.

Added the following method in ContainerTrait:

  • ContainerTrait->copyContentsFromItem(): Copied the citem contents into the block entity's inventory.

Added the following helper class:

  • pocketmine\block\tile\util\ContainerHelper

Added the following trait:

  • pocketmine\block\util\ContainerTrait

Behavioural changes

Any item that has contents in it will now drop them once destroyed. This can include bundles once implemented.

Tests

z90lh0.mp4

@ghost
Copy link

ghost commented Jan 25, 2022

Items should also scatter in different directions. Is it provided?

@dktapps
Copy link
Member

dktapps commented Jan 25, 2022

That's a topic for a different issue. Player deaths should also have the same behaviour but don't.

@ghost
Copy link

ghost commented Jan 25, 2022

That's a topic for a different issue. Player deaths should also have the same behaviour but don't.

Will it be in the next next-minor?

@dktapps dktapps added Category: Gameplay Related to Minecraft gameplay experience Type: Contribution labels Jan 25, 2022
@NTT1906

This comment has been minimized.

@NTT1906

This comment has been minimized.

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 think it would be better to have some kind of API like Item::getContainedItems() instead of making bogus assumptions about NBT.

@JavierLeon9966
Copy link
Contributor Author

I think it would be better to have some kind of API like Item::getContainedItems() instead of making bogus assumptions about NBT.

Yeah, especially when the bundle gets implemented. It could’ve a different NBT compared to chests/shulker boxes.
Now this would require item classes for both Chest and ShulkerBox items or some trait like ContainerItemTrait that has this NBT deserializer, and a serializer to let plugins add items with Item::setContainedItems()

@dktapps
Copy link
Member

dktapps commented Sep 28, 2022

I think it would be better to have some kind of API like Item::getContainedItems() instead of making bogus assumptions about NBT.

Yeah, especially when the bundle gets implemented. It could’ve a different NBT compared to chests/shulker boxes. Now this would require item classes for both Chest and ShulkerBox items or some trait like ContainerItemTrait that has this NBT deserializer, and a serializer to let plugins add items with Item::setContainedItems()

I think a general implementation in Item would be sufficient. I'm just wary of assuming internal item NBT structure from outside and not being able to override it.

Now `ItemEntity` uses `Item::getContainedItems()` to drop the contained items
@JavierLeon9966 JavierLeon9966 changed the base branch from stable to next-minor October 3, 2022 01:19
@dktapps dktapps changed the base branch from next-minor to stable October 11, 2022 20:28
@dktapps dktapps changed the base branch from stable to next-minor October 11, 2022 20:28
@dktapps dktapps added Type: Enhancement Contributes features or other improvements to PocketMine-MP and removed Type: Contribution labels Nov 26, 2022
@ShockedPlot7560
Copy link
Member

Need to be update with the upstream

src/item/Item.php Outdated Show resolved Hide resolved
src/block/tile/util/ContainerHelper.php Outdated Show resolved Hide resolved
src/block/tile/util/ContainerHelper.php Outdated Show resolved Hide resolved
@JavierLeon9966
Copy link
Contributor Author

Found 3 issues that are outside the scope of this PR:

  • When placing chiseled bookshelves with books in it, it doesn't render on the block
  • When placing a brewing stand with blaze powder in it, the fuel time renders full until rendering the block again
  • NamingTrait::copyDataFromItem may need to be renamed to NamingTrait::copyNameFromItem to reduce duplicate code

@dktapps dktapps requested a review from a team as a code owner November 15, 2024 10:30
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.

Looks OK apart from a couple of minor nits

src/item/Item.php Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

6 participants