Skip to content

Remove duplicated reloads while calling addRecipe() - #14301

Open
Ph0sphorW wants to merge 4 commits into
PaperMC:mainfrom
Ph0sphorW:fix/recipe-add-duplication
Open

Ph0sphorW wants to merge 4 commits into
PaperMC:mainfrom
Ph0sphorW:fix/recipe-add-duplication

Conversation

@Ph0sphorW

Copy link
Copy Markdown

I forgot to create a new branch first sry

While trying to add a recipe through this method, the server will resent the ClientboundUpdateRecipesPacket twice.

I found this when I was testing my custom recipe loading plugin. When I tried to reload all the custom recipes (had 150 of them...) my client had no response for a half a minute. After checking client logs I found that my client reloaded the recipe book for 600 times.

The full calling chain before:

CraftServer.addRecipe(...)
  → toAdd.addToRecipeManager()
    → RecipeManager.addRecipe(...)
      → finalizeRecipeLoading()
        → PlayerList.reloadResources()
          → reloadRecipes()          // 1
  → if (true || resendRecipes)
      → reloadRecipes()              // 2

We'd see that the playerList.reloadRecipes() ran twice in total, and remove the second reloadRecipes() didn't affect the reloading at all.
So here's the fixed logic (after):

CraftServer.addRecipe(...)
  → toAdd.addToRecipeManager()
    → RecipeManager.addRecipe(...)
      → finalizeRecipeLoading()
        → PlayerList.reloadResources()
          → reloadRecipes()          // 1

I didn't remove the 2nd param for backward compatibility issues.
Sorry for my poor English expression🙏🏻

@Ph0sphorW
Ph0sphorW requested a review from a team as a code owner September 24, 2026 14:42
@github-project-automation github-project-automation Bot moved this to Awaiting review in Paper PR Queue Sep 24, 2026
@Doc94

Doc94 commented Sep 24, 2026 •

Copy link
Copy Markdown
Member

Hi, Thanks for the PR, yeah looks like the logic its called twice.. this also happen with the removeRecipe, if you can remove that reload too i feel this PR its ok.

maybe deprecate that methods because are always reloaded internally...

if (toAdd == null) return false;
toAdd.addToRecipeManager();
// Paper start - API for updating recipes on clients
if (true || resendRecipes) { // Always needs to be resent now... TODO

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Since this was with a TODO, maybe it would make sense to make it false && and update the command to say that it's already resent?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

i dont see a reason for keep this in code when NMS already make the reload maybe mention here in the addToRecipeMananger with a comment about how that call a reloadResources -> reloadRecipes

@Ph0sphorW Ph0sphorW Sep 24, 2026 •

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

For sure add a comment would be better. My bad
I've added them in new commit

If there's any issue you may change it directly cuz I may not reply timely thx

@@ -1644,10 +1640,8 @@ public boolean removeRecipe(NamespacedKey recipeKey, boolean resendRecipes) {

// Paper start - resend recipes on successful removal

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This comments can be removed (the Paper end too and maybe inline the return with the remove)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Got that. Removed in latest commit

@Doc94

Doc94 commented Sep 24, 2026

Copy link
Copy Markdown
Member

Can be good deprecate the methods where the bool is passed because its totally ignored.

@JustAHuman-xD

Copy link
Copy Markdown
Contributor

this is actually covered in my pre-existing recipe PR here: #13945

@JustAHuman-xD

Copy link
Copy Markdown
Contributor

this is actually covered in my pre-existing recipe PR here: #13945

Which I see now I need to update with paper's update to the new mc version

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Awaiting review

Development

Successfully merging this pull request may close these issues.

4 participants