Add several codecs for existing capabilities. Add NbtListCollector. #1035
Conversation
|
| Codec.INT.fieldOf("energy").forGetter(x -> x.energy) | ||
| ).apply(i, EnergyStorage::new)); | ||
|
|
||
| public static final StreamCodec<FriendlyByteBuf, EnergyStorage> STREAM_CODEC = StreamCodec.composite( |
There was a problem hiding this comment.
Generally, I think this should be two stream codecs: one for sending only the energy component and the rest for all the information. Most do not generally need to increase the other parameters on the client after the initial construction.
There was a problem hiding this comment.
Doesn't most codec-based syncing just replace the object entirely on the client? Dealing with deltas is annoying. :D
There was a problem hiding this comment.
Yes, but you shouldn't send more information than necessary. For example, you could just sync a single int representing the current energy in 90% of cases.
There was a problem hiding this comment.
If you just need to sync a single int, then use ByteBufCodecs and do it yourself?
src/main/java/net/neoforged/neoforge/fluids/capability/templates/FluidTank.java
Outdated
Show resolved
Hide resolved
src/main/java/net/neoforged/neoforge/items/ItemStackHandler.java
Outdated
Show resolved
Hide resolved
| public static Codec<ItemStackHandler> codec(HolderLookup.Provider provider) { | ||
| return new ItemStackHandlerCodec(provider); | ||
| } |
There was a problem hiding this comment.
Again with only a codec and no stream codec? I can understand the reason why, but we shouldn't assume somebody will not need to synchronize themselves.
There was a problem hiding this comment.
You can wrap the existing (complicated) codec into a Stream codec if needed, but I can work on an implementation in a bit.
| @@ -173,4 +181,49 @@ protected void validateSlotIndex(int slot) { | |||
| protected void onLoad() {} | |||
|
|
|||
| protected void onContentsChanged(int slot) {} | |||
|
|
|||
| public record ItemStackHandlerCodec(HolderLookup.Provider provider) implements Codec<ItemStackHandler> { | |||
There was a problem hiding this comment.
Storing a HolderLookup.Provider like this is incorrect because it can change. It should be retrieved using RegistryOps if you really need it, but I think that the downstream codecs should already handle it?
Having done this in a couple of my mods before, it actually ends up becoming a messy hybrid of the two approaches. It's far easier to deprecate NBT if possible, and tell people to use the codecs directly instead. (ie MC 21, remove |
src/main/java/net/neoforged/neoforge/fluids/capability/templates/FluidTank.java
Outdated
Show resolved
Hide resolved
src/main/java/net/neoforged/neoforge/items/ItemStackHandler.java
Outdated
Show resolved
Hide resolved
ApexModder
added
enhancement
| stream.accept(decoder::process); | ||
| }); | ||
|
|
||
| return Pair.of(finalList, input); |
There was a problem hiding this comment.
The returned value of T should be the results that could not be decoded.
| ops.getList(map.get("items")).ifSuccess(stream -> { | ||
| stream.accept(decoder::process); | ||
| }); |
There was a problem hiding this comment.
I think the failure should be reported here rather than consumed into the ether.
src/main/java/net/neoforged/neoforge/codec/NonNullListCodecs.java
Outdated
Show resolved
Hide resolved
| Codec.INT.fieldOf("energy").forGetter(x -> x.energy) | ||
| ).apply(i, EnergyStorage::new)); | ||
|
|
||
| public static final StreamCodec<FriendlyByteBuf, EnergyStorage> STREAM_CODEC = StreamCodec.composite( |
There was a problem hiding this comment.
Yes, but you shouldn't send more information than necessary. For example, you could just sync a single int representing the current energy in 90% of cases.
| * @return | ||
| */ | ||
| @Deprecated | ||
| public static Codec<ItemStackHandler> nbtCompatibleCodec(HolderLookup.Provider provider) { |
There was a problem hiding this comment.
Is the only intended usecase for this to be serialized to NBT? If it's serialized to JSON, we may want to put some safeguards in place to prevent crashes during number list conversions.
There was a problem hiding this comment.
It may be useful to convert old NBT data into JSON for backups or something. Why guard it?
There was a problem hiding this comment.
The issue occurs when converting JSON number lists to NBT. Basically, the conversion that NBTOps uses checks for the lowest number of bytes needed to represent a number. So, if your first number starts with 8, the ops will create a byte list tag. However, if your numbers are not all represented by bytes in this example, it will crash. For example, converting the list [8, -1, 32914] will crash. So, there needs to be an extra safeguard when transforming between the two.
There was a problem hiding this comment.
I think that having builtin codecs for the capability templates is a good idea.
However, I think that adding all this codec code to support the old serialization format is not worth it. Given that 1.21 is around the corner, shouldn't we instead change the data format to match modern expectations?
By the way, we might want to wait to see how the transfer rework affects these base implementations before updating them.
I also don't like the new provided codecs. They seem too specific to me.
| * @param indexKey A key used for serializing list index information. | ||
| * @param <E> Type of elements in list. | ||
| */ | ||
| public record IndexedNonNullListCodec<E>(MapCodec<E> elementCodec, E defaultValue, Predicate<E> skipPredicate, String indexKey) implements Codec<NonNullList<E>> { |
There was a problem hiding this comment.
This class is oddly specific to the old serialization pattern. Shouldn't we be moving towards always serializing entries?
There was a problem hiding this comment.
If we're willing to break old-NBT support, this could be simplified quite a bit to no longer require a MapCodec. I believe newer codecs for stuff like inventories (a prime usage of NNL) simply encode the data as a group alongside the index, rather than inlining.
| * | ||
| * @return A non-null list codec for item stacks. | ||
| */ | ||
| public static Codec<NonNullList<ItemStack>> optionalItems() { |
There was a problem hiding this comment.
Item-specific code shouldn't be in this more generic class.
There was a problem hiding this comment.
This was requested by @ChampionAsh5357 (#1035 (comment)) and I'd agree - a lot of usages of NNL is storage for items, why not provide a simple utility for that?
| * | ||
| * Usage: {@code Stream.doStuff().collect(NbtListCollector.toNbtList())} | ||
| */ | ||
| public class NbtListCollector implements Collector<Tag, ListTag, ListTag> { |
There was a problem hiding this comment.
Wouldn't this be more broadly applicable as a static method to build an NbtList from a list of tags? (I am surprised that this doesn't already exist, but couldn't find it).
There was a problem hiding this comment.
Maybe I'm interpreting badly, but there's a difference between doing a stream collection of Tags into an NbtList (what this does) and turning a standard collection of Tags into an NbtList (non-streaming)?
There was a problem hiding this comment.
After all, a list of Tag being put into a ListTag is simple code; a foreach will get you there no problem?
|
Superceded by #1163, which adds codecs for INBTSerializable. Seeing as that is a primary usecase and stream codecs can wrap that, this can be re-visited after the transfer reworks |
This is a best-effort PR to add Codec support to the existing capabilities provided by NeoForge. It also adds a NBT List collector for usage with Java stream APIs.