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.18.x Custom datapack registries, round three-ish #8630

Merged
merged 43 commits into from
Jun 5, 2022

Conversation

Commoble
Copy link
Contributor

@Commoble Commoble commented May 21, 2022

This is a fork of #8522, Silverminer's custom datapack registries PR, with some improvements to documentation and error presentation and streamlining of userdev APIs.

Datapack Registries, what are they and why might modders want them

Datapack registries (sometimes called dynamic registries or worldgen registries) are a system added by Mojang several major versions ago, neither static registries nor reloadable data, but a third system separate from the other two. Datapack registries are created by registering a loading codec and (optionally) a network codec (if the network codec exists, we consider the registry to be a Synced Datapack Registry). Examples of vanilla datapack registries include biomes, dimensiontypes, and placed features.

Features of datapack registries (relative to reloadable data)

  • Datapack registries have a simplified standard implementation for loading and optionally syncing (which cannot be changed, as opposed to reloadable data which provides lower-level access to data)
  • Datapack registries have half-builtin support for datagen, using their loading codecs and RegistryOps
  • Datapack registries cannot be reloaded with /reload, they can only be reloaded by rebooting the server
  • Datapack registries automatically support tag jsons
  • Unsynced datapack registries' elements can directly reference any other datapack registries' elements

Lifecycle of datapack registries:

  • The datapack registry and its codec(s) are registered (custom datapacks can be registered via the NewRegistryEvent or DeferredRegisters)
  • "Builtin" objects can optionally be registered in java (via DeferredRegister/RegistryObjects). This is required to datagen objects using RegistryOps.
  • When the server starts, using the registries' loading codecs, builtin objects are deep-copied and pasted into the server's personal RegistryAccess, and then json data from datapacks is loaded and pasted into the server's RegistryAccess (possibly overriding builtin objects).
  • When a client logs in, any synced datapack registries and their contents are sent to the client using the network codecs, and stored in the client's personal RegistryAccess (which is stored in the client's connection). Clients do not have to have any of the server's registrables pre-registered (and any builtin registrables are ignored on the client), but the synced registries themselves are required to have been registered on the client.

Userdev APIs

When using a RegistryBuilder to register a registry (either via DeferredRegister or NewRegistryEvent), the userdev may invoke RegistryBuilder#dataPackRegistry(Codec<T> codec, @Nullable Codec<T> networkCodec), which marks the registry as a datapack registry and registers codecs for it. An overload that only takes the loading codec is also provided (using null for the network codec). If the network codec is non-null, the registry is considered to be a synced registry. Unsynced datapack registries registered on the server do not need to be registered on clients, but synced registries do. RegistryBuilder#disableSync is ignored as datapack registries use the presence of the network codec (or lack thereof) to determine whether to sync the registry.

Registries that are named modid:folder cause data to be loaded from data/<datapack_namespace>/modid/folder/. The folder path can include / for subdirectorying, e.g. worldgen/biome.

Network API Changes

  • Bump netversion from 2 to 3.
  • The S2CModList packet now may or may not contain a list of synced non-vanilla datapack registry IDs at the end of the packet bytestream.
  • Backwards-compatibility is kept for clients on netversion 2, see below. This backwards-compatibility may be dropped in 1.19 (sending an empty list instead of nothing).

End-User Concerns

If no changes are made to login netcode, then if a client were to attempt to connect to a server when the server has syncable datapack registries that don't exist on the client, the login packet fails to decode and the client is presented with an unreadable error message (due to the decoder dumping the entire contents of all synced datapack registries into the login error screen).

To avoid this, we validate the syncability of custom datapack registries in these ways:

Forge clients on netversion 2 will experience an "incompatible server" warning in the server list menu due to the netcode version mismatch, however they can still join the server if the server has no syncable custom datapack registries..

Summary of Patches

  • RegistryAccess has a hook patched into its codec registry so forge can add custom datapack registries' codecs later
  • RegistryResourceAccess has a patch to include the registry namespace in the directory (like tags) if namespace isn't "minecraft" and the registry was created via the RegistryBuilder API

Silverminer007 and others added 19 commits March 19, 2022 22:11
# Conflicts:
#	src/main/java/net/minecraftforge/common/ForgeHooks.java
# Conflicts:
#	patches/minecraft/net/minecraft/core/RegistryAccess.java.patch
#	src/test/resources/META-INF/mods.toml
# Conflicts:
#	src/test/resources/META-INF/mods.toml
@sciwhiz12 sciwhiz12 added Feature This request implements a new feature. Triage This request requires the active attention of the Triage Team. Requires labelling or reviews. 1.18 labels May 21, 2022
@sciwhiz12 sciwhiz12 removed the Triage This request requires the active attention of the Triage Team. Requires labelling or reviews. label May 25, 2022
@Commoble Commoble requested a review from sciwhiz12 May 28, 2022 01:02
Copy link
Contributor

@sciwhiz12 sciwhiz12 left a comment

Choose a reason for hiding this comment

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

