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

Registry sync v2 - API #167

Open
wants to merge 31 commits into
base: feature/registry-sync-v2/main
Choose a base branch
from

Conversation

thecatcore
Copy link
Member

New registry api, more extensible, only handles registry infrastructure (remapping included).

Individual registries for content are moved to their own module in other pr depending on this one.

build.gradle Outdated Show resolved Hide resolved
@thecatcore thecatcore self-assigned this May 9, 2024
@thecatcore thecatcore added enhancement New feature or request help wanted Extra attention is needed new module New module bikeshed yes draft labels May 9, 2024
@thecatcore thecatcore marked this pull request as ready for review August 29, 2024 20:22
@thecatcore thecatcore removed the draft label Aug 29, 2024
@thecatcore thecatcore added this to the 1.11.0 milestone Aug 29, 2024

@Override
public PacketByteBuf writeCompound(NbtCompound compound) {
return this.writeNbtCompound(compound);
public PacketByteBuf writeCompound(NbtCompound tag) {
Copy link
Contributor

Choose a reason for hiding this comment

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

tag -> nbt / compound

@@ -71,6 +71,10 @@ public String toString() {
return this.namespace + ':' + this.path;
}

public String toTranslationKey() {
return this.namespace + "." + this.path;
Copy link
Contributor

Choose a reason for hiding this comment

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

unused - any reason why we need this? items, blocks and such have their own format for translation keys

Copy link
Member Author

Choose a reason for hiding this comment

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

It's used later in the registry implementation to set the translation key automatically, which was something already done in registry sync v1 previously.

public PacketByteBuf writeCompound(NbtCompound compound) {
this.writeNbtCompound(compound);
public PacketByteBuf writeCompound(NbtCompound tag) {
this.writeNbtCompound(tag);
Copy link
Contributor

Choose a reason for hiding this comment

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

tag -> nbt / compound

public PacketByteBuf writeCompound(NbtCompound compound) {
this.writeNbtCompound(compound);
public PacketByteBuf writeCompound(NbtCompound tag) {
this.writeNbtCompound(tag);
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

@@ -20,6 +20,7 @@
import net.legacyfabric.fabric.api.util.Identifier;
import net.legacyfabric.fabric.api.util.SinceMC;

@Deprecated
Copy link
Contributor

Choose a reason for hiding this comment

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

could add a javadoc comment here, stating what should be used instead

@@ -20,6 +20,7 @@
import net.legacyfabric.fabric.api.util.Identifier;
import net.legacyfabric.fabric.api.util.SinceMC;

@Deprecated
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto, same for the rest of the v1 api

import net.legacyfabric.fabric.api.registry.v2.event.RegistryEntryAddedCallback;
import net.legacyfabric.fabric.api.util.Identifier;

public interface Registry<T> extends Iterable<T> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Registry -> RegistryExtensions? I don't think its a good idea to keep it as the same name as an existing minecraft class

Copy link
Member Author

Choose a reason for hiding this comment

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

RegistryExtensions doesn't fit in this case as this interface can be used to implement a custom registry implementation.
However maybe we should prefix those classes with either Fabric, LF or LegacyFabric to differentiate them from vanilla classes as you've said.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bikeshed yes enhancement New feature or request help wanted Extra attention is needed new module New module
Projects
Status: In Review
Development

Successfully merging this pull request may close these issues.

2 participants