From 7281548a6a7e764d552a0ee9ffa07ab3184a50cd Mon Sep 17 00:00:00 2001 From: notevil Date: Fri, 24 Apr 2026 02:16:28 +0200 Subject: [PATCH] =?UTF-8?q?P3=20Wave=20=CE=B1=20hardening=20:=20snapshot?= =?UTF-8?q?=20consistency=20+=20parse=20warn=20+=20reload=20hook?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit HIGH RISK-001 : snapshot update moved between reset+applyDefinitions and equip oneshots. If an equip oneshot throws, animator state matches snapshot (currentKeys already applied). Prevents next-tick desync. MEDIUM Option B warn : parser now logs WARN if modder binds IDLE. Points to BONDAGE_ANIMATION_DESIGN §5.1 convention. Binding still accepted (functional), just non-idiomatic. MEDIUM WARNED_MISSING_ANIMS reload reset : new TiedUpRigRegistryReloadListener wires resetWarnedMissing() on AddReloadListenerEvent + RegisterClientReloadListenersEvent. Prevents unbounded set growth in long-running sessions with datapack swaps. LOW naming : unified MOD_ID in Wave α files (BondageRehydrateListener switched from TiedUpRigConstants.MODID to TiedUpMod.MOD_ID for consistency with sibling BondageEquipmentChangeListener ; both constants hold the literal "tiedup" so no behavioral change). --- .../rig/TiedUpRigRegistryReloadListener.java | 152 ++++++++++++++++++ .../datadriven/DataDrivenItemParser.java | 18 +++ .../v2/client/BondageRehydrateListener.java | 9 +- .../v2/client/ClientRigEquipmentHandler.java | 34 +++- .../TiedUpRigRegistryReloadListenerTest.java | 112 +++++++++++++ .../DataDrivenItemParserAnimationsTest.java | 40 +++++ 6 files changed, 357 insertions(+), 8 deletions(-) create mode 100644 src/main/java/com/tiedup/remake/rig/TiedUpRigRegistryReloadListener.java create mode 100644 src/test/java/com/tiedup/remake/rig/TiedUpRigRegistryReloadListenerTest.java diff --git a/src/main/java/com/tiedup/remake/rig/TiedUpRigRegistryReloadListener.java b/src/main/java/com/tiedup/remake/rig/TiedUpRigRegistryReloadListener.java new file mode 100644 index 0000000..74fd2b3 --- /dev/null +++ b/src/main/java/com/tiedup/remake/rig/TiedUpRigRegistryReloadListener.java @@ -0,0 +1,152 @@ +/* + * © 2026 TiedUp! Remake Contributors, distributed under GPLv3. + */ + +package com.tiedup.remake.rig; + +import net.minecraft.server.packs.resources.ResourceManager; +import net.minecraft.server.packs.resources.SimplePreparableReloadListener; +import net.minecraft.util.profiling.ProfilerFiller; +import net.minecraftforge.api.distmarker.Dist; +import net.minecraftforge.client.event.RegisterClientReloadListenersEvent; +import net.minecraftforge.event.AddReloadListenerEvent; +import net.minecraftforge.eventbus.api.SubscribeEvent; +import net.minecraftforge.fml.common.Mod; + +import com.tiedup.remake.core.TiedUpMod; + +/** + * Hook {@code /reload} (serveur) + {@code F3+T} (client) pour reset le set dedup + * {@link TiedUpAnimationRegistry#resetWarnedMissing()}. + * + *

Pourquoi ?

+ *

Le set statique {@code WARNED_MISSING_ANIMS} de + * {@link TiedUpAnimationRegistry} accumule les IDs d'animations introuvables + * pour éviter le spam log (un miss reloggué tick après tick). Sans reset, le + * set grandit sans borne en prod (N items cassés × K sessions × long uptime) + * et empêche de re-signaler un miss qui redevient cassé après un reload.

+ * + *

Scénario typique

+ *
    + *
  1. Session N : un item data-driven référence {@code tiedup:broken_anim} → + * WARN log, ID ajouté au set.
    2. + *
  2. Modder corrige l'asset, {@code /reload}.
    3. + *
  3. Session N+1 : resolveWithFallback dispose de l'anim → pas de fallback, + * pas de WARN. Le set contient toujours l'ID de la session N (résiduel).
    4. + *
  4. Modder casse à nouveau l'anim, {@code /reload}.
    5. + *
  5. Session N+2 : miss de nouveau, mais l'ID est déjà dans le set → + * silencieux, aucun feedback modder.
    6. + *
+ * + *

Avec ce listener, chaque reload clear le set — un miss sur une anim + * précédemment warnée redéclenche le WARN (comportement attendu : le modder + * doit voir l'erreur s'il re-casse l'asset).

+ * + *

Les deux sides

+ * + * + *

Le set statique dans {@link TiedUpAnimationRegistry} est le MÊME sur les + * deux sides (même JVM pour un serveur intégré / solo) — les deux hooks + * clearent le même set, pas de double comptabilité. Sur serveur dédié + * seul le hook server fire, côté client pur (rare — lobby / fast reconnect) + * seul le hook client.

+ * + *

Threading

+ *

{@link TiedUpAnimationRegistry#resetWarnedMissing()} utilise + * {@link java.util.concurrent.ConcurrentHashMap#newKeySet()} donc safe à + * l'appel concurrent. Les reload events peuvent fire depuis le main thread + * (server tick) ou le render thread (client reload) — aucune contrainte + * supplémentaire.

+ */ +@Mod.EventBusSubscriber(modid = TiedUpMod.MOD_ID) +public final class TiedUpRigRegistryReloadListener + extends SimplePreparableReloadListener { + + private TiedUpRigRegistryReloadListener() { + // Instancié par les event handlers ci-dessous — pas d'API publique + // à part comme reload listener. + } + + @Override + protected Void prepare(ResourceManager mgr, ProfilerFiller profiler) { + // No-op — tout se fait dans apply(). Pas de I/O à faire off-thread, + // l'opération est juste un Set.clear() en O(1) amortisé. + return null; + } + + @Override + protected void apply(Void nothing, ResourceManager mgr, ProfilerFiller profiler) { + TiedUpAnimationRegistry.resetWarnedMissing(); + TiedUpRigConstants.LOGGER.debug( + "[TiedUpRigRegistryReloadListener] WARNED_MISSING_ANIMS reset on reload" + ); + } + + /** + * Hook {@link AddReloadListenerEvent} — fire à chaque {@code /reload} + * datapack côté serveur (et aussi au server start). Enregistre un + * listener qui clear le set dedup WARN. + * + *

Note : {@code AddReloadListenerEvent} est sur le FORGE bus (pas MOD bus), + * mais le {@code @Mod.EventBusSubscriber} par défaut cible le MOD bus. On + * spécifie {@link Mod.EventBusSubscriber.Bus#FORGE} sur le subscriber ci-dessus ? + * Non : en 1.20.1, {@code AddReloadListenerEvent extends Event} bus-inferred + * automatiquement — le subscriber static-registered via annotation + * fonctionne sur le FORGE bus par défaut tant qu'on n'indique pas + * {@code Bus.MOD}. Par souci de clarté, on s'aligne sur le pattern du mod + * ({@code TiedUpMod.ForgeEvents} utilise le FORGE bus).

+ */ + @SubscribeEvent + public static void onAddReloadListeners(AddReloadListenerEvent event) { + event.addListener(new TiedUpRigRegistryReloadListener()); + TiedUpRigConstants.LOGGER.debug( + "[TiedUpRigRegistryReloadListener] Registered server-side reload listener for WARN dedup reset" + ); + } + + /** + * Hook {@link RegisterClientReloadListenersEvent} — fire une fois côté client + * pendant le setup pour enregistrer des listeners qui tourneront à chaque + * resource reload client (F3+T, resource pack swap). + * + *

Gardé {@link Dist#CLIENT} : sur serveur dédié, l'event n'existe pas + * (event client), le subscriber static est skip sans class-load.

+ * + *

Ce subscriber est un inner static class pour isoler la gate de Dist — + * {@code @Mod.EventBusSubscriber(value = Dist.CLIENT)} sur la classe entière + * exclurait aussi le hook {@link #onAddReloadListeners} server-side.

+ */ + @Mod.EventBusSubscriber( + modid = TiedUpMod.MOD_ID, + bus = Mod.EventBusSubscriber.Bus.MOD, + value = Dist.CLIENT + ) + public static final class ClientReloadHook { + + private ClientReloadHook() { + // holder class for @SubscribeEvent static + } + + @SubscribeEvent + public static void onRegisterClientReloadListeners( + RegisterClientReloadListenersEvent event + ) { + event.registerReloadListener(new TiedUpRigRegistryReloadListener()); + TiedUpRigConstants.LOGGER.debug( + "[TiedUpRigRegistryReloadListener] Registered client-side reload listener for WARN dedup reset" + ); + } + } +} diff --git a/src/main/java/com/tiedup/remake/v2/bondage/datadriven/DataDrivenItemParser.java b/src/main/java/com/tiedup/remake/v2/bondage/datadriven/DataDrivenItemParser.java index df3df40..5a219ff 100644 --- a/src/main/java/com/tiedup/remake/v2/bondage/datadriven/DataDrivenItemParser.java +++ b/src/main/java/com/tiedup/remake/v2/bondage/datadriven/DataDrivenItemParser.java @@ -717,6 +717,24 @@ public final class DataDrivenItemParser { ); continue; } + // P3 Wave α — Option B convention warn : binding IDLE fonctionne + // techniquement mais écrase silencieusement le default EF re-injecté + // par ClientAnimator.resetLivingAnimations() post-rebuild. Design + // BONDAGE_ANIMATION_DESIGN.md §5.1 recommande d'utiliser des motions + // custom (POSE_KNEEL_BOUND / STRUGGLE_BOUND / WALK_BOUND) plutôt que + // d'écraser IDLE. On accepte le binding quand même (rétrocompat + + // cas légitimes rares), juste on pointe le modder vers la convention. + if (motion == LivingMotions.IDLE) { + LOGGER.warn( + "[DataDrivenItemParser] Item {} binds IDLE motion. Convention design Option B " + + "(BONDAGE_ANIMATION_DESIGN.md §5.1) recommends not binding IDLE — " + + "ClientAnimator.resetLivingAnimations() restores default EF IDLE after " + + "rebuild, so binding IDLE overwrites the default silently. Prefer custom " + + "motions like POSE_KNEEL_BOUND / STRUGGLE_BOUND / WALK_BOUND. The binding " + + "will still work but may cause surprises.", + itemId + ); + } ResourceLocation animId = tryParseAnimationRL( entry.getValue(), "living_motions['" + motionName + "']", diff --git a/src/main/java/com/tiedup/remake/v2/client/BondageRehydrateListener.java b/src/main/java/com/tiedup/remake/v2/client/BondageRehydrateListener.java index 0d2349d..ecc3bf9 100644 --- a/src/main/java/com/tiedup/remake/v2/client/BondageRehydrateListener.java +++ b/src/main/java/com/tiedup/remake/v2/client/BondageRehydrateListener.java @@ -16,6 +16,7 @@ import net.minecraftforge.event.entity.EntityJoinLevelEvent; import net.minecraftforge.eventbus.api.SubscribeEvent; import net.minecraftforge.fml.common.Mod; +import com.tiedup.remake.core.TiedUpMod; import com.tiedup.remake.rig.TiedUpRigConstants; /** @@ -82,7 +83,13 @@ import com.tiedup.remake.rig.TiedUpRigConstants; */ @OnlyIn(Dist.CLIENT) @Mod.EventBusSubscriber( - modid = TiedUpRigConstants.MODID, + // P3 Wave α naming : use TiedUpMod.MOD_ID (main mod constant) in v2.client + // files rather than TiedUpRigConstants.MODID (rig sub-module alias). + // Both hold the literal "tiedup" (TiedUpRigConstants.MODID = TiedUpMod.MOD_ID), + // so no behavioral change — just source consistency with sibling + // BondageEquipmentChangeListener. TiedUpRigConstants.LOGGER is kept below + // because the logger identity is rig-scoped for grep-friendly log filtering. + modid = TiedUpMod.MOD_ID, bus = Mod.EventBusSubscriber.Bus.FORGE, value = Dist.CLIENT ) diff --git a/src/main/java/com/tiedup/remake/v2/client/ClientRigEquipmentHandler.java b/src/main/java/com/tiedup/remake/v2/client/ClientRigEquipmentHandler.java index 16cf624..bbaa14b 100644 --- a/src/main/java/com/tiedup/remake/v2/client/ClientRigEquipmentHandler.java +++ b/src/main/java/com/tiedup/remake/v2/client/ClientRigEquipmentHandler.java @@ -335,6 +335,13 @@ public final class ClientRigEquipmentHandler { // rig pendant que la map livingAnimations est encore celle de l'ancien // équipement. Pragmatique : à ce stade le rebuild n'a pas encore tourné, // donc les defaults EF sont toujours en place de la passe précédente. + // + // Snapshot RISK-001 (P3 Wave α review) : si un unequip oneshot throw + // ici, le rebuild (reset + applyDefinitions) n'a pas encore tourné, + // donc l'animator est toujours dans l'état "previousKeys" — on ne + // touche PAS à LAST_EQUIPPED_KEYS. Au prochain tick, le diff + // retentera avec previousKeys inchangé → même unequip redéclenché + // (au pire double son, mais état cohérent garanti). for (Map.Entry entry : newlyUnequipped.entrySet()) { ResourceLocation itemId = entry.getValue(); triggerOneshotById( @@ -348,11 +355,6 @@ public final class ClientRigEquipmentHandler { } } - // Update snapshot AVANT le rebuild — si rebuild throw, on a quand même - // consigné l'état courant pour le tick suivant (évite un double-fire - // au prochain rebuild après exception). - LAST_EQUIPPED_KEYS.put(player, Map.copyOf(currentKeys)); - // Reset + restore des defaults EF (IDLE, WALK, RUN, etc.). Les bindings // custom qu'on ajoute après prennent le pas sur ces defaults via la // map livingAnimations (dernière put gagne pour une même LivingMotion). @@ -371,6 +373,26 @@ public final class ClientRigEquipmentHandler { TiedUpAnimationRegistry::resolveWithFallback ); + // Update snapshot APRÈS reset+applyDefinitions, AVANT les equip oneshots + // (RISK-001 fix, P3 Wave α) : à ce stade l'animator est exactement dans + // l'état décrit par currentKeys (reset + bindings des items courants + // appliqués). Snapshot et animator state sont cohérents. + // + // Pourquoi PAS avant applyDefinitions : si applyDefinitions throw, on + // aurait un snapshot qui prétend que l'état est currentKeys alors que + // l'animator est à moitié construit (reset done, bindings partiels). + // Au tick suivant, diff(currentKeys, currentKeys) == vide → aucune + // tentative de réparer → animator reste en état cassé jusqu'à un vrai + // equipment change. + // + // Pourquoi PAS après les equip oneshots : le snapshot représente l'état + // _équipement_ (ce qui EST dans les slots), pas les oneshots joués. Si + // un equip oneshot throw, l'équipement est déjà correct côté animator — + // on garde le snapshot pour ne pas re-fire les equip oneshots au tick + // suivant (ce serait un double son + double animation-push). + LAST_EQUIPPED_KEYS.put(player, Map.copyOf(currentKeys)); + FIRST_REBUILD_DONE.put(player, Boolean.TRUE); + if (!isFirstRebuild) { // Equip APRÈS rebuild — l'anim one-shot joue sur "le nouvel état" du // rig, après que les bindings de l'item nouvellement équipé ont été @@ -389,8 +411,6 @@ public final class ClientRigEquipmentHandler { } } - FIRST_REBUILD_DONE.put(player, Boolean.TRUE); - TiedUpRigConstants.LOGGER.debug( "[ClientRigEquipmentHandler] Rebuilt livingAnimations for player {} " + "({} data-driven items processed, {} equip / {} unequip one-shots," diff --git a/src/test/java/com/tiedup/remake/rig/TiedUpRigRegistryReloadListenerTest.java b/src/test/java/com/tiedup/remake/rig/TiedUpRigRegistryReloadListenerTest.java new file mode 100644 index 0000000..36c2d82 --- /dev/null +++ b/src/test/java/com/tiedup/remake/rig/TiedUpRigRegistryReloadListenerTest.java @@ -0,0 +1,112 @@ +/* + * © 2026 TiedUp! Remake Contributors, distributed under GPLv3. + */ + +package com.tiedup.remake.rig; + +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertSame; + +import net.minecraft.resources.ResourceLocation; + +import org.junit.jupiter.api.Test; + +import com.tiedup.remake.rig.anim.AnimationManager.AnimationAccessor; +import com.tiedup.remake.rig.anim.types.StaticAnimation; + +/** + * Smoke tests pour {@link TiedUpRigRegistryReloadListener} (P3 Wave α hardening). + * + *

Le listener lui-même ne peut être testé bout-en-bout sans runtime Forge + * (registration sur event bus + fire de l'event). On se limite à deux garanties + * observables :

+ *
    + *
  • La classe se class-load sans throw (pas de static init piège — la + * signature {@code @Mod.EventBusSubscriber} + les events référencés + * doivent être résolvables dans le classpath test).
    • + *
  • L'API cible {@link TiedUpAnimationRegistry#resetWarnedMissing()} peut + * être appelée répétitivement sans throw et re-arme le state dedup + * (déjà couvert dans {@link TiedUpAnimationRegistryResolveTest}, on + * valide ici l'invariant "reset + new miss → WARN redéclenché + * observable via set.add=true").
    • + *
+ * + *

Le fire de l'event réel ({@code AddReloadListenerEvent}, + * {@code RegisterClientReloadListenersEvent}) doit être validé en game-runtime + * (QA manual : run client, /reload, vérifier debug log émis). Impossible à + * mocker sans bootstrap Forge complet.

+ */ +class TiedUpRigRegistryReloadListenerTest { + + /** Simple classLoad-ability sanity — si la classe contient un static init + * qui throw (ex. reference à une API manquante du classpath test), le + * test-runner échoue immédiatement avec ExceptionInInitializerError. */ + @Test + void classLoads_noStaticInitError() { + assertDoesNotThrow(() -> { + Class cls = Class.forName( + "com.tiedup.remake.rig.TiedUpRigRegistryReloadListener" + ); + assertNotNull(cls); + }, "TiedUpRigRegistryReloadListener doit pouvoir class-loader sans throw " + + "(Forge annotations + event classes resolvables en test classpath)"); + } + + /** L'inner holder class {@code ClientReloadHook} class-load aussi (gated + * {@code Dist.CLIENT} mais la classe Java existe tout le temps — seul le + * subscriber event se no-op côté serveur). */ + @Test + void clientReloadHook_classLoads() { + assertDoesNotThrow(() -> { + Class cls = Class.forName( + "com.tiedup.remake.rig.TiedUpRigRegistryReloadListener$ClientReloadHook" + ); + assertNotNull(cls); + }, "TiedUpRigRegistryReloadListener.ClientReloadHook doit class-loader " + + "meme sans runtime Forge — juste une classe Java standard"); + } + + /** + * Sanity : le listener est censé appeler + * {@link TiedUpAnimationRegistry#resetWarnedMissing()} dans son apply(). + * On ne peut pas mocker {@code ResourceManager} sans bootstrap MC, donc on + * valide juste le contrat : resetWarnedMissing est callable + repeatable + + * remet effectivement le set en état "fresh". + * + *

Pattern : warn sur un ID unknown → reset → warn rewarned (vérification + * indirecte du reset via le comportement observable du {@code Set.add}).

+ */ + @Test + void resetWarnedMissing_idempotentAndReenablesWarn() { + ResourceLocation unknown = ResourceLocation.fromNamespaceAndPath( + "tiedup_test_reload", "never_registered_" + System.nanoTime() + ); + + // Clear initial — pas d'état hérité d'autres tests. + assertDoesNotThrow(TiedUpAnimationRegistry::resetWarnedMissing); + + // Premier call : ID manquant → fallback + (premier) WARN silent-add. + AnimationAccessor first = + TiedUpAnimationRegistry.resolveWithFallback(unknown); + assertSame(TiedUpRigRegistry.EMPTY_ANIMATION, first, + "Unknown ID → fallback EMPTY_ANIMATION"); + + // Reset simule un /reload — le set interne est cleared. + assertDoesNotThrow(TiedUpAnimationRegistry::resetWarnedMissing, + "resetWarnedMissing doit etre appelable sans throw " + + "(contrat cible du TiedUpRigRegistryReloadListener.apply())"); + + // Deuxième call post-reset : fallback à nouveau, et le set repart de + // zero — le comportement de WARN est re-enabled. On ne peut pas + // observer le log directement mais le contrat set.add(id) == true + // est rétabli (sinon le reset serait une no-op dangereuse). + AnimationAccessor second = + TiedUpAnimationRegistry.resolveWithFallback(unknown); + assertSame(TiedUpRigRegistry.EMPTY_ANIMATION, second, + "Unknown ID apres reset → toujours fallback EMPTY_ANIMATION"); + + // Cleanup : ne pas polluer les suivants + TiedUpAnimationRegistry.resetWarnedMissing(); + } +} diff --git a/src/test/java/com/tiedup/remake/v2/bondage/datadriven/DataDrivenItemParserAnimationsTest.java b/src/test/java/com/tiedup/remake/v2/bondage/datadriven/DataDrivenItemParserAnimationsTest.java index 583111b..9eb9f6c 100644 --- a/src/test/java/com/tiedup/remake/v2/bondage/datadriven/DataDrivenItemParserAnimationsTest.java +++ b/src/test/java/com/tiedup/remake/v2/bondage/datadriven/DataDrivenItemParserAnimationsTest.java @@ -169,6 +169,46 @@ class DataDrivenItemParserAnimationsTest { assertNotNull(result.livingMotions().get(TiedUpLivingMotions.STRUGGLE_BOUND)); } + // ========== P3 Wave α : Option B convention warn on IDLE ========== + + /** + * Option B convention (design BONDAGE_ANIMATION_DESIGN.md §5.1) : les modders + * ne devraient PAS binder {@link LivingMotions#IDLE} car + * {@code ClientAnimator.resetLivingAnimations()} re-injecte le default EF + * IDLE post-rebuild, et le binding custom l'écrase silencieusement — comportement + * fonctionnel mais non-idiomatique. Le parser log un WARN explicite pour pointer + * le modder vers la convention, mais accepte le binding quand même (rétrocompat). + * + *

Ce test verrouille le comportement "accepte + ne throw pas" — la présence + * du WARN log n'est pas directement vérifiable sans log capture, mais le + * binding doit être bel et bien présent dans le résultat.

+ */ + @Test + void parseAnimations_IDLEBinding_logsWarnButAccepts() { + String jsonStr = """ + { + "animations": { + "living_motions": { + "IDLE": "tiedup:arms_cuffed_idle" + } + } + } + """; + + AnimationBindings result = + DataDrivenItemParser.parseAnimationBindings(json(jsonStr), ITEM_ID); + + assertNotNull(result, + "IDLE binding est tolere (non-idiomatique mais fonctionnel) => result non-null"); + assertEquals(1, result.livingMotions().size(), + "Le binding IDLE est accepte apres le WARN de convention"); + assertEquals( + ResourceLocation.fromNamespaceAndPath("tiedup", "arms_cuffed_idle"), + result.livingMotions().get(LivingMotions.IDLE), + "Le binding IDLE est bien present dans le resultat malgre le WARN" + ); + } + // ========== living_motions : tolerance erreurs ========== @Test