-
Notifications
You must be signed in to change notification settings - Fork 399
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
Add a tracked data handler registry #3584
base: 1.20.5
Are you sure you want to change the base?
Add a tracked data handler registry #3584
Conversation
...-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
would it be beneficial to add a warning if a non-vanilla handler is registered? (outside of the api) |
@Linguardium A warning already will be logged in that scenario. |
Yep, i missed that |
private FabricTrackedDataRegistry() { | ||
} | ||
|
||
public static void registerHandler(Identifier id, TrackedDataHandler<?> handler) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs some docs.
import net.fabricmc.fabric.api.event.registry.RegistryIdRemapCallback; | ||
import net.fabricmc.fabric.mixin.object.builder.TrackedDataHandlerRegistryAccessor; | ||
|
||
public class FabricTrackedDataRegistryImpl { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: final
} | ||
|
||
Registry.register(HANDLER_REGISTRY, id, handler); | ||
reorderHandlers(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont think this needs to happen here, if the above change is made.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
.attribute(RegistryAttribute.SYNCED) | ||
.buildAndRegister(); | ||
|
||
RegistryIdRemapCallback.event(HANDLER_REGISTRY).register(state -> reorderHandlers()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there are no tracked data handlers registered via Fabric API, the sort would have no effect.
import net.fabricmc.fabric.impl.object.builder.FabricTrackedDataRegistryImpl; | ||
|
||
@Mixin(TrackedDataHandlerRegistry.class) | ||
public class TrackedDataHandlerRegistryMixin { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
import net.minecraft.util.math.GlobalPos; | ||
import net.minecraft.world.World; | ||
|
||
public class TrackStackEntity extends MobEntity { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be nice to have a gametest that spawns the entity? Just make sure things dont crash?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how to implement game tests, but using one here seems reasonable. The entity interaction should be simulated as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@Mixin(TrackedDataHandlerRegistry.class) | ||
public class TrackedDataHandlerRegistryMixin { | ||
@Inject(method = "<clinit>", at = @At("TAIL")) | ||
private static void storeVanillaHandlers(CallbackInfo ci) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use a static block over an inject.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)?
@haykam821 are you intrested in finishing this off? I dont think there was anything too crazy left to do on it. |
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:
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:
FabricTrackedDataRegistry
rather thanFabricTrackedDataHandlerRegistry
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.Fixes #3482