From dd30a8d4f9601fa9e4deb28a2c783423e77a06b7 Mon Sep 17 00:00:00 2001 From: notevil Date: Fri, 24 Apr 2026 10:48:57 +0200 Subject: [PATCH] P3 cross-check fixes : composite motion + dead code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit HIGH fix : currentCompositeMotion was initialized to IDLE and never written — any COMPOSITE_LAYER overlay bound to a non-IDLE motion would never fire (e.g. cuffs_arms_idle overlay on WALK_BOUND). Set composite = currentLivingMotion in updateMotion, matching EF pattern AbstractClientPlayerPatch:129-141. MEDIUM cleanup : triggerOneshot was 85% duplicate of triggerOneshotById and had 0 prod callers (tests only). Removed + migrated 6 tests to triggerOneshotById. LOW doc fix : ExtendableEnumManager.synchronized javadoc over-sold the risk as 'BUG-001 review fix' — it's actually conservative defense against a theoretical cross-classloader race. EF upstream runs without sync since 2020. Kept synchronized (no cost, no harm) but documented honestly. --- .../tiedup/remake/rig/patch/PlayerPatch.java | 27 +++- .../rig/util/ExtendableEnumManager.java | 32 +++-- .../v2/client/ClientRigEquipmentHandler.java | 86 +----------- .../patch/PlayerPatchUpdateMotionTest.java | 10 ++ .../client/ClientRigEquipmentHandlerTest.java | 128 ++++-------------- 5 files changed, 86 insertions(+), 197 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 ef6f4ab..f1de548 100644 --- a/src/main/java/com/tiedup/remake/rig/patch/PlayerPatch.java +++ b/src/main/java/com/tiedup/remake/rig/patch/PlayerPatch.java @@ -133,7 +133,7 @@ public abstract class PlayerPatch extends LivingEntityPatch boolean sneaking = p.isShiftKeyDown(); boolean bound = BondageStateHelpers.isBound(p); - this.currentLivingMotion = resolveMotion( + LivingMotion motion = resolveMotion( onFurnitureVehicle, sleepingBound, dogPose, @@ -143,6 +143,31 @@ public abstract class PlayerPatch extends LivingEntityPatch moving, sneaking, bound); + this.currentLivingMotion = motion; + + // Composite passe — aligne la motion composite sur la motion base. + // + // EF pattern reference : AbstractClientPlayerPatch.updateMotion:129-141 + // fait une passe composite dédiée après la passe base. Leur fallback + // par défaut (aucun item custom, aucune use-anim vanilla) est + // {@code currentCompositeMotion = currentLivingMotion} (:154). + // + // On adopte ce fallback comme comportement unique MVP : les overlays + // COMPOSITE_LAYER bindés par un data-driven item (ex. cuffs_arms_walk + // attaché à WALK_BOUND via bindings.composite, ou cuffs_arms_idle sur + // IDLE) seront bien lookup par ClientAnimator.tick() qui lit + // compositeLivingAnimations[currentCompositeMotion]. Sans cette + // assignation, le field restait initialisé à IDLE (init + // LivingEntityPatch:56) à vie — aucun overlay non-IDLE ne pouvait fire. + // + // Évolutions Phase 4+ si besoin : + // - composite désaligné du base (ex. body POSE_SLEEP_BOUND + arms + // overlay IDLE) → ajouter une seconde résolution dédiée + // - use-anim vanilla (BLOCK / EAT / DRINK / SPYGLASS) → branches + // équivalentes à EF:141-154 si on ajoute le combat + // - item capability custom motion (EF:134-140) → inutile tant que + // TiedUp n'a pas d'items "skill-based" + this.currentCompositeMotion = motion; } /** diff --git a/src/main/java/com/tiedup/remake/rig/util/ExtendableEnumManager.java b/src/main/java/com/tiedup/remake/rig/util/ExtendableEnumManager.java index f6c011d..3629e6d 100644 --- a/src/main/java/com/tiedup/remake/rig/util/ExtendableEnumManager.java +++ b/src/main/java/com/tiedup/remake/rig/util/ExtendableEnumManager.java @@ -43,11 +43,17 @@ public class ExtendableEnumManager { } /** - * Synchronized to guard against concurrent class-load of multiple extendable - * enum classes on different threads. The reflective invoke of values() below - * triggers static init which calls {@link #assign(ExtendableEnum)} — also - * synchronized on the same monitor (re-entrant, no deadlock risk). See - * review BUG-001 on P3-01 commit 15e405f. + * {@code synchronized} par conservatisme. Le {@code } JVM est déjà + * sérialisé par classloader lock pour un même classloader, et Forge + * {@code FMLCommonSetupEvent} tourne sur main thread, donc en pratique un + * seul thread atteint cette méthode. Le risque n'est que théorique si un + * mod tiers avec un classloader distinct étend {@code LivingMotion} en + * parallèle via une réflection exotique. EF upstream vit sans sync depuis + * 2020 — on le garde ici car le coût est nul (bootstrap only, N appels + * max) et la garantie explicite coûte moins qu'un audit futur pour la + * retirer. Le {@link #assign(ExtendableEnum)} ré-entre sur le même + * monitor depuis les {@code } déclenchés par l'invoke reflective + * ci-dessous (re-entrant, no deadlock risk). */ public synchronized void loadEnum() { List orderByModid = new ArrayList<>(this.enums.keySet()); @@ -76,14 +82,14 @@ public class ExtendableEnumManager { } /** - * Synchronized because {@code lastOrdinal}, {@code enumMapByOrdinal} - * ({@link Int2ObjectLinkedOpenHashMap}) and {@code enumMapByName} - * ({@link java.util.LinkedHashMap}) are not thread-safe. Concurrent class - * init of extendable enums on different threads (plausible if another mod - * extends LivingMotion at the same bootstrap) would race on the - * read-modify-write of lastOrdinal and on map mutation. No perf impact — - * called N×|motions| times at bootstrap only. See review BUG-001 on P3-01 - * commit 15e405f. + * {@code synchronized} par conservatisme — même justification que + * {@link #loadEnum()}. {@code lastOrdinal}, {@code enumMapByOrdinal} + * ({@link Int2ObjectLinkedOpenHashMap}) et {@code enumMapByName} + * ({@link java.util.LinkedHashMap}) ne sont pas thread-safe ; le + * risque théorique d'un cross-classloader race (mod tiers qui étend + * {@code LivingMotion} sur un thread séparé durant le bootstrap) est + * couvert sans coût observable (N × |motions| appels au bootstrap). + * EF upstream fonctionne sans sync depuis 2020. */ public synchronized int assign(T value) { int lastOrdinal = this.lastOrdinal; 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 79f899c..50741b1 100644 --- a/src/main/java/com/tiedup/remake/v2/client/ClientRigEquipmentHandler.java +++ b/src/main/java/com/tiedup/remake/v2/client/ClientRigEquipmentHandler.java @@ -9,7 +9,6 @@ import java.util.Collections; import java.util.Comparator; import java.util.EnumMap; import java.util.IdentityHashMap; -import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.Set; @@ -315,19 +314,6 @@ public final class ClientRigEquipmentHandler { Map newlyUnequipped = diffKeys(previousKeys, currentKeys); - // Mapping itemId → stack pour les entries nouvellement équipées : - // le callsite du one-shot / sound a besoin du stack concret (ou de la - // définition pour le oneshot, cf plus bas). Pour les unequip, le stack - // est parti — on passe directement l'itemId et on lookup la définition - // dans le registry (qui garde la def en cache). - Map itemIdToStack = new LinkedHashMap<>(); - for (Map.Entry entry : equipped.entrySet()) { - ResourceLocation id = extractItemId(entry.getValue(), DataDrivenItemRegistry::get); - if (id != null && !itemIdToStack.containsKey(id)) { - itemIdToStack.put(id, entry.getValue()); - } - } - // La capability dédupe déjà par identity (cf V2BondageEquipment.getAllEquipped, // IdentityHashMap seen+LinkedHashMap result) — on applique néanmoins notre // propre dédup défensive pour ne pas dépendre d'un invariant qu'un refactor @@ -541,11 +527,12 @@ public final class ClientRigEquipmentHandler { * (on_equip ou on_unequip) via l'{@code extractor}, et joue l'animation * via le {@code oneshotPlayer}. * - *

Utilisé à la place de {@link #triggerOneshot} quand on n'a plus le - * stack concret (cas unequip : le stack vient de quitter le slot). La - * {@link DataDrivenItemDefinition} reste accessible via - * {@link DataDrivenItemRegistry#get(ResourceLocation)} tant que le - * registry n'a pas été rechargé ({@code /reload}).

+ *

Utilisé pour les deux triggers equip / unequip depuis + * {@link #rebuildBondageAnimations} : à ce stade, on a un + * {@link ResourceLocation} itemId extrait du snapshot + * {@code (region → itemId)}. La {@link DataDrivenItemDefinition} reste + * accessible via {@link DataDrivenItemRegistry#get(ResourceLocation)} + * tant que le registry n'a pas été rechargé ({@code /reload}).

* *

No-op silencieux si :

*
    @@ -583,67 +570,6 @@ public final class ClientRigEquipmentHandler { oneshotPlayer.play(accessor, ONESHOT_TRANSITION_TIME); } - /** - * Trigger un one-shot animation pour un item donné via le callback - * {@code oneshotPlayer}. Extrait soit {@code onEquip} soit {@code onUnequip} - * de {@link AnimationBindings} via {@code extractor}. - * - *

    No-op silencieux si :

    - *
      - *
    • Le stack n'a pas de {@link DataDrivenItemDefinition} (item V2 legacy) ;
      • - *
    • La définition n'a pas d'{@link AnimationBindings} (animations==null) ;
      • - *
    • Le binding ne déclare pas de one-shot pour ce trigger (onEquip ou - * onUnequip == null dans le JSON).
      • - *
    - * - *

    Si un one-shot est déclaré, il est résolu via {@code animResolver} - * ({@link TiedUpAnimationRegistry#resolveWithFallback} en prod) et joué - * via {@code oneshotPlayer} avec {@link #ONESHOT_TRANSITION_TIME}.

    - * - *

    Priority forwarding : on NE passe PAS de priority override à - * {@link ClientAnimator#playAnimation(AssetAccessor, float)} (signature - * limitée). La priority vient de - * {@link StaticAnimation#getPriority()} intrinsèque (property JSON du - * fichier anim). Cohérent avec la décision P3-11 : - * {@code PacketPlayRigAnim.priorityOrdinal} reste informationnel / loggable, - * priority override est TODO Phase 4 si besoin.

    - * - *

    Package-private + générique {@code } pour unit test avec - * callbacks injectés et stacks {@link Object} dummy (la prod passe - * {@link ItemStack}).

    - * - * @param type de l'item (prod : {@link ItemStack}, - * tests : {@link Object} dummy) - * @param oneshotPlayer callback play(accessor, transitionTime) ; - * en prod {@code animator::playAnimation} - * @param stack item en cours de trigger ; null toléré (no-op) - * @param extractor fonction de sélection du trigger, typiquement - * {@code AnimationBindings::onEquip} ou {@code ::onUnequip} - * @param defResolver resolver stack → {@link DataDrivenItemDefinition} - * (null pour items V2 legacy) - * @param animResolver resolver {@link ResourceLocation} → accessor, - * ex. {@link TiedUpAnimationRegistry#resolveWithFallback} - */ - static void triggerOneshot( - OneshotPlayer oneshotPlayer, - T stack, - Function extractor, - Function defResolver, - Function> animResolver - ) { - if (stack == null) return; - DataDrivenItemDefinition def = defResolver.apply(stack); - if (def == null) return; - AnimationBindings bindings = def.animations(); - if (bindings == null) return; - ResourceLocation oneshotId = extractor.apply(bindings); - if (oneshotId == null) return; - - AssetAccessor accessor = animResolver.apply(oneshotId); - if (accessor == null) return; // defensive — resolveWithFallback ne renvoie jamais null en prod - oneshotPlayer.play(accessor, ONESHOT_TRANSITION_TIME); - } - /** * Joue le son equip/unequip vanilla au voisinage du player. Client-only * ({@link net.minecraft.world.level.Level#playLocalSound} n'émet aucun diff --git a/src/test/java/com/tiedup/remake/rig/patch/PlayerPatchUpdateMotionTest.java b/src/test/java/com/tiedup/remake/rig/patch/PlayerPatchUpdateMotionTest.java index 1918fc6..5bc4116 100644 --- a/src/test/java/com/tiedup/remake/rig/patch/PlayerPatchUpdateMotionTest.java +++ b/src/test/java/com/tiedup/remake/rig/patch/PlayerPatchUpdateMotionTest.java @@ -25,6 +25,16 @@ import com.tiedup.remake.rig.anim.TiedUpLivingMotions; *

    Le binding {@code Player → flags} dans {@code updateMotion} est un thin * wrapper trivial (un if de gate + 9 appels de getters) — couvert par les * tests in-game manuels de P3-05/06, pas par JUnit.

    + * + *

    Composite motion assignment (P3 cross-check fix) : {@code updateMotion} + * assigne {@code currentCompositeMotion = motion} après + * {@code currentLivingMotion = motion}. Ce write n'est PAS couvert ici — il + * exige un {@code Player} live (MC bootstrap + {@code BondageStateHelpers} + * static deps) que le module test n'a pas. La garantie vient de l'inspection + * du code + gameday QA : un overlay {@code COMPOSITE_LAYER} bindé sur + * {@link TiedUpLivingMotions#WALK_BOUND} doit fire quand le player marche + * bound (cuffs_arms_walk cas canon). Voir EF pattern + * {@code AbstractClientPlayerPatch:129-141}.

    */ class PlayerPatchUpdateMotionTest { diff --git a/src/test/java/com/tiedup/remake/v2/client/ClientRigEquipmentHandlerTest.java b/src/test/java/com/tiedup/remake/v2/client/ClientRigEquipmentHandlerTest.java index 8cc73d9..97503c3 100644 --- a/src/test/java/com/tiedup/remake/v2/client/ClientRigEquipmentHandlerTest.java +++ b/src/test/java/com/tiedup/remake/v2/client/ClientRigEquipmentHandlerTest.java @@ -57,8 +57,9 @@ import com.tiedup.remake.v2.bondage.movement.MovementStyle; *
  • {@link ClientRigEquipmentHandler#diffKeys} — diff key-based, * base du détection equip / unequip post-P3-07.
    • *
  • {@link ClientRigEquipmentHandler#triggerOneshotById} — fire - * one-shot à partir d'un itemId (cas unequip post-P3-07 où le - * stack est parti).
    • + * one-shot à partir d'un itemId (utilisé pour equip et unequip + * post-P3-07 : le callsite key-based ne manipule plus de stack + * concret). *
* *

Les ItemStack sont remplacés par des {@link Object} dummy : la logique @@ -935,59 +936,25 @@ class ClientRigEquipmentHandlerTest { "transition time doit etre 0.15s (ONESHOT_TRANSITION_TIME)"); } - // ========== P3-07 legacy : triggerOneshot (stack-based, still used for equip) ========== - - /** - * Stack sans {@link DataDrivenItemDefinition} (resolver → null) → skip - * silencieusement, aucun appel au oneshotPlayer. - */ - @Test - void triggerOneshot_noDefinition_skipsSilently() { - CapturingOneshotPlayer player = new CapturingOneshotPlayer(); - Object stack = new Object(); - - ClientRigEquipmentHandler.triggerOneshot( - player, - stack, - AnimationBindings::onEquip, - any -> null, // resolver retourne null : item V2 legacy - PASSTHROUGH_RESOLVER - ); - - assertTrue(player.calls.isEmpty(), - "stack sans definition → pas de trigger"); - } - - /** - * Définition dont {@code animations() == null} → skip silencieusement. - */ - @Test - void triggerOneshot_nullBindings_skipsSilently() { - CapturingOneshotPlayer player = new CapturingOneshotPlayer(); - Object stack = new Object(); - DataDrivenItemDefinition def = makeDef(ITEM_A, 10, /* animations */ null); - - ClientRigEquipmentHandler.triggerOneshot( - player, - stack, - AnimationBindings::onEquip, - any -> def, - PASSTHROUGH_RESOLVER - ); - - assertTrue(player.calls.isEmpty(), - "animations()==null → pas de trigger"); - } + // ========== P3-07 : triggerOneshotById extractor coverage (onEquip) ========== + // + // Note : les cas d'équivalence (null id, unknown id, null bindings, null + // stack) sont couverts par les tests triggerOneshotById_* ci-dessus sur + // l'extractor onUnequip. Les tests suivants fournissent la couverture + // symétrique pour l'extractor onEquip (null + happy path) — les autres + // branches du flow sont identiques (même implémentation, juste extractor + // différent). /** * Bindings présents mais {@code onEquip == null} → skip silencieusement. * Le cas dominant : 99% des items data-driven n'ont que des - * {@code living_motions}, pas de one-shots. + * {@code living_motions}, pas de one-shots. Pendant du test + * {@code triggerOneshotById_onUnequipNull_skipsSilently} pour l'extractor + * {@link AnimationBindings#onEquip}. */ @Test - void triggerOneshot_onEquipNull_skipsSilently() { + void triggerOneshotById_onEquipNull_skipsSilently() { CapturingOneshotPlayer player = new CapturingOneshotPlayer(); - Object stack = new Object(); AnimationBindings bindings = new AnimationBindings( Map.of(LivingMotions.WALK, ANIM_WALK_A), /* onEquip */ null, @@ -995,11 +962,11 @@ class ClientRigEquipmentHandlerTest { ); DataDrivenItemDefinition def = makeDef(ITEM_A, 10, bindings); - ClientRigEquipmentHandler.triggerOneshot( + ClientRigEquipmentHandler.triggerOneshotById( player, - stack, + ITEM_A, AnimationBindings::onEquip, - any -> def, + id -> def, PASSTHROUGH_RESOLVER ); @@ -1010,12 +977,13 @@ class ClientRigEquipmentHandlerTest { /** * Happy path — bindings avec {@code onEquip} défini → le oneshotPlayer * est appelé avec l'accessor résolu + la transition time canonique - * (0.15s, {@code ONESHOT_TRANSITION_TIME}). + * (0.15s, {@code ONESHOT_TRANSITION_TIME}). Pendant du test + * {@code triggerOneshotById_happyPath_callsPlayerWithResolvedAccessor} + * qui couvre {@link AnimationBindings#onUnequip}. */ @Test - void triggerOneshot_happyPath_callsPlayerWithResolvedAccessor() { + void triggerOneshotById_onEquipHappyPath_callsPlayerWithResolvedAccessor() { CapturingOneshotPlayer player = new CapturingOneshotPlayer(); - Object stack = new Object(); ResourceLocation oneshotId = ResourceLocation.fromNamespaceAndPath("tiedup", "cuffs_equip_oneshot"); @@ -1026,11 +994,11 @@ class ClientRigEquipmentHandlerTest { ); DataDrivenItemDefinition def = makeDef(ITEM_A, 10, bindings); - ClientRigEquipmentHandler.triggerOneshot( + ClientRigEquipmentHandler.triggerOneshotById( player, - stack, + ITEM_A, AnimationBindings::onEquip, - any -> def, + id -> def, PASSTHROUGH_RESOLVER ); @@ -1042,50 +1010,4 @@ class ClientRigEquipmentHandlerTest { 0.0001F, "transition time doit etre 0.15s (ONESHOT_TRANSITION_TIME)"); } - - /** - * Happy path côté {@code onUnequip} : même flow, mais l'extractor pointe - * sur {@link AnimationBindings#onUnequip}. - */ - @Test - void triggerOneshot_onUnequip_callsPlayerWithResolvedAccessor() { - CapturingOneshotPlayer player = new CapturingOneshotPlayer(); - Object stack = new Object(); - - ResourceLocation unequipId = - ResourceLocation.fromNamespaceAndPath("tiedup", "cuffs_unequip_oneshot"); - AnimationBindings bindings = new AnimationBindings( - Map.of(), - /* onEquip */ null, - /* onUnequip */ unequipId - ); - DataDrivenItemDefinition def = makeDef(ITEM_A, 10, bindings); - - ClientRigEquipmentHandler.triggerOneshot( - player, - stack, - AnimationBindings::onUnequip, - any -> def, - PASSTHROUGH_RESOLVER - ); - - assertEquals(1, player.calls.size()); - assertEquals(unequipId, player.calls.get(0).getKey()); - } - - /** Stack null est toléré — pas de NPE, pas d'appel player. */ - @Test - void triggerOneshot_nullStack_noThrow() { - CapturingOneshotPlayer player = new CapturingOneshotPlayer(); - assertDoesNotThrow(() -> - ClientRigEquipmentHandler.triggerOneshot( - player, - /* stack */ null, - AnimationBindings::onEquip, - any -> { throw new AssertionError("resolver ne doit pas etre appele pour null"); }, - PASSTHROUGH_RESOLVER - ) - ); - assertTrue(player.calls.isEmpty()); - } }