-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Implement anvil #6418
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
base: minor-next
Are you sure you want to change the base?
Implement anvil #6418
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a quick overview
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My gut reaction to this is: I don't like the code.
I don't think we should be using custom actions for this since the actions are only usable with specific types of transactions. By the way other stuff has been done, different AnvilTransaction classes should be implemented.
I'm also not liking the logic of some of the item comparisons being limited to checking type IDs. Plugins will likely want to implement more complex logic, like using tags or NBT checking.
There was a problem hiding this 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 also probably be preferable to have more events, e.g. PlayerItemRenameEvent
, PlayerItemRepairEvent
, PlayerItemCombineEvent
(not sure about this last one). (That being said, if there were transaction types for this instead of actions, it would be a good amount nicer).
Doesn't make much sense to make plugins do complex logic to figure out the action type when we can just tell them directly.
This PR has been marked as "Waiting on Author", but we haven't seen any activity in 7 days. If there is no further activity, it will be closed in 28 days. Note for maintainers: Adding an assignee to the PR will prevent it from being marked as stale. |
This is a bump from my workspace
}else{ | ||
$newBlock = $anvilBlock->setDamage($newDamage); | ||
} | ||
$world->setBlock($inventory->getHolder(), $newBlock); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to be able to cancel this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements anvil functionality by adding new API methods for repair material validation, XP cost calculation, and enhanced anvil behavior. Key changes include new tests for anvil crafting, extended item repair cost handling, and integration of anvil recipes into the crafting manager.
Reviewed Changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
tests/phpunit/crafting/* | Added tests covering material repair and item self-combination recipes for anvils |
src/network/mcpe/handler/ItemStackRequestExecutor.php | Integrated AnvilTransaction support and optional anvil crafting action handling |
src/item/Item.php | Introduced anvil repair cost management with appropriate tag serialization |
src/inventory/transaction/AnvilTransaction.php | Added a new transaction type to validate, execute, and trigger anvil events |
src/crafting/* | Added new recipe types (MaterialRepairRecipe, ItemSelfCombineRecipe, etc.) and updated the crafting manager for anvil recipes |
src/block/utils/AnvilHelper.php | Encapsulated anvil result calculation logic with XP cost filtering |
|
||
protected function callExecuteEvent() : bool{ | ||
if($this->baseItem === null){ | ||
throw new AssumptionFailedError("Expected that baseItem are not null before executing the event"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message contains a grammatical inconsistency; consider changing "are not null" to "is not null" for clarity.
throw new AssumptionFailedError("Expected that baseItem are not null before executing the event"); | |
throw new AssumptionFailedError("Expected that baseItem is not null before executing the event"); |
Copilot uses AI. Check for mistakes.
$xpCost += 2 ** $input->getAnvilRepairCost() - 1; | ||
$xpCost += 2 ** $material->getAnvilRepairCost() - 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding parentheses to clarify operator precedence, for example: $xpCost += (2 ** $input->getAnvilRepairCost()) - 1;
$xpCost += 2 ** $input->getAnvilRepairCost() - 1; | |
$xpCost += 2 ** $material->getAnvilRepairCost() - 1; | |
$xpCost += (2 ** $input->getAnvilRepairCost()) - 1; | |
$xpCost += (2 ** $material->getAnvilRepairCost()) - 1; |
Copilot uses AI. Check for mistakes.
Introduction
As the title suggests, this PR implements the anvils
Relevant issues
Changes
API changes
Durable::isValidRepairMaterial
enabling everyone to implement their own verification logicrepairMaterials
toArmorMaterial
andToolTier
Item
Behavioural changes
Tests
I tested this PR by doing the following (tick all that apply):
tests/phpunit
folder)https://youtu.be/d3vcwi_45DY