Fix SMELL-LOG-01 : dedup SpawnParticleAction joint-not-found WARN
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.
This commit is contained in:
notevil
@@ -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"
|
||||
);
|
||||
}
|
||||
|
||||
|
||||
@@ -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.
|
||||
*
|
||||
* <p>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).</p>
|
||||
*
|
||||
* <p>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.</p>
|
||||
*/
|
||||
static final Set<String> WARNED_MISSING_JOINTS = ConcurrentHashMap.newKeySet();
|
||||
|
||||
public static final Codec<SpawnParticleAction> 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).
|
||||
*
|
||||
* <p>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.</p>
|
||||
*
|
||||
* @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.
|
||||
*
|
||||
* <p>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.</p>
|
||||
*
|
||||
* <p>Thread-safe via {@link ConcurrentHashMap#newKeySet()} — pas de
|
||||
* synchronisation externe nécessaire.</p>
|
||||
*/
|
||||
public static void resetWarnedMissing() {
|
||||
WARNED_MISSING_JOINTS.clear();
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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).
|
||||
*
|
||||
* <p>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.</p>
|
||||
*/
|
||||
@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).
|
||||
*
|
||||
* <p>Cas couvers :</p>
|
||||
* <ul>
|
||||
* <li>Même armature, joints différents → 2 WARN distincts</li>
|
||||
* <li>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).</li>
|
||||
* </ul>
|
||||
*/
|
||||
@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.");
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user