diff --git a/src/main/java/com/tiedup/remake/rig/patch/LivingEntityPatch.java b/src/main/java/com/tiedup/remake/rig/patch/LivingEntityPatch.java index 4e1c9d4..ea79fc4 100644 --- a/src/main/java/com/tiedup/remake/rig/patch/LivingEntityPatch.java +++ b/src/main/java/com/tiedup/remake/rig/patch/LivingEntityPatch.java @@ -165,12 +165,11 @@ public abstract class LivingEntityPatch 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.

- */ - /** - * 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. + * + *

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.

*/ static final Map LAST_SYNC_PLAY_BY_ENTITY = Collections.synchronizedMap(new WeakHashMap<>()); @@ -218,20 +217,53 @@ public abstract class LivingEntityPatch extends EntityPa * les animations ambient / motion-driven, préférer {@code addLivingAnimation} * sur l'animator via le pipeline d'équipement data-driven.

* - *

Anti-spam guard : 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.

+ *

Ordre des preconditions (P3-12 review fix)

+ *

L'ordre des vérifications est load-bearing pour éviter deux classes + * de bugs :

+ *
    + *
  1. Null accessor / registryName : 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.
    2. + *
  2. Animator null : 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.
    3. + *
  3. Anti-spam : le timestamp n'est enregistré qu'APRÈS les deux + * preconditions, pour éviter un lockout sur un call qui n'a rien fait.
    4. + *
* - *

Server-side play : 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.

+ *

Server-side play

+ *

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.

* - *

Priority non-forwardée : 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 + *

Anti-spam design (400ms global)

+ *

Un guard global de 400 ms par entity drop les calls {@code playAnimationSync} + * suivants. C'est INTENTIONAL 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).

+ * + *

Edge case acceptée : 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.

+ * + *

Pour la locomotion (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.

+ * + *

Phase 4 : 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.

+ * + *

Priority non-forwardée

+ *

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.

* * @param accessor l'animation à jouer (typiquement résolue via @@ -252,34 +284,29 @@ public abstract class LivingEntityPatch 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 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é). + * + *

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).

+ * + *

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.

+ * + * @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 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; + } } diff --git a/src/test/java/com/tiedup/remake/rig/patch/LivingEntityPatchSpamGuardTest.java b/src/test/java/com/tiedup/remake/rig/patch/LivingEntityPatchSpamGuardTest.java index 24813bb..8687e20 100644 --- a/src/test/java/com/tiedup/remake/rig/patch/LivingEntityPatchSpamGuardTest.java +++ b/src/test/java/com/tiedup/remake/rig/patch/LivingEntityPatchSpamGuardTest.java @@ -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 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 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). + * + *

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).

+ */ + @Test + @SuppressWarnings("unchecked") + void resolveBroadcastableAnimId_happyPath_returnsRegistryName_noSpamTimestamp() { + ResourceLocation expectedId = + ResourceLocation.fromNamespaceAndPath("tiedup", "context_stand_idle"); + AnimationAccessor 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."); + } }