From 23b249dcd22c8baa8a22ad30af031dfeffeb9efd Mon Sep 17 00:00:00 2001 From: notevil Date: Mon, 27 Apr 2026 00:41:28 +0200 Subject: [PATCH] Fix SMELL-API-01 + log levels : dispatchCodec API hygiene + INFO armature loaded MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reviewer P3 review convergent findings (LOW severity). SMELL-API-01 : CodecDispatchRegistry.dispatchCodec() is public (legitimate for cross-package interface CODEC fields) but consumers might inadvertently call it twice and reason about identity. Added @apiNote stating canonical single-call usage + memoization to guarantee idempotent identity. ArmatureReloadListener log level : promoted 'Registered armature: X' from DEBUG to INFO. Datapack-loaded custom armatures are visible at default log level — gameday smoke test for D6 wiring. Added summary line 'Datapack armature reload : N custom armatures registered (builtin BIPED active)' at end of apply(). --- .../datapack/ArmatureReloadListener.java | 13 +++- .../rig/util/CodecDispatchRegistry.java | 71 ++++++++++++++----- 2 files changed, 65 insertions(+), 19 deletions(-) diff --git a/src/main/java/com/tiedup/remake/rig/armature/datapack/ArmatureReloadListener.java b/src/main/java/com/tiedup/remake/rig/armature/datapack/ArmatureReloadListener.java index fe049b9..0028dcc 100644 --- a/src/main/java/com/tiedup/remake/rig/armature/datapack/ArmatureReloadListener.java +++ b/src/main/java/com/tiedup/remake/rig/armature/datapack/ArmatureReloadListener.java @@ -184,7 +184,11 @@ public class ArmatureReloadListener extends SimpleJsonResourceReloadListener { Armature armature = def.toRuntimeArmature(); DATAPACK_ARMATURES.put(id, armature); - TiedUpRigConstants.LOGGER.debug( + // INFO (was DEBUG) — datapack-loaded custom armatures are visible + // at the default log level, providing a gameday smoke test for + // D6 wiring. Volume is bounded by the number of files in + // data//tiedup/armatures/, currently 0 in vanilla setups. + TiedUpRigConstants.LOGGER.info( "[ArmatureReloadListener] Registered armature: {} ({} joints)", id, armature.getJointNumber() ); @@ -198,8 +202,13 @@ public class ArmatureReloadListener extends SimpleJsonResourceReloadListener { } } + // Summary line — emitted unconditionally to confirm the listener ran. + // The builtin BIPED is registered separately in TiedUpArmatures (Java), + // not here ; this counter only reflects datapack JSONs from + // data//tiedup/armatures/. TiedUpRigConstants.LOGGER.info( - "[ArmatureReloadListener] Reload done : {} armature(s) loaded, {} skipped", + "[ArmatureReloadListener] Datapack armature reload : {} custom armature(s) " + + "registered ({} skipped, builtin BIPED active)", loaded, skipped ); } diff --git a/src/main/java/com/tiedup/remake/rig/util/CodecDispatchRegistry.java b/src/main/java/com/tiedup/remake/rig/util/CodecDispatchRegistry.java index a0666d2..970f855 100644 --- a/src/main/java/com/tiedup/remake/rig/util/CodecDispatchRegistry.java +++ b/src/main/java/com/tiedup/remake/rig/util/CodecDispatchRegistry.java @@ -56,6 +56,20 @@ public abstract class CodecDispatchRegistry> types = new HashMap<>(); + /** + * Memoized dispatch codec — built lazily on first {@link #dispatchCodec()} + * call and reused thereafter. Guarantees {@code identity} stability for + * consumers that compare codec references and avoids allocating a new + * {@code partialDispatch} wrapper on every call. The wrapped codec defers + * map lookups to parse time, so caching it before all registrations have + * run is safe (see init-order contract in the class javadoc). + * + *

Volatile + double-checked-locking pattern : reload listeners and the + * server thread can both reach this field in race-y circumstances during + * mod init.

+ */ + private volatile Codec dispatchCodecCache; + /** * Subclass-specific human-readable name — used in the duplicate * registration exception and in the «unknown type» dispatch error so the @@ -99,35 +113,58 @@ public abstract class CodecDispatchRegistryUses {@link Codec#partialDispatch} rather than {@link Codec#dispatch} * so that unknown types surface as a {@link DataResult} error instead of * an unchecked exception — the upstream parser logs and drops the - * offending entry instead of crashing the load. + * offending entry instead of crashing the load.

+ * + * @apiNote Conventionally invoked exactly once per registry, at the + * corresponding interface's {@code CODEC} static field + * initialization (e.g. {@code Codec CODEC = + * AnimationActionRegistry.INSTANCE.dispatchCodec();}). The + * returned codec is memoized so repeated calls return the same + * instance — consumers should still prefer the canonical + * {@code Interface.CODEC} field for clarity rather than calling + * this method ad hoc, since reasoning about codec identity in + * serialization frameworks (DFU, Mojang Codec) is brittle. */ public final Codec dispatchCodec() { - return ResourceLocation.CODEC.partialDispatch( - "type", - value -> DataResult.success(value.type()), - id -> { - Codec codec = this.types.get(id); - if (codec == null) { - return DataResult.error( - () -> "Unknown " + this.registryName() + " type: " + id - ); - } - return DataResult.success(codec); + Codec cached = this.dispatchCodecCache; + if (cached != null) { + return cached; + } + synchronized (this) { + if (this.dispatchCodecCache == null) { + this.dispatchCodecCache = ResourceLocation.CODEC.partialDispatch( + "type", + value -> DataResult.success(value.type()), + id -> { + Codec codec = this.types.get(id); + if (codec == null) { + return DataResult.error( + () -> "Unknown " + this.registryName() + " type: " + id + ); + } + return DataResult.success(codec); + } + ); } - ); + return this.dispatchCodecCache; + } } /** - * Test-only state reset — clears the type map. Public so JUnit - * fixtures across packages can call it but named to make production usage - * obviously wrong. + * Test-only state reset — clears the type map and invalidates the + * memoized dispatch codec so subsequent {@link #dispatchCodec()} calls + * rebuild against the post-reset state. Public so JUnit fixtures across + * packages can call it but named to make production usage obviously wrong. */ public final void clearForTests() { this.types.clear(); + synchronized (this) { + this.dispatchCodecCache = null; + } } }