-
-
Notifications
You must be signed in to change notification settings - Fork 270
Reimplement on-demand synchronization of recipe data to clients #2021
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
Conversation
Last commit published: 22c64047c403ba7002325a3515e7ea6fbfddb03a. PR PublishingThe artifacts published by this PR:
Repository DeclarationIn order to use the artifacts published by the PR, add the following repository to your buildscript: repositories {
maven {
name 'Maven for PR #2021' // https://github.com/neoforged/NeoForge/pull/2021
url 'https://prmaven.neoforged.net/NeoForge/pr2021'
content {
includeModule('net.neoforged', 'neoforge')
includeModule('net.neoforged', 'testframework')
}
}
}MDK installationIn order to setup a MDK using the latest PR version, run the following commands in a terminal. mkdir NeoForge-pr2021
cd NeoForge-pr2021
curl -L https://prmaven.neoforged.net/NeoForge/pr2021/net/neoforged/neoforge/21.4.123-beta-pr-2021-feature-full-recipe-sync/mdk-pr2021.zip -o mdk.zip
jar xf mdk.zip
rm mdk.zip || del mdk.zipTo test a production environment, you can download the installer from here. |
|
@mezz @emilyploszaj Your feedback would be appreciated since this only makes sense if you do not sync recipes in addition to this feature. |
|
Feedback from maty: The client-side event should also fire if a NF modded client connects to a Vanilla server to indicate no recipes are going to arrive. |
marchermans
left a comment
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.
Some small questions that you can decide yourself if you want to address them.
Approved.
patches/net/minecraft/client/multiplayer/ClientPacketListener.java.patch
Outdated
Show resolved
Hide resolved
tests/src/main/java/net/neoforged/neoforge/oldtest/client/RecipeSyncTest.java
Show resolved
Hide resolved
src/main/java/net/neoforged/neoforge/event/OnDatapackSyncEvent.java
Outdated
Show resolved
Hide resolved
src/main/java/net/neoforged/neoforge/event/OnDatapackSyncEvent.java
Outdated
Show resolved
Hide resolved
src/main/java/net/neoforged/neoforge/network/payload/RecipeContentPayload.java
Outdated
Show resolved
Hide resolved
For clarification, this means mods like JEI and EMI would need to be present on the server to receive recipes, rather than being able to send a packet from the client and request these recipes? This would be unfortunate for the current design of these mods which, prior to 1.21.1, were able to function well client only. |
Yes that is correct, if either mod sends all recipe types, it will work with swapped out mods on the client too. (as in, JEI on server, EMI on client). The reasoning behind this in the NF Discord was that some servers may decide to make custom "hidden" recipes now that they would not want their players to have in a Vanilla+ scenario. If we allowed clients to request recipes unilaterally, we'd be exposing more information than Vanilla by default and without recourse for these servers. |
|
Personally, I think it'd benefit the average use case if recipes could be requested by default but mods that do have those secrecy concerns could either not have serializers for their recipes, or have a Neo endpoint to disable syncing of specific recipes/types. I think the number of users who setup a neo server with just create and farmer's delight and have players who want to use JEI or EMI on their own instance outweighs the somewhat niche case of server mods wanting to hide information, but either way both can be accommodated with a default on approach. WDYT? |
|
I think the concern was more server owners using light Vanilla+ with custom data pack, not a mod in particular. |
|
I'm unsure if the fallback behavior will simply be no function on vanilla servers or some bespoke client datapack solution. I'll still stand by the concern being contrived and I think it'd cause more benefit than harm. |
|
Could a config option be provided to allow server owners to turn on syncing in some way without requiring installing a full mod? Ie a list of recipe types (prefferably with wildcard support, so you can specify |
|
As one comment, I don't think JEI or EMI should request all recipe types. Given how JEIs API works, it's always optional in for mods to add their recipes to the recipe viewer, so it makes sense that mods will either manually request the types they plant to show in JEI, or they will request the types through JEIs API. Reason this is worth mentioning is some recipe types simply have no support to display in JEI, so aren't worth syncing. |
|
🚀 This PR has been released as NeoForge version |
Motivation: Mods like JEI and EMI have signaled that they plan to synchronize all recipe data to clients following the removal of that from Vanilla after 1.21.1. If these rather ubiquitous mods plan to do this, having this feature as part of the platform would allow a lot of other mods to have a far easier time porting, so this PR aims to introduce said platform feature.
Design: We reuse the existing OnDatapackSync event, which fires server-side, to allow mods to request specific recipe types to be sent to the player. We then collect all recipes for these recipe types and send them similar to how they've been sent in previous Minecraft versions. When the client-side receives the payload, it fires a new event
RecipesReceivedEvent, which signals 1) for which recipe types the client received data and 2) aRecipeMapcontaining these recipes.Notes: