Add a tracked data handler registry #3584

haykam821 · 2024-02-11T00:03:57Z

This pull request introduces a new API for registering tracked data handlers, which reduces conflicts between mods registering tracked data handlers in an order that is inconsistent between clients and servers.

To use the API, simply replace usages of the vanilla method:

- TrackedDataHandlerRegistry.register(TRACKED_DATA_HANDLER);
+ FabricTrackedDataRegistry.registerHandler(TRACKED_DATA_HANDLER_ID, TRACKED_DATA_HANDLER);

An example of a mod introducing tracked data handlers is provided within the testmod. This entity uses tracked data handlers for global positions, items, and optional item groups, all of which are not available as one of the vanilla tracked data handlers.

A few remarks regarding the implementation of this API:

Documentation has not been written yet.
Note that the class name is FabricTrackedDataRegistry rather than FabricTrackedDataHandlerRegistry so that tracked data support can be added in the future within the same class; see Hooks to allow registration of custom tracked data fields on entities. #1049 for more information.
Each registration recomputes the map used by vanilla. I'm not sure whether optimizing this behavior is worthwhile; the work required to delay computation until the map is accessed would be more invasive than the current setup.
Mods that do not switch to this API will still work as before. Said mods will not gain the benefits of using this API and will still lack a guarantee of tracked data handlers matching between clients and servers.
There isn't any guard against registering a tracked data handler after mod initialization. Should one be added? Currently, the unspoken idea is that mods will only use this API during mod initialization.
Mods assuming that tracked data handler IDs are stable during mod initialization (or possibly later, per the last remark) will be broken. I assume this breakage is fine, as I doubt many mods interact with tracked data handlers in this way.

Fixes #3482

Fixes FabricMC#3482

...-v1/src/main/java/net/fabricmc/fabric/impl/object/builder/FabricTrackedDataRegistryImpl.java

Linguardium · 2024-02-17T23:21:52Z

would it be beneficial to add a warning if a non-vanilla handler is registered? (outside of the api)

haykam821 · 2024-02-18T00:56:49Z

@Linguardium A warning already will be logged in that scenario.

Linguardium · 2024-02-18T00:59:57Z

Yep, i missed that

modmuss50 · 2024-02-18T14:24:06Z

...rc/main/java/net/fabricmc/fabric/api/object/builder/v1/entity/FabricTrackedDataRegistry.java

+	private FabricTrackedDataRegistry() {
+	}
+
+	public static void registerHandler(Identifier id, TrackedDataHandler<?> handler) {


Needs some docs.

modmuss50 · 2024-02-18T14:24:23Z

...-v1/src/main/java/net/fabricmc/fabric/impl/object/builder/FabricTrackedDataRegistryImpl.java

+import net.fabricmc.fabric.api.event.registry.RegistryIdRemapCallback;
+import net.fabricmc.fabric.mixin.object.builder.TrackedDataHandlerRegistryAccessor;
+
+public class FabricTrackedDataRegistryImpl {


modmuss50 · 2024-02-18T14:30:11Z

...-v1/src/main/java/net/fabricmc/fabric/impl/object/builder/FabricTrackedDataRegistryImpl.java

+		}
+
+		Registry.register(HANDLER_REGISTRY, id, handler);
+		reorderHandlers();


I dont think this needs to happen here, if the above change is made.

The reviews seem to be ordered differently for me; are you talking about changing the event at which reordering occurs? The idea of reordering after registration is so that every tracked data handler registered via Fabric API is immediately available in the map. If that behavior is unnecessary, then this reordering step can be removed.

modmuss50 · 2024-02-18T14:31:20Z

...-v1/src/main/java/net/fabricmc/fabric/impl/object/builder/FabricTrackedDataRegistryImpl.java

+					.attribute(RegistryAttribute.SYNCED)
+					.buildAndRegister();
+
+			RegistryIdRemapCallback.event(HANDLER_REGISTRY).register(state -> reorderHandlers());


Would Client/ServerPlayConnectionEvents.JOIN be a better place to do this? I dont think it would be a bad idea to sort them event when a modded one isnt registered.

If there are no tracked data handlers registered via Fabric API, the sort would have no effect.

modmuss50 · 2024-02-18T14:38:05Z

.../src/main/java/net/fabricmc/fabric/mixin/object/builder/TrackedDataHandlerRegistryMixin.java

+import net.fabricmc.fabric.impl.object.builder.FabricTrackedDataRegistryImpl;
+
+@Mixin(TrackedDataHandlerRegistry.class)
+public class TrackedDataHandlerRegistryMixin {


Would it be worth while injecting into register and logging a warning (or even crashing in dev?) if a handler is registered the vanilla way?

Logging the warning with a stacktrace would be more helpful when trying to track down mods that arent using the API.

modmuss50 · 2024-02-18T14:39:15Z

...uilder-api-v1/src/testmod/java/net/fabricmc/fabric/test/object/builder/TrackStackEntity.java

+import net.minecraft.util.math.GlobalPos;
+import net.minecraft.world.World;
+
+public class TrackStackEntity extends MobEntity {


Might be nice to have a gametest that spawns the entity? Just make sure things dont crash?

I'm not sure how to implement game tests, but using one here seems reasonable. The entity interaction should be simulated as well.

https://github.com/FabricMC/fabric/blob/892bf04af77eebeb135b611785ce6efa97ebd475/fabric-command-api-v2/src/testmod/java/net/fabricmc/fabric/test/command/EntitySelectorGameTest.java might be a good starting point. Happy to guide you through them.

The game tests only run as the server, so they wouldnt be able to check that the data is sycning correctly, but it can at least assume it.

LlamaLad7 · 2024-02-19T13:59:52Z

.../src/main/java/net/fabricmc/fabric/mixin/object/builder/TrackedDataHandlerRegistryMixin.java

+@Mixin(TrackedDataHandlerRegistry.class)
+public class TrackedDataHandlerRegistryMixin {
+	@Inject(method = "<clinit>", at = @At("TAIL"))
+	private static void storeVanillaHandlers(CallbackInfo ci) {


Use a static block over an inject.

Note that TheEndBiomeSourceMixin, RenderLayersMixin, DefaultAttributeRegistryMixin, BlockEntityRendererFactoriesMixin, and EntityRenderersMixin also seem to use a <clinit> inject in a similar way to this mixin; should those be changed as well (at a later time)?

modmuss50 · 2024-04-04T14:03:42Z

@haykam821 are you intrested in finishing this off? I dont think there was anything too crazy left to do on it.

Add a tracked data handler registry

84d919f

Fixes FabricMC#3482

modmuss50 added the enhancement New feature or request label Feb 11, 2024

modmuss50 self-requested a review February 12, 2024 20:25

modmuss50 reviewed Feb 14, 2024

View reviewed changes

...-v1/src/main/java/net/fabricmc/fabric/impl/object/builder/FabricTrackedDataRegistryImpl.java Show resolved Hide resolved

...-v1/src/main/java/net/fabricmc/fabric/impl/object/builder/FabricTrackedDataRegistryImpl.java Show resolved Hide resolved

Sync the tracked data handler registry

03d9015

modmuss50 reviewed Feb 18, 2024

View reviewed changes

LlamaLad7 suggested changes Feb 19, 2024

View reviewed changes

Merge branch '1.20.5' into tracked-data-handler-registry

cad7e71

Sollace mentioned this pull request Apr 26, 2024

Server Log Spamming Sollace/Unicopia#14

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a tracked data handler registry #3584

Add a tracked data handler registry #3584

haykam821 commented Feb 11, 2024 •

edited

Linguardium commented Feb 17, 2024 •

edited

haykam821 commented Feb 18, 2024

Linguardium commented Feb 18, 2024

modmuss50 Feb 18, 2024

modmuss50 Feb 18, 2024

modmuss50 Feb 18, 2024

haykam821 Feb 19, 2024

modmuss50 Feb 18, 2024

haykam821 Feb 19, 2024

modmuss50 Feb 18, 2024

modmuss50 Feb 18, 2024

haykam821 Feb 19, 2024

modmuss50 Feb 19, 2024

LlamaLad7 Feb 19, 2024

haykam821 Feb 19, 2024 •

edited

modmuss50 commented Apr 4, 2024

Add a tracked data handler registry #3584

Are you sure you want to change the base?

Add a tracked data handler registry #3584

Conversation

haykam821 commented Feb 11, 2024 • edited

Linguardium commented Feb 17, 2024 • edited

haykam821 commented Feb 18, 2024

Linguardium commented Feb 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haykam821 Feb 19, 2024 • edited

Choose a reason for hiding this comment

modmuss50 commented Apr 4, 2024

haykam821 commented Feb 11, 2024 •

edited

Linguardium commented Feb 17, 2024 •

edited

haykam821 Feb 19, 2024 •

edited