Skip to content

Conversation

@HimmelKreis4865
Copy link
Contributor

Introduction

This PR adds working smithing tables to pocketmine.
There are no known issues or problems

Changes

API changes

Amor->setTrim(ArmorTrim)
Armor->getTrim() : ?ArmorTrim
Armor->clearTrim()

SmithingTableInventory->getInput() : Item
SmithingTableInventory->getAddition() : Item
SmithingTableInventory->getTemplate() : Item

The new enums: ArmorTrimPattern & ArmorTrimMaterial (required to construct ArmorTrim)

Tests

Tested ingame with the smithing table several times:
image

Tested the code

$player->getInventory()->addItem(VanillaItems::LEATHER_CAP()->setTrim(new ArmorTrim(ArmorTrimMaterial::QUARTZ, ArmorTrimPattern::EYE)));

@ShockedPlot7560 ShockedPlot7560 added Category: API Related to the plugin API Category: Core Related to internal functionality Type: Enhancement Contributes features or other improvements to PocketMine-MP labels Dec 20, 2023
@HimmelKreis4865
Copy link
Contributor Author

There's not much more I can do at this point, everthing is fixed except for save ids.

Copy link
Contributor

@jasonw4331 jasonw4331 left a comment

Choose a reason for hiding this comment

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

Looks pretty good to me, one comment.

@fractalllll

This comment was marked as off-topic.

@pandaaaBE
Copy link
Contributor

pandaaaBE commented Mar 22, 2024

image
Looks finished now :)

@HimmelKreis4865
Copy link
Contributor Author

yeah, ready to be reviewed again.

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 mostly OK.

I'm a bit uneasy about allowing custom armor trim patterns because they'll generate confusing errors if the objects are duplicated. The ArmorTrimPattern and ArmorTrimMaterial classes should use the NotCloneable and NotSerializable traits.

@kostamax27
Copy link
Contributor

image

[12:57:00.529] [Server thread/DEBUG]: [NetworkSession: kostamax27] Invalid inventory transaction -35: Expected an exact multiple of required Item Netherite Upgrade Smithing Template (20269:0)x2 (given: 1, needed: 2)
[12:57:00.529] [Server thread/DEBUG]: [NetworkSession: kostamax27] Detected prediction mismatch in inventory pocketmine\inventory\PlayerCursorInventory#214179 slot 0
[12:57:00.529] [Server thread/DEBUG]: [NetworkSession: kostamax27] Detected prediction mismatch in inventory pocketmine\block\inventory\CraftingTableInventory#88462 slot 0
[12:57:00.529] [Server thread/DEBUG]: [NetworkSession: kostamax27] Detected prediction mismatch in inventory pocketmine\block\inventory\CraftingTableInventory#88462 slot 1
[12:57:00.529] [Server thread/DEBUG]: [NetworkSession: kostamax27] Detected prediction mismatch in inventory pocketmine\block\inventory\CraftingTableInventory#88462 slot 2
[12:57:00.529] [Server thread/DEBUG]: [NetworkSession: kostamax27] Detected prediction mismatch in inventory pocketmine\block\inventory\CraftingTableInventory#88462 slot 3
[12:57:00.529] [Server thread/DEBUG]: [NetworkSession: kostamax27] Detected prediction mismatch in inventory pocketmine\block\inventory\CraftingTableInventory#88462 slot 4
[12:57:00.529] [Server thread/DEBUG]: [NetworkSession: kostamax27] Detected prediction mismatch in inventory pocketmine\block\inventory\CraftingTableInventory#88462 slot 5
[12:57:00.529] [Server thread/DEBUG]: [NetworkSession: kostamax27] Detected prediction mismatch in inventory pocketmine\block\inventory\CraftingTableInventory#88462 slot 6
[12:57:00.529] [Server thread/DEBUG]: [NetworkSession: kostamax27] Detected prediction mismatch in inventory pocketmine\block\inventory\CraftingTableInventory#88462 slot 7
[12:57:00.529] [Server thread/DEBUG]: [NetworkSession: kostamax27] Detected prediction mismatch in inventory pocketmine\block\inventory\CraftingTableInventory#88462 slot 8
[12:57:00.560] [Server thread/DEBUG]: [NetworkSession: kostamax27] Full inventory sync requested, sending contents of 6 inventories

@dktapps
Copy link
Member

dktapps commented Oct 17, 2025

For posterity: I think this got stalled because I wasn't happy about using RegistryTrait for armor patterns and trims.
My issue with that is mostly around wanting to use enums wherever possible, but we run into #6292. I don't feel good about introducing inconsistent APIs around this stuff, but no resolution was reached in internal discussions about this.

Will revisit this soon ™️

@dktapps
Copy link
Member

dktapps commented Dec 21, 2025

So, the issue with the registries makes more sense to me now.
Basically, the problem is that the patterns and materials are registered by object ID as if they were enums, but it's extremely easy for plugin devs to just do setTrim(new ArmorTrim(new ArmorTrimMaterial(VanillaItems::SOMETHING()), ...)) and then proceed to have the server crash later when it tries to save data, because that particular trim material object's identity was never associated with a material ID.

When we use enums, it's easy to guarantee that all enum cases are saveable since there's never going to be others that weren't defined at compile time. This is not true when using registries, which makes the API here a series of footguns which will lead to confusing crashes.

This is linked to a broader issue described in #6654

}
if($input !== null && $addition !== null && $template !== null){
$material = ArmorTrimMaterialTypeIdMap::getInstance()->fromItem($addition);
$pattern = ArmorTrimPatternTypeIdMap::getInstance()->fromItem($template);
Copy link
Member

Choose a reason for hiding this comment

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

Really don't like this. pocketmine\data namespace is explicitly internal.
Type ID mapping is part of an internal package because plugins aren't supposed to deal with that stuff, but plugins should be able to get the trim color of an item without using internal namespaces.
Likewise, the pattern should be obtainable without using internal namespaces.

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: Core Related to internal functionality Type: Enhancement Contributes features or other improvements to PocketMine-MP

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants