Adding PredicateChoice to Paper API (updated version) - #12017
Conversation
CPieter
changed the title
lynxplay
left a comment
lynxplay
left a comment
There was a problem hiding this comment.
Welcome to paper 🎉
Thank you for updating the PR. I don'T have time for a indepth review rn, but I left some initial easy nitpicks around comments 👍
Will get to a review next week probably
|
Hi and welcome to the Paper PR world (well this more a thing can say a Paper Team Member xd) The paper comments currently just apply to code in patches, its not required in the api,craftbukkit,paper classes. |
|
I just noticed there is already a PredicateRecipeChoice for potions, with the addition of the PredicateChoice this could be a bit confusing. Maybe it should be renamed to something more clear like a PotionRecipeChoice/PotionPredicateChoice? |
well rename sounds a little breaking i think, maybe this new PredicateChoice can be named ItemPredicateChoice (? or add an extension of RecipeChoice for Potion and move things to that for make clear the diff... but not really sure. |
Since PredicateRecipeChoice isn't public, delete it, and make Here's a great example of what I talked about in my review, suggesting to keep implementation types out of the "api" because we can now just delete PredicateRecipeChoice entirely. |
Machine-Maker
left a comment
Machine-Maker
left a comment
There was a problem hiding this comment.
Looks good, just a few more things. I also opened the PR adding the future alternative to MaterialChoice. Will merge mine in after yours
Most issues with the PR should now be resolved, but I'm not quite sure how to properly replace the |
|
Everything looks mostly good, some small style stuff, but we can fix that ourselves right before we merge it.
I think what we can do is just create a dummy example stack there. The only place the old predicate choice was used was in potion mixes where the example stack didn't matter. So just creating a random itemstack isn't going to break anything unless people start using the now-deprecated method for actual recipes. Which is their problem. |
|
After additional testing I've found that the predicate matching works unpredictably when used with shapeless recipes. I've converted the PR to a draft while I figure out the issue. |
|
Thanks for continuing to work on the idea! All the best, Verdox |
|
Hey @CPieter any update on this feature? I'd be really glad to see this live |
Replace RecipeChoice class with an interface + records, add PredicateChoice, implement recipe book matching, and switch over potion mixing recipes.
3f515a9 to
9255caf
Compare
Machine-Maker
left a comment
Machine-Maker
left a comment
There was a problem hiding this comment.
Ok, I found the issue, (ty Claude) and did a bunch of testing, I think this is fixed now. Works for shaped/shapeless and the recipe book.
papermc-projects
Bot
moved this from Changes required
to Full approval
in Paper PR Queue
|
Ok, addressed reviews |
lynxplay
left a comment
lynxplay
left a comment
There was a problem hiding this comment.
Not much of a fan of the test logic, maybe the models of today can emit something more logical. A full test matrix seems possibly better but requires a good bit of work. So for now they can stay in.
LGTM
github-project-automation
Bot
moved this from Full approval
to Merged
in Paper PR Queue
Upstream has released updates that appear to apply and compile correctly Paper Changes: PaperMC/Paper@1797fbcc Add PredicateChoice (PaperMC/Paper#12017) [closes #1460] PaperMC/Paper@a2a42c5b Correctly consume chunkStatus in loadChunksAsync (PaperMC/Paper#14220)
7 participants
This PR is an updated version of derverdox's PredicateChoice PR #9996. Since the branch in the old PR was quite outdated I reïmplemented the changes in a new branch, but most of the changes remain the same. Credit of the code should go to derverdox and Machine-Maker.
The feature seems to work fine from my testing but I'm unfamiliar with the Paper API so I please let me know if I missed something.