Fix SMELL-API-01 + log levels : dispatchCodec API hygiene + INFO armature loaded
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().
This commit is contained in:
@@ -184,7 +184,11 @@ public class ArmatureReloadListener extends SimpleJsonResourceReloadListener {
|
|||||||
|
|
||||||
Armature armature = def.toRuntimeArmature();
|
Armature armature = def.toRuntimeArmature();
|
||||||
DATAPACK_ARMATURES.put(id, armature);
|
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/<ns>/tiedup/armatures/, currently 0 in vanilla setups.
|
||||||
|
TiedUpRigConstants.LOGGER.info(
|
||||||
"[ArmatureReloadListener] Registered armature: {} ({} joints)",
|
"[ArmatureReloadListener] Registered armature: {} ({} joints)",
|
||||||
id, armature.getJointNumber()
|
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/<ns>/tiedup/armatures/.
|
||||||
TiedUpRigConstants.LOGGER.info(
|
TiedUpRigConstants.LOGGER.info(
|
||||||
"[ArmatureReloadListener] Reload done : {} armature(s) loaded, {} skipped",
|
"[ArmatureReloadListener] Datapack armature reload : {} custom armature(s) "
|
||||||
|
+ "registered ({} skipped, builtin BIPED active)",
|
||||||
loaded, skipped
|
loaded, skipped
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -56,6 +56,20 @@ public abstract class CodecDispatchRegistry<T extends CodecDispatchRegistry.Type
|
|||||||
|
|
||||||
private final Map<ResourceLocation, Codec<? extends T>> types = new HashMap<>();
|
private final Map<ResourceLocation, Codec<? extends T>> 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).
|
||||||
|
*
|
||||||
|
* <p>Volatile + double-checked-locking pattern : reload listeners and the
|
||||||
|
* server thread can both reach this field in race-y circumstances during
|
||||||
|
* mod init.</p>
|
||||||
|
*/
|
||||||
|
private volatile Codec<T> dispatchCodecCache;
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Subclass-specific human-readable name — used in the duplicate
|
* Subclass-specific human-readable name — used in the duplicate
|
||||||
* registration exception and in the «unknown type» dispatch error so the
|
* registration exception and in the «unknown type» dispatch error so the
|
||||||
@@ -99,15 +113,31 @@ public abstract class CodecDispatchRegistry<T extends CodecDispatchRegistry.Type
|
|||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Build the dispatch codec.
|
* Build (or return the memoized) dispatch codec.
|
||||||
*
|
*
|
||||||
* <p>Uses {@link Codec#partialDispatch} rather than {@link Codec#dispatch}
|
* <p>Uses {@link Codec#partialDispatch} rather than {@link Codec#dispatch}
|
||||||
* so that unknown types surface as a {@link DataResult} error instead of
|
* so that unknown types surface as a {@link DataResult} error instead of
|
||||||
* an unchecked exception — the upstream parser logs and drops the
|
* an unchecked exception — the upstream parser logs and drops the
|
||||||
* offending entry instead of crashing the load.
|
* offending entry instead of crashing the load.</p>
|
||||||
|
*
|
||||||
|
* @apiNote Conventionally invoked exactly once per registry, at the
|
||||||
|
* corresponding interface's {@code CODEC} static field
|
||||||
|
* initialization (e.g. {@code Codec<AnimationAction> 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<T> dispatchCodec() {
|
public final Codec<T> dispatchCodec() {
|
||||||
return ResourceLocation.CODEC.partialDispatch(
|
Codec<T> cached = this.dispatchCodecCache;
|
||||||
|
if (cached != null) {
|
||||||
|
return cached;
|
||||||
|
}
|
||||||
|
synchronized (this) {
|
||||||
|
if (this.dispatchCodecCache == null) {
|
||||||
|
this.dispatchCodecCache = ResourceLocation.CODEC.partialDispatch(
|
||||||
"type",
|
"type",
|
||||||
value -> DataResult.success(value.type()),
|
value -> DataResult.success(value.type()),
|
||||||
id -> {
|
id -> {
|
||||||
@@ -121,13 +151,20 @@ public abstract class CodecDispatchRegistry<T extends CodecDispatchRegistry.Type
|
|||||||
}
|
}
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
return this.dispatchCodecCache;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Test-only state reset — clears the type map. Public so JUnit
|
* Test-only state reset — clears the type map and invalidates the
|
||||||
* fixtures across packages can call it but named to make production usage
|
* memoized dispatch codec so subsequent {@link #dispatchCodec()} calls
|
||||||
* obviously wrong.
|
* 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() {
|
public final void clearForTests() {
|
||||||
this.types.clear();
|
this.types.clear();
|
||||||
|
synchronized (this) {
|
||||||
|
this.dispatchCodecCache = null;
|
||||||
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user