-
-
Notifications
You must be signed in to change notification settings - Fork 187
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
Fix EffectCure
syncing by introducing connection aware StreamCodec
#1041
Fix EffectCure
syncing by introducing connection aware StreamCodec
#1041
Conversation
…nto connection-aware-stream-codec # Conflicts: # patches/net/minecraft/world/effect/MobEffectInstance.java.patch
Last commit published: e71a8d4206bebeae304a4e0aa6f0168b080f3685. 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 #1041' // https://github.com/neoforged/NeoForge/pull/1041
url 'https://prmaven.neoforged.net/NeoForge/pr1041'
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-pr1041
cd NeoForge-pr1041
curl -L https://prmaven.neoforged.net/NeoForge/pr1041/net/neoforged/neoforge/20.6.121-beta-pr-1041-connection-aware-stream-codec/mdk-pr1041.zip -o mdk.zip
jar xf mdk.zip
rm mdk.zip || del mdk.zip To test a production environment, you can download the installer from here. |
patches/net/minecraft/network/RegistryFriendlyByteBuf.java.patch
Outdated
Show resolved
Hide resolved
patches/net/minecraft/network/RegistryFriendlyByteBuf.java.patch
Outdated
Show resolved
Hide resolved
patches/net/minecraft/world/effect/MobEffectInstance.java.patch
Outdated
Show resolved
Hide resolved
src/main/java/net/neoforged/neoforge/network/codec/NeoForgeStreamCodecs.java
Outdated
Show resolved
Hide resolved
patches/net/minecraft/world/effect/MobEffectInstance.java.patch
Outdated
Show resolved
Hide resolved
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.
Looks fine to me now. Have you tested whether connecting an NF server to a vanilla client and vice-versa and then interacting with something referencing effects such as potions works fine?
After the latest commit replacing |
patches/net/minecraft/world/effect/MobEffectInstance.java.patch
Outdated
Show resolved
Hide resolved
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.
Seems fine with me. If we encounter problems with the type-tightening from ByteBuf
to RFBB on the mob effect instance codec, we can adjust it to downcast when available.
I suspect we will not encounter such issues, I don't envision anyone syncing specifically MobEffectInstance.Details
outside of the Play phase, given that the only usage is in MobEffectInstance
which requires RFBB.
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.
Basically two suggestions. What do you think @Shadows-of-Fire and @XFactHD?
src/main/java/net/neoforged/neoforge/common/util/FriendlyByteBufUtil.java
Show resolved
Hide resolved
e71a8d4
Due to the nature of the
StreamCodec
, modifying vanillaStreamCodec
s due to NeoForge's extra data has always been hard when it comes to keep NeoForge<->Vanilla connection compatibility.In some cases, we can make use of unused fields like list size (like in the
Ingredient
'sStreamCodec
) to conditionally write or read NeoForge specific data, but that's not always applicable, like #986.The workaround for #986 in this PR is to patch
RegistryFriendlyByteBuf
to let it contain NeoForge'sConnectionType
and thus providing context forStreamCodec
to handle serialization/deserialization differently forNEOFORGE
andOTHER
connections.