P3-12 review fixes : reorder preconditions + document 400ms decision
HIGH BUG-001 + MEDIUM RISK-001 : spam timestamp was burned BEFORE preconditions were validated. Null registryName meant server played but clients got no broadcast (state desync). Null animator meant 400ms lockout for an entity that never played. Reorder : validate registryName + animator FIRST, record spam only after preconditions pass, then server play + broadcast. MEDIUM RISK-002 : document 400ms global guard as INTENTIONAL design for cinematic one-shots. Edge case hit+fall-damage same-tick drops the 2nd — acceptable trade-off. Per-category rate limiting is Phase 4 if gameplay needs it. LOW SMELL-001 : merge duplicate javadoc on LAST_SYNC_PLAY_BY_ENTITY.
This commit is contained in:
notevil
@@ -165,12 +165,11 @@ public abstract class LivingEntityPatch<T extends LivingEntity> extends EntityPa
|
||||
* handler appelle {@code playAnimationSync} hors tick. Les
|
||||
* {@code get}/{@code put} individuels sont atomic via le wrapper ; pas
|
||||
* d'itération concurrente ici donc pas de sync externe nécessaire.</p>
|
||||
*/
|
||||
/**
|
||||
* Key-type = {@link Object} (pas {@link LivingEntity}) pour permettre le
|
||||
* test en isolation sans bootstrap MC. En prod la key effective est
|
||||
* toujours une {@link LivingEntity} ; {@link WeakHashMap} tolère
|
||||
* n'importe quel Object comme clé et GC-cleanup reste valide.
|
||||
*
|
||||
* <p><b>Key-type = {@link Object}</b> (pas {@link LivingEntity}) pour
|
||||
* permettre le test en isolation sans bootstrap MC. En prod la key
|
||||
* effective est toujours une {@link LivingEntity} ; {@link WeakHashMap}
|
||||
* tolère n'importe quel Object comme clé et GC-cleanup reste valide.</p>
|
||||
*/
|
||||
static final Map<Object, Long> LAST_SYNC_PLAY_BY_ENTITY =
|
||||
Collections.synchronizedMap(new WeakHashMap<>());
|
||||
@@ -218,20 +217,53 @@ public abstract class LivingEntityPatch<T extends LivingEntity> extends EntityPa
|
||||
* les animations ambient / motion-driven, préférer {@code addLivingAnimation}
|
||||
* sur l'animator via le pipeline d'équipement data-driven.</p>
|
||||
*
|
||||
* <p><b>Anti-spam guard</b> : les appels consécutifs sur la même entity
|
||||
* dans une fenêtre courte (400 ms) sont dropped silencieusement. Prévient
|
||||
* l'abus master-click-spam (hit/slap) qui flood-erait les packets.</p>
|
||||
* <h3>Ordre des preconditions (P3-12 review fix)</h3>
|
||||
* <p>L'ordre des vérifications est load-bearing pour éviter deux classes
|
||||
* de bugs :</p>
|
||||
* <ol>
|
||||
* <li><b>Null accessor / registryName</b> : validé AVANT le server play.
|
||||
* Sinon, un accessor qui retourne {@code null} en
|
||||
* {@code registryName()} (cas {@link com.tiedup.remake.rig.anim.types.LinkAnimation}
|
||||
* ou {@link com.tiedup.remake.rig.anim.types.LayerOffAnimation}) jouerait
|
||||
* côté serveur mais les clients ne recevraient rien → state desync.</li>
|
||||
* <li><b>Animator null</b> : validé AVANT {@code checkAndRecordSpam}.
|
||||
* Sinon, un appel durant la fenêtre {@code onConstructed} (pre-postInit
|
||||
* race) burnerait le timestamp spam-guard pour 400ms alors que
|
||||
* l'animation n'a jamais été jouée — un call légitime suivant
|
||||
* serait drop silencieusement.</li>
|
||||
* <li><b>Anti-spam</b> : le timestamp n'est enregistré qu'APRÈS les deux
|
||||
* preconditions, pour éviter un lockout sur un call qui n'a rien fait.</li>
|
||||
* </ol>
|
||||
*
|
||||
* <p><b>Server-side play</b> : l'animator côté server est un
|
||||
* {@code ServerAnimator} — il ne rend rien visuellement mais track l'état
|
||||
* d'animation pour {@code StateSpectrum} / {@code EntityState} (utilisé par
|
||||
* la logique AI, collision, etc.). Donc on joue localement aussi, pas
|
||||
* seulement broadcast.</p>
|
||||
* <h3>Server-side play</h3>
|
||||
* <p>L'animator côté server est un {@code ServerAnimator} — il ne rend rien
|
||||
* visuellement mais track l'état d'animation pour {@code StateSpectrum} /
|
||||
* {@code EntityState} (utilisé par la logique AI, collision, etc.). Donc
|
||||
* on joue localement aussi, pas seulement broadcast.</p>
|
||||
*
|
||||
* <p><b>Priority non-forwardée</b> : le paramètre {@code priority} est
|
||||
* encodé dans le packet pour future use / logging client-side, mais
|
||||
* {@link Animator#playAnimation} ne l'accepte pas — la priority effective
|
||||
* vient de {@link StaticAnimation#getPriority()} (déclarée en JSON via
|
||||
* <h3>Anti-spam design (400ms global)</h3>
|
||||
* <p>Un guard global de 400 ms par entity drop les calls {@code playAnimationSync}
|
||||
* suivants. C'est <b>INTENTIONAL</b> pour les cinematic one-shots
|
||||
* (capture grab, hurt, death) — prévient la cacophonie visuelle quand
|
||||
* plusieurs events serveur fire dans la même fenêtre (ex: master-click-spam).</p>
|
||||
*
|
||||
* <p><b>Edge case acceptée</b> : hit + fall-damage dans le même tick drop
|
||||
* le 2e call silencieusement. Le server-authored gagne par order of call ;
|
||||
* la perte visuelle est préférable à l'override brutal de la 1re anim.</p>
|
||||
*
|
||||
* <p><b>Pour la locomotion</b> (walk / idle / struggle / sneak), utiliser
|
||||
* le pipeline {@code addLivingAnimation} map (pas de rate limit) — il est
|
||||
* driven par {@code updateMotion} tick-by-tick sans broadcast discret.</p>
|
||||
*
|
||||
* <p><b>Phase 4</b> : si le gameplay réclame des buckets par catégorie
|
||||
* (cinematic vs reaction vs env-feedback), introduire un paramètre
|
||||
* {@code category} + maps séparées. Scope creep Phase 3.</p>
|
||||
*
|
||||
* <h3>Priority non-forwardée</h3>
|
||||
* <p>Le paramètre {@code priority} est encodé dans le packet pour future
|
||||
* use / logging client-side, mais {@link Animator#playAnimation} ne
|
||||
* l'accepte pas — la priority effective vient de
|
||||
* {@link StaticAnimation#getPriority()} (déclarée en JSON via
|
||||
* {@code ClientAnimationProperties.PRIORITY}). Cf. design §12.</p>
|
||||
*
|
||||
* @param accessor l'animation à jouer (typiquement résolue via
|
||||
@@ -252,34 +284,29 @@ public abstract class LivingEntityPatch<T extends LivingEntity> extends EntityPa
|
||||
return;
|
||||
}
|
||||
|
||||
// Anti-spam guard : check + record en une étape atomique (cf. helper
|
||||
// 1+2+3. Preconditions : accessor / registryName / animator.
|
||||
// Validées AVANT checkAndRecordSpam pour éviter de burn le timestamp
|
||||
// sur un call filtré (cf. BUG-001 / RISK-001).
|
||||
ResourceLocation animId = resolveBroadcastableAnimId(
|
||||
accessor, this.animator, this.original.getId()
|
||||
);
|
||||
if (animId == null) {
|
||||
return; // log déjà émis par resolveBroadcastableAnimId
|
||||
}
|
||||
|
||||
// 4. Anti-spam guard : check + record en une étape atomique (cf. helper
|
||||
// package-private pour tests unitaires sans LivingEntity réel).
|
||||
// Record APRÈS les preconditions pour qu'un call filtré n'empêche pas
|
||||
// un call légitime suivant dans la fenêtre 400ms.
|
||||
if (!checkAndRecordSpam(this.original, System.currentTimeMillis())) {
|
||||
return;
|
||||
}
|
||||
|
||||
// Server-side : play locally (ServerAnimator track l'état pour
|
||||
// 5. Server-side : play locally (ServerAnimator track l'état pour
|
||||
// StateSpectrum/EntityState même sans rendu visuel serveur).
|
||||
if (this.animator != null) {
|
||||
this.animator.playAnimation(accessor, transitionTime);
|
||||
}
|
||||
|
||||
// Broadcast aux clients trackers.
|
||||
if (accessor == null) {
|
||||
TiedUpRigConstants.LOGGER.warn(
|
||||
"[LivingEntityPatch.playAnimationSync] null accessor, cannot broadcast"
|
||||
);
|
||||
return;
|
||||
}
|
||||
ResourceLocation animId = accessor.registryName();
|
||||
if (animId == null) {
|
||||
TiedUpRigConstants.LOGGER.warn(
|
||||
"[LivingEntityPatch.playAnimationSync] accessor has null registryName, cannot broadcast: {}",
|
||||
accessor
|
||||
);
|
||||
return;
|
||||
}
|
||||
this.animator.playAnimation(accessor, transitionTime);
|
||||
|
||||
// 6. Broadcast aux clients trackers.
|
||||
PacketPlayRigAnim packet = PacketPlayRigAnim.of(
|
||||
this.original.getId(),
|
||||
animId,
|
||||
@@ -292,4 +319,61 @@ public abstract class LivingEntityPatch<T extends LivingEntity> extends EntityPa
|
||||
packet
|
||||
);
|
||||
}
|
||||
|
||||
/**
|
||||
* Précondition-check pure pour {@link #playAnimationSync} : retourne la
|
||||
* {@link ResourceLocation} broadcastable si tout est OK, {@code null} sinon
|
||||
* (avec un log warn/debug approprié).
|
||||
*
|
||||
* <p>Package-private pour tests unitaires sans bootstrap MC (on peut passer
|
||||
* accessor/animator null ou un stub lambda ; l'entityId est juste un int
|
||||
* pour le logging).</p>
|
||||
*
|
||||
* <p>Side-effect-free pour la spam map : ce helper ne touche pas
|
||||
* {@link #LAST_SYNC_PLAY_BY_ENTITY}. C'est exactement le point du fix
|
||||
* BUG-001 / RISK-001 — les preconditions sont validées AVANT d'enregistrer
|
||||
* le timestamp anti-spam.</p>
|
||||
*
|
||||
* @param accessor l'accessor à valider (null → skip)
|
||||
* @param animator l'animator à valider (null → skip)
|
||||
* @param entityIdForLog entity id utilisé dans le message de log
|
||||
* @return la registryName broadcastable, ou null si une precondition fail
|
||||
*/
|
||||
@Nullable
|
||||
static ResourceLocation resolveBroadcastableAnimId(
|
||||
@Nullable AnimationAccessor<? extends StaticAnimation> accessor,
|
||||
@Nullable Animator animator,
|
||||
int entityIdForLog
|
||||
) {
|
||||
if (accessor == null) {
|
||||
TiedUpRigConstants.LOGGER.warn(
|
||||
"[LivingEntityPatch.playAnimationSync] null accessor for entity={}, skip "
|
||||
+ "(do NOT burn spam timestamp, do NOT server-play).",
|
||||
entityIdForLog
|
||||
);
|
||||
return null;
|
||||
}
|
||||
|
||||
ResourceLocation animId = accessor.registryName();
|
||||
if (animId == null) {
|
||||
TiedUpRigConstants.LOGGER.warn(
|
||||
"[LivingEntityPatch.playAnimationSync] accessor has null registryName, "
|
||||
+ "cannot broadcast to trackers (entity={}, class={}). "
|
||||
+ "Skipping server play too to preserve client-server consistency.",
|
||||
entityIdForLog, accessor.getClass().getSimpleName()
|
||||
);
|
||||
return null;
|
||||
}
|
||||
|
||||
if (animator == null) {
|
||||
TiedUpRigConstants.LOGGER.debug(
|
||||
"[LivingEntityPatch.playAnimationSync] animator null (pre-postInit race?), "
|
||||
+ "skipping for entity={}",
|
||||
entityIdForLog
|
||||
);
|
||||
return null;
|
||||
}
|
||||
|
||||
return animId;
|
||||
}
|
||||
}
|
||||
|
||||
@@ -5,11 +5,21 @@
|
||||
package com.tiedup.remake.rig.patch;
|
||||
|
||||
import static org.junit.jupiter.api.Assertions.assertFalse;
|
||||
import static org.junit.jupiter.api.Assertions.assertNotNull;
|
||||
import static org.junit.jupiter.api.Assertions.assertNull;
|
||||
import static org.junit.jupiter.api.Assertions.assertTrue;
|
||||
import static org.mockito.Mockito.mock;
|
||||
import static org.mockito.Mockito.when;
|
||||
|
||||
import org.junit.jupiter.api.BeforeEach;
|
||||
import org.junit.jupiter.api.Test;
|
||||
|
||||
import net.minecraft.resources.ResourceLocation;
|
||||
|
||||
import com.tiedup.remake.rig.anim.AnimationManager.AnimationAccessor;
|
||||
import com.tiedup.remake.rig.anim.Animator;
|
||||
import com.tiedup.remake.rig.anim.types.StaticAnimation;
|
||||
|
||||
/**
|
||||
* Tests du helper {@link LivingEntityPatch#checkAndRecordSpam} — anti-spam
|
||||
* guard de {@code playAnimationSync}. Logique pure, injectable clock (paramètre
|
||||
@@ -138,4 +148,113 @@ class LivingEntityPatchSpamGuardTest {
|
||||
"SPAM_GUARD_MS doit rester 400ms (tuning master-click-spam). "
|
||||
+ "Changer cette valeur impacte le feeling game — à reviewer.");
|
||||
}
|
||||
|
||||
// -----------------------------------------------------------------------
|
||||
// P3-12 review fix : preconditions AVANT spam timestamp (BUG-001 + RISK-001)
|
||||
// -----------------------------------------------------------------------
|
||||
//
|
||||
// Ces tests valident que resolveBroadcastableAnimId() — le helper précondition
|
||||
// extrait pour testabilité — retourne null SANS burn le spam map quand
|
||||
// accessor/registryName/animator est invalide.
|
||||
//
|
||||
// Rationale (cf. commit P3-12 review fixes) :
|
||||
// - Si le code burnait le timestamp AVANT les preconditions, un appel
|
||||
// filtré (null accessor/animator) lockerait l'entity pour 400ms,
|
||||
// empêchant un call légitime suivant.
|
||||
// - Side-effect-freeness de resolveBroadcastableAnimId est la garantie
|
||||
// que l'ordre actuel dans playAnimationSync est correct.
|
||||
|
||||
/**
|
||||
* Accessor null → resolveBroadcastableAnimId retourne null, spam map
|
||||
* reste vide (pas de timestamp burn).
|
||||
*/
|
||||
@Test
|
||||
void resolveBroadcastableAnimId_nullAccessor_noSpamTimestamp() {
|
||||
Animator animator = mock(Animator.class);
|
||||
|
||||
ResourceLocation result = LivingEntityPatch.resolveBroadcastableAnimId(
|
||||
null, animator, /* entityIdForLog= */ 42
|
||||
);
|
||||
|
||||
assertNull(result, "Null accessor → null return (no broadcast possible).");
|
||||
assertTrue(LivingEntityPatch.LAST_SYNC_PLAY_BY_ENTITY.isEmpty(),
|
||||
"Precondition failure must NOT burn the spam map — sinon un call "
|
||||
+ "filtré lockerait l'entity pour 400ms (BUG-001 regression).");
|
||||
}
|
||||
|
||||
/**
|
||||
* Accessor non-null mais {@code registryName()} returns null (cas
|
||||
* LinkAnimation / LayerOffAnimation) → skip complet. Sans ce guard,
|
||||
* le server play ferait feu mais les clients ne recevraient rien →
|
||||
* state desync (RISK-001).
|
||||
*/
|
||||
@Test
|
||||
@SuppressWarnings("unchecked")
|
||||
void resolveBroadcastableAnimId_nullRegistryName_noSpamTimestamp() {
|
||||
AnimationAccessor<StaticAnimation> accessor = mock(AnimationAccessor.class);
|
||||
when(accessor.registryName()).thenReturn(null);
|
||||
Animator animator = mock(Animator.class);
|
||||
|
||||
ResourceLocation result = LivingEntityPatch.resolveBroadcastableAnimId(
|
||||
accessor, animator, /* entityIdForLog= */ 42
|
||||
);
|
||||
|
||||
assertNull(result,
|
||||
"Null registryName → null return (preserve client-server consistency).");
|
||||
assertTrue(LivingEntityPatch.LAST_SYNC_PLAY_BY_ENTITY.isEmpty(),
|
||||
"Precondition failure (null registryName) must NOT burn spam map.");
|
||||
}
|
||||
|
||||
/**
|
||||
* Accessor avec registryName valide mais animator null → skip
|
||||
* (pre-postInit race defensive guard, BUG-001). Sans ce reorder,
|
||||
* le timestamp serait enregistré alors que rien ne s'est joué.
|
||||
*/
|
||||
@Test
|
||||
@SuppressWarnings("unchecked")
|
||||
void resolveBroadcastableAnimId_nullAnimator_noSpamTimestamp() {
|
||||
AnimationAccessor<StaticAnimation> accessor = mock(AnimationAccessor.class);
|
||||
when(accessor.registryName()).thenReturn(
|
||||
ResourceLocation.fromNamespaceAndPath("tiedup", "context_stand_idle")
|
||||
);
|
||||
|
||||
ResourceLocation result = LivingEntityPatch.resolveBroadcastableAnimId(
|
||||
accessor, /* animator= */ null, /* entityIdForLog= */ 42
|
||||
);
|
||||
|
||||
assertNull(result,
|
||||
"Null animator → null return (skip to avoid 400ms lockout on entity "
|
||||
+ "that never actually played).");
|
||||
assertTrue(LivingEntityPatch.LAST_SYNC_PLAY_BY_ENTITY.isEmpty(),
|
||||
"Precondition failure (null animator) must NOT burn spam map.");
|
||||
}
|
||||
|
||||
/**
|
||||
* Happy path : accessor valide + animator non-null → retourne la
|
||||
* registryName broadcastable. Garantit que le helper n'est pas
|
||||
* trop restrictif (toutes preconditions passent = return OK).
|
||||
*
|
||||
* <p>Ce test aussi vérifie que le helper ne touche PAS la spam map
|
||||
* (c'est {@code checkAndRecordSpam} qui doit s'en charger dans
|
||||
* {@code playAnimationSync} APRÈS appel de ce helper).</p>
|
||||
*/
|
||||
@Test
|
||||
@SuppressWarnings("unchecked")
|
||||
void resolveBroadcastableAnimId_happyPath_returnsRegistryName_noSpamTimestamp() {
|
||||
ResourceLocation expectedId =
|
||||
ResourceLocation.fromNamespaceAndPath("tiedup", "context_stand_idle");
|
||||
AnimationAccessor<StaticAnimation> accessor = mock(AnimationAccessor.class);
|
||||
when(accessor.registryName()).thenReturn(expectedId);
|
||||
Animator animator = mock(Animator.class);
|
||||
|
||||
ResourceLocation result = LivingEntityPatch.resolveBroadcastableAnimId(
|
||||
accessor, animator, /* entityIdForLog= */ 42
|
||||
);
|
||||
|
||||
assertNotNull(result, "All preconditions pass → return the registry name.");
|
||||
assertTrue(expectedId.equals(result),
|
||||
"Returned registryName must be exactly what accessor provided.");
|
||||
assertTrue(LivingEntityPatch.LAST_SYNC_PLAY_BY_ENTITY.isEmpty(),
|
||||
"Helper is side-effect-free — spam recording is playAnimationSync's job.");
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user