Skip to content
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

[1.21] Add ICustomHolderSet support for StreamCodec created from ByteBufCodecs#holderSet #991

Merged
merged 15 commits into from
Jul 10, 2024

Conversation

RaymondBlaze
Copy link
Contributor

Fixes #981.
This PR adds support for ICustomHolderSet for StreamCodec created from ByteBufCodecs#holderSet.
Introduced tests for HolderSet related Codec and StreamCodec.

Changes

  • HolderSetType is no longer a functional interface due to the newly introduced makeStreamCodec method.
  • NeoForgeRegistries.HOLDER_SET_TYPES is now synced as ByteBufCodecs#holderSet's implementation uses raw id for HolderSetType.

Implementation Note

  • ByteBufCodecs#holderSet's implementation made use of the vanilla unused negative VarInt values(starting from -1) as HolderSetType ids to support ICustomHolderSet.
  • StreamCodec instance for ICustomHolderSet created from HolderSetType#makeStreamCodec method are lazily computed and cached in the StreamCodec instance created from ByteBufCodecs#holderSet for better performance, which is a bit different from the implementation of HolderSetCodec which doesn't cache the created Codec from HolderSetType#makeCodec.

@CLAassistant
Copy link

CLAassistant commented May 22, 2024

CLA assistant check
All committers have signed the CLA.

@neoforged-pr-publishing
Copy link

  • Publish PR to GitHub Packages

Copy link
Member

@XFactHD XFactHD left a comment

Choose a reason for hiding this comment

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

This approach will blow up on a NeoForge -> Vanilla connection because vanilla will try to handle the custom holder sets as HolderSet.Directs: The decoder will first attempt to create an ArrayList with an initial size of Math.min(varInt, 65536) which will throw an IllegalArgumentException due to a negative value. If the decoder were to get past this list issue, then the data emitted by the specific implementations will either misbehave or crash when the decoder attempts to read the first entry: The AndHolderSet and OrHolderSet write a List<HolderSet> (would attempt to read the list size as the first Holder or part thereof), the NotHolderSet writes a nested HolderSet (would attempt to read the direct holder size or named holder set marker [the var-int being repurposed in this PR] as the first Holder or part thereof) and the AnyHolderSet writes nothing (would attempt to read past the buffer's end or into data that should be handled by another decoder).

I currently have no idea how this could be solved in a way that is vanilla-compatible. While it's definitely not great in terms of packet size (especially for the AnyHolderSet), I would consider it safer to keep the fallback to HolderSet.Direct for custom holder sets for the time being as it retains full vanilla network compatibility.

@lukebemish
Copy link
Contributor

Beyond that, wouldn't making NeoForgeRegistries.HOLDER_SET_TYPES synced blow up those connections even earlier? Also not sure what a sensible solution here is.

@XFactHD
Copy link
Member

XFactHD commented May 23, 2024

No, marking a registry as synced does not blow up the connection as registry sync only happens on NF<->NF connections

@RaymondBlaze
Copy link
Contributor Author

This approach will blow up on a NeoForge -> Vanilla connection

This only happens if a ICustomHolderSet is used, I would argue the behavior of "sending ICustomHolderSet to Vanilla" itself should not expect any success.
Note that the patched StreamCodec for Ingredient is already using the same implementation that make use of illegal list size VarInt, and the above argue also applies to it.

@RaymondBlaze RaymondBlaze requested a review from XFactHD May 24, 2024 01:43
…odec

# Conflicts:
#	src/main/java/net/neoforged/neoforge/registries/NeoForgeRegistries.java
@RaymondBlaze RaymondBlaze changed the base branch from 1.20.x to 1.21.x June 18, 2024 07:47
@RaymondBlaze RaymondBlaze changed the title [1.20.6] Add ICustomHolderSet support for StreamCodec created from ByteBufCodecs#holderSet [1.21] Add ICustomHolderSet support for StreamCodec created from ByteBufCodecs#holderSet Jun 18, 2024
@RaymondBlaze
Copy link
Contributor Author

Changed target to 1.21.x, added checks for ConnectionType and vanilla connection should be happy with ICustomHolderSets now.

@neoforged-compatibility-checks
Copy link

neoforged-compatibility-checks bot commented Jun 25, 2024

@RaymondBlaze, this PR introduces breaking changes.
Fortunately, this project is currently accepting breaking changes, but if they are not intentional, please revert them.
Last checked commit: 47f81c67ba3b01598273c77e1f3aafafc464d9f2.

Compatibility checks

neoforge (:neoforge)

  • net/neoforged/neoforge/registries/holdersets/OrHolderSet
    • codec(Lnet/minecraft/resources/ResourceKey;Lcom/mojang/serialization/Codec;Z)Lcom/mojang/serialization/MapCodec;: ❗ API method was removed
  • net/neoforged/neoforge/registries/holdersets/NotHolderSet
    • codec(Lnet/minecraft/resources/ResourceKey;Lcom/mojang/serialization/Codec;Z)Lcom/mojang/serialization/MapCodec;: ❗ API method was removed
  • net/neoforged/neoforge/registries/holdersets/HolderSetType
    • makeStreamCodec(Lnet/minecraft/resources/ResourceKey;)Lnet/minecraft/network/codec/StreamCodec;: ❗ Method was made abstract
  • net/neoforged/neoforge/registries/holdersets/AnyHolderSet
    • codec(Lnet/minecraft/resources/ResourceKey;Lcom/mojang/serialization/Codec;Z)Lcom/mojang/serialization/MapCodec;: ❗ API method was removed
  • net/neoforged/neoforge/registries/holdersets/AndHolderSet
    • codec(Lnet/minecraft/resources/ResourceKey;Lcom/mojang/serialization/Codec;Z)Lcom/mojang/serialization/MapCodec;: ❗ API method was removed

@RaymondBlaze RaymondBlaze requested a review from XFactHD July 2, 2024 09:11
XFactHD
XFactHD previously approved these changes Jul 2, 2024
@XFactHD XFactHD added the regression Worked previously but doesn't anymore label Jul 2, 2024
@XFactHD XFactHD added breaking change Breaks binary compatibility 1.21 Targeted at Minecraft 1.21 networking Related to networking labels Jul 2, 2024
Copy link
Contributor

@Shadows-of-Fire Shadows-of-Fire left a comment

Choose a reason for hiding this comment

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

Impl looks fine. Couple changes needed for sanity, then should be good to go.

@XFactHD XFactHD merged commit d043a92 into neoforged:1.21.x Jul 10, 2024
6 checks passed
@RaymondBlaze RaymondBlaze deleted the holder-set-stream-codec branch July 16, 2024 06:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.21 Targeted at Minecraft 1.21 breaking change Breaks binary compatibility networking Related to networking regression Worked previously but doesn't anymore
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[1.20.6] ByteBufCodecs#holderSet missing ICustomHolderSet specific implementation
5 participants