I tried to test this PR using the provided test mod, but I got an error while trying to load a singleplayer world in DataPackRegistriesTest.ClientEvents#onClientTagsUpdated because the player is null (and the attempt to read the connection field fails). I've fixed it in my local copy by getting the RegistryAccess from TagsUpdatedEvent#getTagRegistry instead.


  • Datapack registries cannot be reloaded with /reload, they can only be reloaded by rebooting the server

[...]

Lifecycle of datapack registries:

[...]

  • When the server starts, using the registries' loading codecs, builtin objects are deep-copied and pasted into the server's personal RegistryAccess, and then json data from datapacks is loaded and pasted into the server's RegistryAccess (possibly overriding builtin objects).

To clarify for me, this means that only the JSON data from datapacks which affect a datapack registry that are present during server startup will affect the contents of the datapack registry, even if said JSON data is removed later on and a /reload is issued?

() -> new RegistryBuilder<Unsyncable>().disableSaving().dataPackRegistry(Unsyncable.DIRECT_CODEC));
// The overload of #dataPackRegistry that takes a second codec marks the datapack registry as syncable.
syncables.makeRegistry(Syncable.class,
() -> new RegistryBuilder<Syncable>().disableSaving().dataPackRegistry(Syncable.DIRECT_CODEC, Syncable.DIRECT_CODEC));
Copy link
Contributor

Choose a reason for hiding this comment

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

What are the cases that a data pack registry registers a network codec that is different from the main codec?

Copy link
Member

Choose a reason for hiding this comment

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

See vanilla usages. Stuff that is only ever needed/used/processed by the server is just wasted networking data if sent through the main codec over networking.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There also exist codecs that don't work at all when used for networking because they're hardcoded to be used by RegistryOps and the netcode doesn't use RegistryOps.

{
DataProvider.save(gson, cache, json, path);
}
catch(IOException e) // we're inside this ifpresent so the throws can't deal with this
Copy link
Contributor

Choose a reason for hiding this comment

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

Formatting: space after catch keyword.

Suggested change
catch(IOException e) // we're inside this ifpresent so the throws can't deal with this
catch (IOException e) // we're inside this ifpresent so the throws can't deal with this

Also, perhaps you mean as ("as the throws can't deal with this") and not so? Unless I'm interpreting the comment's intention incorrectly, the reason the catch exists is because we cannot propagate the IOException upwards normally because of the use of a lambda, so as would be more semantically correct here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're interpreting it correctly, but I don't think that would be a correct use of "as" either. I'll use "because" for minimum ambiguity -- "The throws can't deal with this exception, because we're inside this ifPresent."

@Commoble
Copy link
Contributor Author

To clarify for me, this means that only the JSON data from datapacks which affect a datapack registry that are present during server startup will affect the contents of the datapack registry, even if said JSON data is removed later on and a /reload is issued?

Yes, after the first load of datapack registries during server startup, the datapack registry jsons are never read again until the server reboots.

I tried to test this PR using the provided test mod, but I got an error while trying to load a singleplayer world in DataPackRegistriesTest.ClientEvents#onClientTagsUpdated because the player is null (and the attempt to read the connection field fails). I've fixed it in my local copy by getting the RegistryAccess from TagsUpdatedEvent#getTagRegistry instead.

I really want to make sure the player's connection's registryaccess gets populated, as that's what clientside mods should be querying during server runtime -- I'll try moving the test to a different event again.

@Commoble Commoble requested a review from sciwhiz12 May 28, 2022 14:35
Copy link
Contributor

@sciwhiz12 sciwhiz12 left a comment

Choose a reason for hiding this comment

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

The test works now without changes. :shipit:


As a side-note, during testing of this PR, I noticed that the /forge tags command doesn't suggest dynamic/datapack registries, but works fine when manually entered.

With help from @Commoble, the cause and solution was found. The cause was because the registry suggestor used Registry.REGISTRY, which only contains the static registries and excludes the dynamic registries. As a fix, one can replace TagsCommand#suggestRegistries with the following snippet:

private static CompletableFuture<Suggestions> suggestRegistries(final CommandContext<CommandSourceStack> ctx,
        final SuggestionsBuilder builder)
{
    ctx.getSource().registryAccess().registries()
            .map(RegistryAccess.RegistryEntry::key)
            .map(ResourceKey::location)
            .map(ResourceLocation::toString)
            .forEach(builder::suggest);
    return builder.buildFuture();
}

This will be fixed soon, either through a PR from me to 1.18 or in the 1.19 branch.

@sciwhiz12 sciwhiz12 added the Assigned This request has been assigned to a core developer. Will not go stale. label May 28, 2022
@TheCurle TheCurle merged commit 997d8e0 into MinecraftForge:1.18.x Jun 5, 2022
tmvkrpxl0 pushed a commit to tmvkrpxl0/MinecraftForge that referenced this pull request Jul 24, 2022
…istryBuilder (MinecraftForge#8630)

Co-authored-by: Silverminer007 <66484505+Silverminer007@users.noreply.github.com>
Co-authored-by: Curle <42079760+TheCurle@users.noreply.github.com>
@Commoble Commoble deleted the 1.18.x-data-pack-registries branch September 17, 2022 14:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.18 Assigned This request has been assigned to a core developer. Will not go stale. Feature This request implements a new feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants