[1.21.1-1.21.4] Custom Ingredients sync fix #4322
Conversation
|
Hi, got a few questions: is there a reason why this targets 1.21.1 and not 1.21.4? Could this be a backport of #4287 instead? Ill have to have a proper look with the code in front of me as well. |
|
I targeted 1.21.1 because my Sophisticated Storage port is still on 1.21.1 so I would need the fix there. I want to make 2 more PRs against .3 and .4 just wanted to get the okay first that the changes are okay and not do the work twice. #4287 is unfortunately not a fix to the issue, I tried to explain my findings in #4225. I can look into backporting #4276 and what applies from #4287 and either add them here or make a new PR for it. Yeah please take your time, I would like to have MerchantPug confirm the fix works for them too so we can be sure that there isn't a second bug. |
| @@ -26,4 +26,6 @@ | |||
| */ | |||
| public interface SupportedIngredientsPacketEncoder { | |||
There was a problem hiding this comment.
This interface should be renamed to SupportedIngredientsClientConnection to match the name of the class it's actually implemented on.
There was a problem hiding this comment.
Will change that and also fix the comment as it was still on EncoderHandler
There was a problem hiding this comment.
It's not necessary to change the indentation of the whole file. All Fabric API mixin json files use 2 spaces for indentation.
There was a problem hiding this comment.
Ups, will undo that
| @@ -51,7 +40,11 @@ public void fabric_setSupportedCustomIngredients(Set<Identifier> supportedCustom | |||
| method = "encode(Lio/netty/channel/ChannelHandlerContext;Lnet/minecraft/network/packet/Packet;Lio/netty/buffer/ByteBuf;)V" | |||
| ) | |||
| private void capturePacketEncoder(ChannelHandlerContext channelHandlerContext, Packet<?> packet, ByteBuf byteBuf, CallbackInfo ci) { | |||
| CustomIngredientSync.CURRENT_SUPPORTED_INGREDIENTS.set(fabric_supportedCustomIngredients); | |||
| ChannelHandler client = channelHandlerContext.pipeline().get("packet_handler"); | |||
There was a problem hiding this comment.
Nit: client as a variable name here doesn't make sense to me. I think a simple channelHandler would be fine.
| CustomIngredientSync.CURRENT_SUPPORTED_INGREDIENTS.set(fabric_supportedCustomIngredients); | ||
| ChannelHandler client = channelHandlerContext.pipeline().get("packet_handler"); | ||
|
|
||
| if (client != null) { |
There was a problem hiding this comment.
It might be better to replace this check with client instanceof SupportedIngredientsPacketEncoder.
|
Will test this soonish. |
Before PR
After PR
Looks like it works for me! 👍 |
modmuss50
changed the title
modmuss50
added
bug
|
dumb question but does this changes the behavior i reported in #4170 in any way? |
|
@vico93 yes and no, the recipe book will still not work with this. But your client side is now aware of custom ingredients again meaning that if REI has support for custom ingredients it should work but from my own testings with REI I at least need to write a custom TransferHandler, unfortunately I'm not sure if this is required for you too. EMI is a bit nicer in that regard as I only needed to override the recipe and let EMI know that it needs to use a ListIngredient but for that I need to know about the current recipe and its custom ingredient on the client side, |
Fixes #4225.
The synced
fabric_supportedCustomIngredientslist was lost due to minecraft removing theEncoderHandlerbefore joining the world.Therefore all recipes were synced with the fallback method instead.
This PR fixes that issue by storing the list in the
ClientConnectionclass instead and making use of thepacket_handlerpipeline which points to theClientConnectioninstance.@MerchantPug could you maybe test this fix against your implementation?