Initial Commit: CustomRegistryEntryLists. #4680
Conversation
...fabricmc/fabric/api/event/registry/entrylists/CustomRegistryEntryListSerializerRegistry.java
Outdated
Show resolved
Hide resolved
...ain/java/net/fabricmc/fabric/impl/registry/entrylists/defaults/InverseRegistryEntryList.java
Show resolved
Hide resolved
.../main/java/net/fabricmc/fabric/api/event/registry/entrylists/DependentRegistryEntryList.java
Outdated
Show resolved
Hide resolved
|
Please can you improve the description of this PR what it does and why its needed for someone like me who isnt familar with this. Linking to other tickets and mod loaders means I have to assume that what they said is exactally what you have here. This is a very large PR that looks very complex but provides no guideance on where to start. |
Sure. I also did a lot of javadocing today that I thought I already did, but apparently didn't |
| @ApiStatus.NonExtendable | ||
| public interface FabricRegistryEntryList<T> { | ||
| // creating a cycle in the graph will lead to stack overflow, do with this knowledge what you will | ||
| Map<RegistryEntryList<?>, Set<RegistryEntryList<?>>> DEPENDENCIES = new WeakHashMap<>(); |
There was a problem hiding this comment.
I think it should not be public, i.e. it should be moved to a separate impl class
| private static boolean initialized = false; | ||
|
|
||
| public static void register() { | ||
| if (initialized) { | ||
| return; | ||
| } | ||
|
|
||
| initialized = true; | ||
| CustomRegistryEntryListSerializerImpl.registerSerializer(UnionRegistryEntryList.SERIALIZER); | ||
| CustomRegistryEntryListSerializerImpl.registerSerializer(IntersectionRegistryEntryList.SERIALIZER); | ||
| CustomRegistryEntryListSerializerImpl.registerSerializer(InverseRegistryEntryList.SERIALIZER); | ||
| CustomRegistryEntryListSerializerImpl.registerSerializer(UniversalRegistryEntryList.SERIALIZER); | ||
| } |
There was a problem hiding this comment.
It would be cleaner to move this to FabricRegistryInit directly and get rid of the unnecessary initialized field
| * limitations under the License. | ||
| */ | ||
|
|
||
| package net.fabricmc.fabric.impl.registry.sync.entrylists.defaults; |
There was a problem hiding this comment.
It doesn't matter, but for consistency you can rename defaults to builtin just like with the Custom Ingredient API: https://github.com/FabricMC/fabric/blob/1.21.6/fabric-recipe-api-v1/src/main/java/net/fabricmc/fabric/impl/recipe/ingredient/builtin/AllIngredient.java
| * | ||
| * @param <T> type of entries held by this list | ||
| */ | ||
|
|
| @Unique | ||
| private static final String COMMENT_IN_MIXIN_EXPORT = "-45 is ours. if anyone else uses it, they deserve the consequences"; | ||
| @Unique | ||
| private static final int FABRIC_CUSTOM_REGISTRY_LIST_ID = -45; |
There was a problem hiding this comment.
I think it is better to put such constants in the public API? Or at least put them out of the mixin into some general class like FabricRegistryInit and then we can removeCOMMENT_IN_MIXIN_EXPORT
| ) | ||
| private <T> void bindFabricCodec(RegistryKey<? extends Registry<T>> registry, Codec<RegistryEntry<T>> entryCodec, boolean alwaysSerializeAsList, CallbackInfo ci) { | ||
| fabric$codec = CustomRegistryEntryListSerializerImpl.SERIALIZER_CODEC.dispatch( | ||
| "fabric:type", |
There was a problem hiding this comment.
(edit for clarification)
"fabric:type"
It's worth putting it in some sort of general class, just like with: https://github.com/FabricMC/fabric/blob/1.21.6/fabric-recipe-api-v1/src/main/java/net/fabricmc/fabric/impl/recipe/ingredient/CustomIngredientImpl.java#L49
| cancellable = true | ||
| ) | ||
| private <T> void encodeCustomRegistryEntryList(RegistryByteBuf registryByteBuf, RegistryEntryList<T> registryEntryList, CallbackInfo ci) { | ||
| if (!ChannelUtil.isInstalled(registryByteBuf) || !(registryEntryList instanceof CustomRegistryEntryList<T> customRegistryEntryList)) { |
There was a problem hiding this comment.
This will fail if the client has installed Fabric API, without mods adding more custom registry entry lists. Case being people with Fabric API and some client only mods playing on otherwise vanilla compatible fabric server. See Fabric's ingredient api for reference
There was a problem hiding this comment.
Could you explain more, I'm not sure what you mean?
| ) | ||
| private <T, E> void encodeCustomWithCustomCodec(RegistryEntryList<E> registryEntryList, DynamicOps<T> dynamicOps, T prefix, CallbackInfoReturnable<DataResult<T>> cir) { | ||
| if (registryEntryList instanceof CustomRegistryEntryList<E> customRegistryEntryList) { | ||
| cir.setReturnValue(fabric$codec.encode(customRegistryEntryList, dynamicOps, prefix)); |
There was a problem hiding this comment.
Also applies here, through currently fabric api has no way to detect if codec is running on net thread. This could fail in case of vanilla clients and biomes / etc using custom entry lists (including fabric ones)
4 participants
Implements CustomRegistryEntryLists in an attempt to fill the hole described in #3372. inspiration taken from #2586 and MinecraftForge/MinecraftForge#8928.
Edit for Clarity:
What it does:
This Pr adds the ability for modders to provide custom RegistryEntryList implementations, with codecs, and packet codecs, and also provides default implements of common set operations such as union, intersection, negation and subtraction, as well as a universal set, containing anything contained in the target registry.
in json, these lists and their types can be identified by the
fabric:typekey.the implementation should also be compatible with vanilla clients, although I haven't had the opportunity to test this
Why it's needed:
In many places, like worldgen and enchantments, modders must express a subset of a registry as a list. vanilla provides RegistryEntryList to accomplish this, but in certain modded scenarios, it can be cumbersome to describe specific relations, such as everything in
minecraft:flowers, but withoutminecraft:poppyor everywhere that oak tress spawn, except places that are beaches. In vanilla, a prospective tagger must create a separate tag that includes (manually) everything in another tag, but leaves out the unliked entries. this is bad for compatibility because other mods cannot add to this list.Lmk if you need more.