From 7c994a9ffaf030f6cf3d8fe9c6cda7f4b49c04d8 Mon Sep 17 00:00:00 2001 From: notevil Date: Mon, 27 Apr 2026 00:09:19 +0200 Subject: [PATCH] Fix SMELL-LOG-01 : dedup SpawnParticleAction joint-not-found WARN MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reviewer P3 review convergent finding (MEDIUM severity). Period events fire at 20 Hz across the period duration. A typo in 'at: "wrongJoint"' would log WARN every tick × duration — log flood that pollutes latest.log and masks real issues. Fix : ConcurrentHashMap.newKeySet() keyed by armature+joint, log WARN only once per unique miss. Reset hook wired into TiedUpRigRegistryReloadListener.apply() so /reload re-enables warn (in case modder fixes their JSON between reloads). Pattern identical to TiedUpAnimationRegistry.WARNED_MISSING_ANIMS. --- .../rig/TiedUpRigRegistryReloadListener.java | 4 +- .../anim/action/impl/SpawnParticleAction.java | 76 +++++++++++- .../action/impl/SpawnParticleActionTest.java | 114 ++++++++++++++++++ 3 files changed, 189 insertions(+), 5 deletions(-) diff --git a/src/main/java/com/tiedup/remake/rig/TiedUpRigRegistryReloadListener.java b/src/main/java/com/tiedup/remake/rig/TiedUpRigRegistryReloadListener.java index 74fd2b3..00dbd16 100644 --- a/src/main/java/com/tiedup/remake/rig/TiedUpRigRegistryReloadListener.java +++ b/src/main/java/com/tiedup/remake/rig/TiedUpRigRegistryReloadListener.java @@ -14,6 +14,7 @@ import net.minecraftforge.eventbus.api.SubscribeEvent; import net.minecraftforge.fml.common.Mod; import com.tiedup.remake.core.TiedUpMod; +import com.tiedup.remake.rig.anim.action.impl.SpawnParticleAction; /** * Hook {@code /reload} (serveur) + {@code F3+T} (client) pour reset le set dedup @@ -89,8 +90,9 @@ public final class TiedUpRigRegistryReloadListener @Override protected void apply(Void nothing, ResourceManager mgr, ProfilerFiller profiler) { TiedUpAnimationRegistry.resetWarnedMissing(); + SpawnParticleAction.resetWarnedMissing(); TiedUpRigConstants.LOGGER.debug( - "[TiedUpRigRegistryReloadListener] WARNED_MISSING_ANIMS reset on reload" + "[TiedUpRigRegistryReloadListener] WARNED_MISSING_ANIMS + WARNED_MISSING_JOINTS reset on reload" ); } diff --git a/src/main/java/com/tiedup/remake/rig/anim/action/impl/SpawnParticleAction.java b/src/main/java/com/tiedup/remake/rig/anim/action/impl/SpawnParticleAction.java index e18e713..2e91f9b 100644 --- a/src/main/java/com/tiedup/remake/rig/anim/action/impl/SpawnParticleAction.java +++ b/src/main/java/com/tiedup/remake/rig/anim/action/impl/SpawnParticleAction.java @@ -4,6 +4,9 @@ package com.tiedup.remake.rig.anim.action.impl; +import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; + import com.mojang.serialization.Codec; import com.mojang.serialization.codecs.RecordCodecBuilder; @@ -64,6 +67,25 @@ public record SpawnParticleAction( public static final ResourceLocation ID = TiedUpRigConstants.identifier("spawn_particle"); + /** + * Set (thread-safe) des couples {@code armatureName + ":" + joint} pour + * lesquels un WARN « joint inconnu » a déjà été émis. Évite le spam log : + * les period events fire à 20 Hz × duration, donc une typo modder + * ({@code "at": "wrongJoint"}) sans dedup polluerait {@code latest.log} + * d'un WARN par tick et masquerait les vrais bugs. + * + *

Pattern identique à {@code TiedUpAnimationRegistry.WARNED_MISSING_ANIMS} + * — {@link ConcurrentHashMap#newKeySet()} pour la sûreté en cas d'appels + * concurrents (le code execute peut tourner sur le client tick thread comme + * sur un thread de network handler).

+ * + *

Le reset est wired dans + * {@code TiedUpRigRegistryReloadListener.apply()} : à chaque {@code /reload} + * (datapack) ou F3+T (resource pack client) on purge le set, ce qui ré-active + * le WARN pour le cas modder corrige son JSON puis le re-casse.

+ */ + static final Set WARNED_MISSING_JOINTS = ConcurrentHashMap.newKeySet(); + public static final Codec CODEC = RecordCodecBuilder.create(i -> i.group( ResourceLocation.CODEC.fieldOf("particle").forGetter(SpawnParticleAction::particle), Codec.STRING.optionalFieldOf("at", "").forGetter(SpawnParticleAction::joint), @@ -143,10 +165,16 @@ public record SpawnParticleAction( Joint target = armature.searchJointByName(this.joint); if (target == null) { - TiedUpRigConstants.LOGGER.warn( - "SpawnParticleAction : unknown joint '{}' on armature '{}', falling back to entity position", - this.joint, armature - ); + // Dedup : period events fire à 20 Hz, sans dedup une typo modder + // produirait un WARN par tick × duration. Un seul WARN par couple + // (armature, joint) jusqu'au prochain reload. + if (tryWarnMissingJoint(armature.toString(), this.joint)) { + TiedUpRigConstants.LOGGER.warn( + "SpawnParticleAction : unknown joint '{}' on armature '{}', falling back to entity position " + + "(further occurrences for this armature+joint suppressed until next /reload)", + this.joint, armature + ); + } return entityPos; } @@ -155,4 +183,44 @@ public record SpawnParticleAction( // through AnimationAction.execute which today only forwards elapsed. return entityPos; } + + /** + * Helper de dedup pour le WARN « joint inconnu ». Retourne {@code true} si + * le couple {@code (armatureName, joint)} n'avait pas encore été warné — le + * caller doit alors logger ; sinon retourne {@code false} (déjà warné, on + * skip le log). + * + *

Extrait en helper static package-private pour pouvoir tester la + * sémantique de dedup directement (les tests purs ne peuvent pas exercer + * {@link #execute} qui requiert un {@link LivingEntityPatch} mocké et un + * {@link Level} client réels — runtime MC nécessaire). Le test exerce ce + * helper, et l'execute() délègue à ce même helper, donc la couverture de la + * logique critique (dedup) est complète.

+ * + * @param armatureName le nom de l'armature (généralement + * {@code armature.toString()} qui retourne {@code this.name}) + * @param joint le nom du joint introuvable + * @return {@code true} si c'est le premier miss pour ce couple + * (le caller doit logger), {@code false} sinon (déjà warné). + */ + static boolean tryWarnMissingJoint(String armatureName, String joint) { + return WARNED_MISSING_JOINTS.add(armatureName + ":" + joint); + } + + /** + * Reset le set dedup des WARN « joint inconnu ». Appelé par + * {@code TiedUpRigRegistryReloadListener.apply()} à chaque reload datapack / + * resource-pack, et par les tests pour garantir l'isolation entre cas. + * + *

Sans reset après {@code /reload}, un modder qui corrige son JSON puis + * re-casse l'asset ne reverrait jamais le WARN (l'ID resterait dans le set). + * Avec reset, le warn redéclenche après chaque reload — comportement attendu + * pour le feedback modder.

+ * + *

Thread-safe via {@link ConcurrentHashMap#newKeySet()} — pas de + * synchronisation externe nécessaire.

+ */ + public static void resetWarnedMissing() { + WARNED_MISSING_JOINTS.clear(); + } } diff --git a/src/test/java/com/tiedup/remake/rig/anim/action/impl/SpawnParticleActionTest.java b/src/test/java/com/tiedup/remake/rig/anim/action/impl/SpawnParticleActionTest.java index 186f2ff..0b42cc5 100644 --- a/src/test/java/com/tiedup/remake/rig/anim/action/impl/SpawnParticleActionTest.java +++ b/src/test/java/com/tiedup/remake/rig/anim/action/impl/SpawnParticleActionTest.java @@ -15,6 +15,8 @@ import com.mojang.serialization.JsonOps; import net.minecraft.resources.ResourceLocation; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; /** @@ -76,4 +78,116 @@ class SpawnParticleActionTest { void type_isStable() { assertEquals(new ResourceLocation("tiedup", "spawn_particle"), SpawnParticleAction.ID); } + + // ---------------------------------------------------------------------- + // Dedup WARN « joint inconnu » — SMELL-LOG-01 + // + // La logique de dedup est exposée via le helper static package-private + // {@link SpawnParticleAction#tryWarnMissingJoint} qui retourne {@code true} + // uniquement la première fois qu'un couple (armature, joint) est observé. + // Le call site dans {@link SpawnParticleAction#execute} log conditionnellement + // sur ce retour. On teste donc le helper directement — exercer execute() + // nécessiterait un LivingEntityPatch + Level mockés (runtime MC), + // disproportionné pour une logique de Set.add(). + // ---------------------------------------------------------------------- + + /** + * Garantit l'isolation entre tests : un test qui ajoute des entrées dans + * {@link SpawnParticleAction#WARNED_MISSING_JOINTS} pollue les suivants + * sinon (set statique, ordre JUnit non garanti). + */ + @BeforeEach + void clearDedupBefore() { + SpawnParticleAction.resetWarnedMissing(); + } + + @AfterEach + void clearDedupAfter() { + SpawnParticleAction.resetWarnedMissing(); + } + + /** + * Appel répété {@code tryWarnMissingJoint("armX", "wrongJoint")} 5 fois → + * un seul {@code add} retourne {@code true} (le premier), les 4 suivants + * retournent {@code false}. C'est exactement le contrat dedup attendu : un + * seul WARN logué par couple unique sur toute la durée de vie du process + * (jusqu'au prochain reload). + * + *

Sans dedup, un period event qui fire à 20 Hz × 2s duration loggerait + * 40 WARN identiques — le scénario réel décrit dans SMELL-LOG-01.

+ */ + @Test + void executeWithUnknownJoint_logsWarnOnce() { + assertTrue(SpawnParticleAction.tryWarnMissingJoint("biped_armature", "wrongJoint"), + "Premier miss → tryWarnMissingJoint doit retourner true (caller log)."); + + // Simulation 4 ticks suivants du même period event — même couple, + // donc tous les appels suivants doivent retourner false (déjà warné). + for (int tick = 1; tick <= 4; tick++) { + assertFalse(SpawnParticleAction.tryWarnMissingJoint("biped_armature", "wrongJoint"), + "Tick #" + tick + " avec même (armature, joint) → tryWarnMissingJoint doit retourner false (skip log)."); + } + } + + /** + * Couples distincts → chaque couple log indépendamment. C'est nécessaire + * pour que le modder voie chaque typo distincte (sinon une seule typo + * masquerait toutes les autres). + * + *

Cas couvers :

+ *
    + *
  • Même armature, joints différents → 2 WARN distincts
    • + *
  • Armatures différentes, même joint name → 2 WARN distincts (le + * même nom de joint peut être valide sur une armature et invalide + * sur une autre — le scope par armature est essentiel).
    • + *
+ */ + @Test + void executeWithDifferentUnknownJoints_logsEach() { + // Cas 1 : même armature, joints distincts. + assertTrue(SpawnParticleAction.tryWarnMissingJoint("biped_armature", "jointA"), + "Premier miss (biped_armature, jointA) → log."); + assertTrue(SpawnParticleAction.tryWarnMissingJoint("biped_armature", "jointB"), + "Premier miss (biped_armature, jointB) — joint distinct → doit aussi log."); + + // Cas 2 : armatures distinctes, même nom de joint. Important : + // "Head" peut être valide sur biped_armature et invalide sur + // custom_armature. Le scope par armature évite les faux positifs. + assertTrue(SpawnParticleAction.tryWarnMissingJoint("custom_armature", "jointA"), + "Premier miss (custom_armature, jointA) — armature distincte → doit aussi log."); + + // Vérification dedup intra-couple : re-appel de chacun retourne false. + assertFalse(SpawnParticleAction.tryWarnMissingJoint("biped_armature", "jointA"), + "Re-appel (biped_armature, jointA) → déjà warné → false."); + assertFalse(SpawnParticleAction.tryWarnMissingJoint("biped_armature", "jointB"), + "Re-appel (biped_armature, jointB) → déjà warné → false."); + assertFalse(SpawnParticleAction.tryWarnMissingJoint("custom_armature", "jointA"), + "Re-appel (custom_armature, jointA) → déjà warné → false."); + } + + /** + * Après {@link SpawnParticleAction#resetWarnedMissing()}, un couple + * précédemment warné doit re-warn s'il rencontre à nouveau un miss. C'est + * le contrat avec le {@code TiedUpRigRegistryReloadListener.apply()} — + * scénario : modder corrige son JSON, {@code /reload}, recasse, le WARN + * doit redéclencher pour signaler la régression. + */ + @Test + void resetWarnedMissing_allowsRewarn() { + // Premier miss → log. + assertTrue(SpawnParticleAction.tryWarnMissingJoint("biped_armature", "wrongJoint"), + "Premier miss → log."); + + // Dedup actif. + assertFalse(SpawnParticleAction.tryWarnMissingJoint("biped_armature", "wrongJoint"), + "Avant reset, re-miss → silent."); + + // Simule un /reload (datapack ou F3+T) : reset du set dedup. + SpawnParticleAction.resetWarnedMissing(); + + // Post-reset, le même couple doit re-warn. + assertTrue(SpawnParticleAction.tryWarnMissingJoint("biped_armature", "wrongJoint"), + "Après resetWarnedMissing(), même couple → tryWarnMissingJoint doit re-retourner true. " + + "C'est le comportement attendu pour le feedback modder après /reload."); + } }