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.4] Refactor registry sync #4233

Draft
wants to merge 2 commits into
base: 1.21.4
Choose a base branch
from

Conversation

modmuss50
Copy link
Member

  • General code cleanup, a lot of stuff can now be made client only as we no longer persist ids.
    • There is scope to move even more (such as the registry remapping code) to the client, but ill leave this for later.
  • Add some more unit tests :)
  • Add optional registries

Optional registries / stricter defaults

The goal of these changes is to document the expected behaviour of missing registry's when a client connects to a modded server. This behaviour is now consistent between a vanilla clients and modded clients. This has nothing to do with individual registry entries (yet).

Some mods have their own registries that only need to be synced when the connecting client has their mod, I have added a new OPTIONAL registry attribute that allows this. This attribute is sent in the registry sync packet.

The behaviour of a connecting client is as follows:

Screenshot 2024-11-15 at 20 25 17

These changes will only apply to 1.21.4 as they are breaking.

TODO

  • Test with real clients and servers
  • Better error message when a registry is missing, I just reused the entries one for now?

@modmuss50 modmuss50 added in progress This issue has PR(s) open to resolve it (or a PR that is WIP) registry-sync Pull requests and issues related to registry sync labels Nov 15, 2024

for (Identifier remoteId : remoteRegistry.keySet()) {
if (!registry.containsId(remoteId)) {
// Found a registry entry from the server that is
Copy link
Contributor

Choose a reason for hiding this comment

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

that is... (missing?)

Comment on lines +96 to +106
// Registry was not found on the client, is it optional?
// If so we can just ignore it.
// Otherwise we throw an exception and disconnect.
if (registry == null) {
if (isRegistryOptional(registryId, data)) {
LOGGER.info("Received registry data for unknown optional registry: {}", registryId);
continue;
} else {
throw new RemapException("Received registry data for unknown registry: " + registryId);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this is duplicative, as checkRemoteRemap should've already thrown in this case?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in progress This issue has PR(s) open to resolve it (or a PR that is WIP) registry-sync Pull requests and issues related to registry sync
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants