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

Add CraftingContext support #1181

Open
wants to merge 1 commit into
base: 1.21.x
Choose a base branch
from

Conversation

Random832
Copy link
Contributor

Previously we only supported the player in the get remaining items stage, this adds it to all stages of recipe evaluation and adds the Level and CraftingContainer as well. This will help in situations such as: the recipe needs to use durability from items in get remaining items; the recipe needs to distinguish between two identical inputs where one is shifted down or right in the grid; etc.

@CLAassistant
Copy link

CLAassistant commented Jun 25, 2024

CLA assistant check
All committers have signed the CLA.

@neoforged-pr-publishing
Copy link

  • Publish PR to GitHub Packages

@Matyrobbrt Matyrobbrt added enhancement New (or improvement to existing) feature or request 1.21 Targeted at Minecraft 1.21 labels Jun 25, 2024
Previously we only supported the player in the get remaining items
stage, this adds it to all stages of recipe evaluation and adds the
Level and CraftingContainer as well.
if (this.player instanceof ServerPlayer serverplayer) {
Level level = serverplayer.level();
CraftingInput craftinginput = this.container.asCraftInput();
+ // using the player-free context here since the player won't be usable by the dispense method
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this comment right? I see a serverplayer variable still being passed in. Looks the same as CraftingMenu patch

@Shadows-of-Fire
Copy link
Contributor

Would it be better to create either

  1. An extension interface for RecipeInput, with defaulted methods for the necessary context, and then patch in local wrappers for those calls instead of using static threadlocal context?
  2. The same approach as above, but with a non-extension interface that recipes would need to instanceof and downcast to (instead of nullchecking the static threadlocal)?

If there is preference to keep the static threadlocal, for whatever reason, it should be moved to a field on CraftingContext instead of CommonHooks so that the entire functionality is self-contained instead of being another random hunk in CommonHooks.

@XFactHD XFactHD added the breaking change Breaks binary compatibility label Jul 12, 2024
@Matyrobbrt Matyrobbrt added 1.21.1 Targeted at Minecraft 1.21.1 and removed 1.21 Targeted at Minecraft 1.21 labels Jul 26, 2024
@neoforged-automation
Copy link

@Random832, this pull request has conflicts, please resolve them for this PR to move forward.

@TelepathicGrunt
Copy link
Contributor

Is this PR still active/desired or should it be closed? There's some comments still outstanding and merge conflicts that needs resolving

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.21.1 Targeted at Minecraft 1.21.1 breaking change Breaks binary compatibility enhancement New (or improvement to existing) feature or request needs rebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants