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

fix: canPlaceOn and canDestroy will be passed on conversion #6461

Closed
wants to merge 1 commit into from

Conversation

Nerahikada
Copy link
Contributor

Introduction

TypeConverter now also uses canPlaceOn and canDestroy parameters for item conversions.

Relevant issues

Changes

API changes

Behavioural changes

Backwards compatibility

Follow-up

This is enough to fix #6460, but still has the following issues:

  • Crashes when using new IDs such as minecraft:grass_block
  • ItemStack extra data contains unnecessary NBT data:
    • Minecraft vanilla: //8BCgAAAwYARGFtYWdlAAAAAAAAAAAAAQAAABUAbWluZWNyYWZ0OmdyYXNzX2Jsb2Nr
    • pmmp: //8BCgAACQoAQ2FuRGVzdHJveQgBAAAAFQBtaW5lY3JhZnQ6Z3Jhc3NfYmxvY2sAAAAAAAEAAAAVAG1pbmVjcmFmdDpncmFzc19ibG9jaw==

Tests

I tested this PR by doing the following (tick all that apply):

  • Writing PHPUnit tests (commit these in the tests/phpunit folder)
  • Playtesting using a Minecraft client (provide screenshots or a video)
  • Writing a test plugin (provide the code and sample output)
  • Other (provide details)
$item = VanillaItems::IRON_HOE();
$item->setCanDestroy(["grass"]);  // minecraft:grass_block or grass_block will crash
$player->getInventory()->addItem($item);

@dries-c
Copy link
Member

dries-c commented Oct 26, 2024

Would be nice if we could validate whatever is set in setCanDestroy.

@jasonw4331 jasonw4331 added Category: API Related to the plugin API Category: Gameplay Related to Minecraft gameplay experience Type: Fix Bug fix, typo fix, or any other fix Category: Protocol Related to (changes in) the Minecraft network protocol Status: Waiting on Author labels Nov 4, 2024
@dktapps
Copy link
Member

dktapps commented Nov 10, 2024

In PM these arrays can accept arbitrary values (by design). Same can't be said for the Minecraft ones, those have to contain actual block IDs. Not sure what the proper solution would be. Perhaps some clunky hack using StringToItemParser and the blockstate serializer to retrieve the ID, but the behaviour still might differ between client & server because Minecraft block IDs don't map 1:1 onto PM block types.

Personally I think this feature is better not bothered with.

@dktapps
Copy link
Member

dktapps commented Nov 10, 2024

Also worth mentioning that PM does enforce these, but only in adventure mode.

@dktapps dktapps added Status: Blocked Depends on other changes which are yet to be completed Opinions Wanted Request for comments & opinions from the community and removed Status: Waiting on Author labels Nov 14, 2024
@dktapps
Copy link
Member

dktapps commented Nov 24, 2024

Closing this as WF since there doesn't seem to be an obvious way to control this by the item's data beside its ID, making it difficult to use with PM-side items without directly exposing Mojang internals to the API. Considering the limited benefit I don't think this is worth bothering with.

@dktapps dktapps closed this Nov 24, 2024
@dktapps dktapps added Resolution: Declined PR has been declined by maintainers and removed Status: Blocked Depends on other changes which are yet to be completed Category: API Related to the plugin API labels Nov 24, 2024
@Nerahikada Nerahikada deleted the Nerahikada-patch-2 branch November 26, 2024 00:37
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 Category: Protocol Related to (changes in) the Minecraft network protocol Opinions Wanted Request for comments & opinions from the community Resolution: Declined PR has been declined by maintainers Type: Fix Bug fix, typo fix, or any other fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

canPlaceOn, canDestroy have no effect on clients
4 participants