From d90ff146682d85a11a3386049040200aff8dc74a Mon Sep 17 00:00:00 2001 From: notevil Date: Mon, 27 Apr 2026 00:20:01 +0200 Subject: [PATCH] RISK-003 self-heal coverage : extract shouldRebindIdle helper + tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Le reviewer flagait une race entre PlayerPatch.initAnimator (bind IDLE au capability attach time) et AnimationManager.apply (datapack reload). En investigant : la race existe architecturalement mais le self-heal est deja en place via maybePlayIdle — quand resolveWithFallback retourne EMPTY pre-apply, le bind EMPTY est mis dans la map (EMPTY_ANIMATION a isPresent()==true donc passe checkNull). Au tick suivant, une fois apply tourne et le registry populé, maybePlayIdle detecte currentIdleBind == EMPTY et rebind sur la vraie anim. Le bind EMPTY est intentionnel : ClientAnimator.postInit appelle playAnimationInstantly(livingAnimations.get(IDLE)) immediatement, et skipper le bind crasherait postInit (NPE sur null.get()). EMPTY a une pose identity (no-op visuel) → bootstrap propre, self-heal post-reload. Le symptome runtime "F6 bindings: 1 mais idle silent" decrit dans la review vient d'un cause differente : AnimationManager n'est pas encore register comme reload listener (Phase 2 wiring pending, tracked sous BUG-RACE-01 dans AnimationManager.apply javadoc). Une fois cable, le self-heal fonctionne — verrouille par les 5 nouveaux tests. Changements : - RigAnimationTickHandler : extract shouldRebindIdle(currentBind, target) pure helper package-private. La logique inline dans maybePlayIdle reste identique, juste deleguee au helper pour testabilite. - PlayerPatch.initAnimator : javadoc enrichi (race + raison du bind EMPTY + reference test coverage). - RigAnimationTickHandlerTest : +5 cases sur shouldRebindIdle (null+real, empty+real RACE, real+real, two real instances, *+empty). Tests : 459 → 464 (+5), tous green. --- .../tiedup/remake/rig/patch/PlayerPatch.java | 49 ++++- .../rig/tick/RigAnimationTickHandler.java | 51 ++++- .../rig/tick/RigAnimationTickHandlerTest.java | 175 +++++++++++++++++- 3 files changed, 264 insertions(+), 11 deletions(-) diff --git a/src/main/java/com/tiedup/remake/rig/patch/PlayerPatch.java b/src/main/java/com/tiedup/remake/rig/patch/PlayerPatch.java index 0d63336..2bed87d 100644 --- a/src/main/java/com/tiedup/remake/rig/patch/PlayerPatch.java +++ b/src/main/java/com/tiedup/remake/rig/patch/PlayerPatch.java @@ -270,15 +270,47 @@ public abstract class PlayerPatch extends LivingEntityPatch * IDLE sur {@code tiedup:context_stand_idle}, résolue via le registry * data-driven ({@link TiedUpAnimationRegistry#resolveWithFallback}). * - *

Pre-reload window : si le patch est construit avant que - * {@code AnimationManager.apply} n'ait tourné (bootstrap client, race - * entre join et resource-pack reload, etc.), l'ID {@code context_stand_idle} - * n'est pas encore dans {@code animationByName} et - * {@code resolveWithFallback} retourne {@link TiedUpRigRegistry#EMPTY_ANIMATION}. - * C'est idempotent côté animator — une prochaine passe de - * {@link com.tiedup.remake.rig.tick.RigAnimationTickHandler#maybePlayIdle} - * re-bind sur la vraie anim une fois le datapack chargé (self-heal).

+ *

Pre-reload window — race avec AnimationManager.apply (RISK-003)

+ *

Si le patch est construit AVANT que + * {@link com.tiedup.remake.rig.anim.AnimationManager#apply} n'ait tourné + * (bootstrap client, race entre join et resource-pack reload, capability + * attach pré-{@code AddReloadListenerEvent} en Phase 2 du wiring), l'ID + * {@code tiedup:context_stand_idle} n'est pas encore dans + * {@code animationByName} et {@code resolveWithFallback} retourne + * {@link TiedUpRigRegistry#EMPTY_ANIMATION}.

* + *

Pourquoi on bind quand même EMPTY_ANIMATION (au lieu de skip)

+ *

{@link com.tiedup.remake.rig.anim.client.ClientAnimator#postInit} appelle + * immédiatement {@code playAnimationInstantly(livingAnimations.get(IDLE))}. + * Si on skipait le bind ({@code null} dans la map), {@code postInit} NPE + * sur {@code nextAnimation.get()}. EMPTY_ANIMATION a {@code isPresent()==true} + * (cf. {@link com.tiedup.remake.rig.anim.types.DirectStaticAnimation#isPresent}) + * donc passe le {@code AnimationManager.checkNull} guard et bootstrap + * proprement avec une pose identity (no-op visuel).

+ * + *

Self-heal après reload

+ *

Une fois {@code AnimationManager.apply} terminé (datapack chargé, anim + * réelle dans le registry), + * {@link com.tiedup.remake.rig.tick.RigAnimationTickHandler#maybePlayIdle} + * détecte le bind EMPTY au tick suivant via + * {@link com.tiedup.remake.rig.tick.RigAnimationTickHandler#shouldRebindIdle} + * et rebind sur la vraie anim. Idempotent — les + * {@code addLivingAnimation} suivantes écrasent l'entry existante. Aucun + * code "retry hook" post-reload n'est nécessaire : le tick handler observe + * en pull (chaque tick) plutôt qu'en push (callback reload).

+ * + *

Couverture test (RISK-003 verrouillé)

+ *

Voir {@code RigAnimationTickHandlerTest.shouldRebindIdle_*} pour les + * tests unitaires de la logique de self-heal pure (sans MC bootstrap). + * Cas couverts :

+ *
    + *
  • {@code currentBind=null, target=real} → rebind
    • + *
  • {@code currentBind=EMPTY, target=real} → rebind (race scenario)
    • + *
  • {@code currentBind=real, target=real} → no-op (steady state)
    • + *
  • {@code currentBind=*, target=EMPTY} → no-op (apply pas encore tourné)
    • + *
+ * + *

Pourquoi on se limite à IDLE

*

L'ordre EF ajoute ~25 motions différentes (WALK, RUN, SNEAK, SIT, * SLEEP, etc.). On se limite à IDLE ici — ajouter les autres sans anim * source = pollution registre pour rien. Au fur et à mesure que les JSON @@ -291,6 +323,7 @@ public abstract class PlayerPatch extends LivingEntityPatch // Full data-driven : lookup par ID, EMPTY_ANIMATION fallback si // l'asset n'est pas encore chargé (rare mais possible au bootstrap). // Le tick handler self-heal le bind une fois le datapack loadé. + // Voir javadoc ci-dessus pour le contrat complet (RISK-003). AnimationAccessor idle = TiedUpAnimationRegistry.resolveWithFallback( TiedUpAnimationRegistry.CONTEXT_STAND_IDLE_ID diff --git a/src/main/java/com/tiedup/remake/rig/tick/RigAnimationTickHandler.java b/src/main/java/com/tiedup/remake/rig/tick/RigAnimationTickHandler.java index 68b4b45..ac750f7 100644 --- a/src/main/java/com/tiedup/remake/rig/tick/RigAnimationTickHandler.java +++ b/src/main/java/com/tiedup/remake/rig/tick/RigAnimationTickHandler.java @@ -261,13 +261,62 @@ public final class RigAnimationTickHandler { // EMPTY_ANIMATION. Dans les deux cas on rebind vers le target résolu. AssetAccessor currentIdleBind = clientAnimator.getLivingAnimation(LivingMotions.IDLE, null); - if (currentIdleBind == null || currentIdleBind == TiedUpRigRegistry.EMPTY_ANIMATION) { + if (shouldRebindIdle(currentIdleBind, target)) { clientAnimator.addLivingAnimation(LivingMotions.IDLE, target); } clientAnimator.playAnimation(target, 0.2F); } + /** + * Pure helper — décide si le bind IDLE courant doit être remplacé par + * {@code target}. Extrait pour tests unitaires sans bootstrap MC (cf. + * {@code RigAnimationTickHandlerTest.shouldRebindIdle_*}). + * + *

Contrat : retourne {@code true} ssi {@code target} est une + * vraie anim (pas {@link TiedUpRigRegistry#EMPTY_ANIMATION}) ET que le + * bind courant est soit absent ({@code null}), soit la sentinelle EMPTY. + * Dans tous les autres cas ({@code target} non-EMPTY identique au bind + * courant, ou {@code target} EMPTY) on garde le bind existant.

+ * + *

L'appelant ({@link #maybePlayIdle}) a déjà filtré + * {@code target == EMPTY_ANIMATION} en early-return — la condition + * {@code target != EMPTY} ici est défensive (l'helper reste correct si + * jamais le filtre upstream est modifié).

+ * + *

Race scenario couvert (RISK-003) :

+ *
    + *
  1. {@code PlayerPatch.initAnimator} construit pre-{@code apply()} : + * {@code resolveWithFallback} retourne EMPTY → {@code addLivingAnimation} + * met EMPTY dans la map (car {@code EMPTY_ANIMATION.isPresent()==true} + * — cf. {@link com.tiedup.remake.rig.anim.types.DirectStaticAnimation#isPresent}).
    2. + *
  2. {@code AnimationManager.apply()} tourne plus tard (datapack reload + * wiring Phase 2). Le registry contient maintenant un nouvel accessor + * pour {@code context_stand_idle}.
    3. + *
  3. Tick suivant : {@code maybePlayIdle} résout {@code target = real + * accessor}, voit {@code currentIdleBind == EMPTY}, rebind via cet + * helper.
    4. + *
+ * + * @param currentBind bind IDLE courant (peut être {@code null} si jamais + * bound, ou {@link TiedUpRigRegistry#EMPTY_ANIMATION} si + * bound pré-apply) + * @param target target résolu via {@link TiedUpAnimationRegistry#resolveWithFallback} + * (par contrat upstream, jamais {@code null}) + * @return {@code true} si rebind nécessaire, {@code false} sinon + */ + static boolean shouldRebindIdle( + AssetAccessor currentBind, + AssetAccessor target + ) { + // Garde défensive : un target EMPTY ne doit jamais écraser un bind + // existant (régresserait l'état). En pratique l'appelant filtre déjà. + if (target == TiedUpRigRegistry.EMPTY_ANIMATION) { + return false; + } + return currentBind == null || currentBind == TiedUpRigRegistry.EMPTY_ANIMATION; + } + /** * Reset des erreurs loggées. Appelé : *
    diff --git a/src/test/java/com/tiedup/remake/rig/tick/RigAnimationTickHandlerTest.java b/src/test/java/com/tiedup/remake/rig/tick/RigAnimationTickHandlerTest.java index c95d6cc..583ccf6 100644 --- a/src/test/java/com/tiedup/remake/rig/tick/RigAnimationTickHandlerTest.java +++ b/src/test/java/com/tiedup/remake/rig/tick/RigAnimationTickHandlerTest.java @@ -5,10 +5,17 @@ package com.tiedup.remake.rig.tick; import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.Test; +import com.tiedup.remake.rig.TiedUpRigRegistry; +import com.tiedup.remake.rig.anim.AnimationManager; +import com.tiedup.remake.rig.anim.AnimationManager.AnimationAccessor; +import com.tiedup.remake.rig.anim.types.StaticAnimation; + /** * Tests de {@link RigAnimationTickHandler} — uniquement la logique pure Java * accessible sans MC runtime. @@ -18,12 +25,14 @@ import org.junit.jupiter.api.Test; * impossible sans bootstrap Forge. * - tickPlayer : necessite TiedUpCapabilities.getEntityPatch() qui utilise * LazyOptional (Forge runtime) + un Player reel. - * - maybePlayIdle : necessite ClientAnimator + LivingEntityPatch (Forge - * capability + MC class hierarchy). + * - maybePlayIdle (full path) : necessite ClientAnimator + LivingEntityPatch + * (Forge capability + MC class hierarchy). * * Ce qui est couvert : * - resetLoggedErrors() : expose explicitement pour les tests (Javadoc l'indique). * Verifie que la methode est idempotente et ne throw pas. + * - shouldRebindIdle() : helper pur extrait pour tester la logique de self-heal + * du bind IDLE post-{@code AnimationManager.apply()} (RISK-003). */ class RigAnimationTickHandlerTest { @@ -65,4 +74,166 @@ class RigAnimationTickHandlerTest { RigAnimationTickHandler.resetLoggedErrors(); }, "Second reset doit rester idempotent (simulation F3+T double-reload)"); } + + // === shouldRebindIdle — RISK-003 self-heal logic ============================ + // + // Verrouille le contrat de self-heal du bind IDLE : si initAnimator a bound + // EMPTY (apply pas encore tourné), le tick handler doit rebind sur la vraie + // anim une fois le registry populé. Voir PlayerPatch.initAnimator javadoc + // pour le scenario complet. + // + // Tests sans MC bootstrap : on injecte des accessors fakes via + // AnimationManager.AnimationBuilder (n'invoque pas onLoad tant qu'on ne + // call pas .get()). + + /** Namespace arbitraire pour les IDs de test — isole des vraies registrations. */ + private static final String TEST_NS = "tiedup_test_rebind"; + + /** + * Cas 1/4 — currentBind=null, target=real → rebind nécessaire. + * + *

    Représente le cas "patch fraîchement construit, jamais bound IDLE + * encore" (théorique : initAnimator skip le bind, scénario alternatif). + * Le rebind doit fire pour amorcer la map.

    + */ + @Test + void shouldRebindIdle_nullCurrent_realTarget_returnsTrue() { + AnimationAccessor realTarget = registerFakeAccessor("real_anim_1"); + + boolean result = RigAnimationTickHandler.shouldRebindIdle(null, realTarget); + + assertTrue(result, "currentBind=null + target=real doit déclencher rebind " + + "(amorce la map, sinon postInit aurait NPE — defensive case)."); + } + + /** + * Cas 2/4 — currentBind=EMPTY, target=real → rebind (RACE SCENARIO RISK-003). + * + *

    C'est LE scenario que RISK-003 décrit :

    + *
      + *
    1. {@code PlayerPatch.initAnimator} fire pre-{@code apply} → bind EMPTY.
      2. + *
    2. {@code AnimationManager.apply()} tourne → registry contient real anim.
      3. + *
    3. Tick suivant : {@code maybePlayIdle} résout {@code target=real}, + * voit {@code currentIdleBind=EMPTY}, doit rebind.
      4. + *
    + * + *

    Si ce test échoue, le self-heal est cassé et l'idle reste silent + * (symptôme F6 overlay {@code bindings: 1} avec EMPTY).

    + */ + @Test + void shouldRebindIdle_emptyCurrent_realTarget_returnsTrue() { + AnimationAccessor realTarget = registerFakeAccessor("real_anim_2"); + + boolean result = RigAnimationTickHandler.shouldRebindIdle( + TiedUpRigRegistry.EMPTY_ANIMATION, + realTarget + ); + + assertTrue(result, "RISK-003 : currentBind=EMPTY + target=real DOIT rebind. " + + "Régression si le self-heal post-AnimationManager.apply() ne triggere " + + "plus → idle silent à vie après bootstrap pre-reload."); + } + + /** + * Cas 3/4 — currentBind=real, target=real (même instance) → no-op. + * + *

    Steady state : le bind est correct, pas de rebind à faire. + * Idempotence du tick handler.

    + */ + @Test + void shouldRebindIdle_realCurrent_realTarget_returnsFalse() { + AnimationAccessor realAnim = registerFakeAccessor("real_anim_3"); + + boolean result = RigAnimationTickHandler.shouldRebindIdle(realAnim, realAnim); + + assertFalse(result, "Steady state (current=target=real) ne doit pas rebind " + + "— sinon spam d'addLivingAnimation à chaque tick (perf hit + log noise)."); + } + + /** + * Cas 3 bis — currentBind=real (instance A), target=real (instance B, même ID). + * + *

    Post-reload : l'accessor a une nouvelle instance (cf. + * {@code AnimationAccessorImpl} créé fresh dans + * {@code AnimationManager.readResourcepackAnimation}). On considère ça + * comme steady state — l'helper compare par identité (pas registryName) + * pour rester simple, et le caller gère le re-binding via la check + * {@code currentAnim.registryName().equals(targetId)} à un autre niveau.

    + * + *

    Cas important pour comprendre la portée de ce helper : il NE résout + * PAS le cas "post-reload, ancienne instance toujours bound mais registry + * a une nouvelle instance avec même ID". Cette resolution est handled par + * la check précédente dans {@code maybePlayIdle} qui compare via + * {@code registryName()}.

    + */ + @Test + void shouldRebindIdle_differentInstancesSameId_returnsTrue() { + // Deux instances distinctes mais avec le MÊME registry name. + // Simule "pré-reload bind" vs "post-reload re-instantiation" du même asset. + AnimationAccessor oldInstance = registerFakeAccessor("realod_test"); + AnimationAccessor newInstance = registerFakeAccessor("realod_test_new"); + // Note : on ne peut pas avoir 2 accessors avec le même path via + // AnimationBuilder (nextAccessor nomme via path donné). On simule juste + // "deux instances réelles distinctes" = bind différent, helper rebind. + + boolean result = RigAnimationTickHandler.shouldRebindIdle(oldInstance, newInstance); + + // shouldRebindIdle compare par identité de référence — un current real + // != target real returns false (ne fait pas de smart "same registryName" + // check). La désynchro registry-name post-reload est gérée par + // maybePlayIdle via une check upstream sur registryName().equals(targetId). + assertFalse(result, "shouldRebindIdle compare par identité — current=real !=null " + + "ET target=real → on ne rebind pas même si instances diffèrent. " + + "La détection post-reload se fait upstream (registryName check)."); + } + + /** + * Cas 4/4 — currentBind=*, target=EMPTY → no-op (jamais écraser un bind par EMPTY). + * + *

    Garde défensive : si {@code AnimationManager.apply()} n'a pas encore + * tourné, {@code resolveWithFallback} retourne EMPTY. Dans ce cas le + * caller ({@code maybePlayIdle}) a déjà early-returned, mais l'helper + * reste safe si jamais le filtre upstream est modifié — un EMPTY target + * ne doit jamais régresser un bind real existant.

    + */ + @Test + void shouldRebindIdle_emptyTarget_returnsFalse() { + AnimationAccessor realCurrent = registerFakeAccessor("real_anim_5"); + + // Avec current=real et target=EMPTY : pas de rebind. + boolean withReal = RigAnimationTickHandler.shouldRebindIdle( + realCurrent, TiedUpRigRegistry.EMPTY_ANIMATION + ); + assertFalse(withReal, "target=EMPTY ne doit jamais écraser un bind real " + + "(régresserait l'état). Defensive guard contre futur bug upstream."); + + // Avec current=null et target=EMPTY : pas de rebind non plus (rien à + // faire de productif — laisse postInit gérer le bootstrap initial). + boolean withNull = RigAnimationTickHandler.shouldRebindIdle( + null, TiedUpRigRegistry.EMPTY_ANIMATION + ); + assertFalse(withNull, "target=EMPTY + current=null : pas de rebind, " + + "rien à amorcer car EMPTY n'apporte aucune info utile."); + + // Cas dégénéré : current=EMPTY, target=EMPTY — aucune progression possible. + boolean withEmpty = RigAnimationTickHandler.shouldRebindIdle( + TiedUpRigRegistry.EMPTY_ANIMATION, TiedUpRigRegistry.EMPTY_ANIMATION + ); + assertFalse(withEmpty, "current=EMPTY + target=EMPTY : registry pas encore " + + "populé, attendre la prochaine reload — pas de rebind."); + } + + /** + * Inject un accessor fake dans {@link AnimationManager} via le builder + * public. {@code onLoad} retourne null mais ce n'est pas invoqué tant + * qu'on ne call pas {@code .get()} dessus dans les tests (on compare + * uniquement par identité de référence). + */ + private static AnimationAccessor registerFakeAccessor(String idPath) { + // Suffix nano pour éviter les collisions entre test cases (le builder + // jette IllegalArgumentException si le namespace est déjà utilisé). + String uniqueNs = TEST_NS + "_" + System.nanoTime(); + return new AnimationManager.AnimationBuilder(uniqueNs, b -> {}) + .nextAccessor(idPath, acc -> null); + } }