From 5428f13f98170f462b7966ed09ecdd9f96e53492 Mon Sep 17 00:00:00 2001 From: notevil Date: Fri, 24 Apr 2026 01:13:00 +0200 Subject: [PATCH] P3-07 review fixes : key-based diff + first-rebuild guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CRITICAL BUG-001 reviewer : identity-based diff triggered sound 2x per sync because V2BondageEquipment.deserializeNBT regenerates ItemStack instances. 19 call-sites (respawn, dialogue, lock toggle, whip) do full resync → every interaction = sound cacophony. Switch LAST_EQUIPPED from Set (identity) to Map keyed by (region, itemId). Same region+item = no change, ignored. HIGH RISK-001 reviewer : first rebuild per player had LAST_EQUIPPED empty → all items detected as newly equipped → sound spam at login (5 items = 5 sounds). Add FIRST_REBUILD_DONE WeakHashMap flag to skip oneshots+sounds on very first rebuild per player. MEDIUM SMELL-001/002 : javadoc of diffBy removed (assumed identity sharing which never happens). Padlock NBT patch detection promise removed — would require NBT hash in snapshot key (Phase 4). Tests refactored : key-based (region, itemId) semantics. --- .../v2/client/ClientRigEquipmentHandler.java | 393 +++++++++++------ .../client/ClientRigEquipmentHandlerTest.java | 413 +++++++++++++++--- 2 files changed, 613 insertions(+), 193 deletions(-) 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 ca4b2cb..16cf624 100644 --- a/src/main/java/com/tiedup/remake/v2/client/ClientRigEquipmentHandler.java +++ b/src/main/java/com/tiedup/remake/v2/client/ClientRigEquipmentHandler.java @@ -7,12 +7,13 @@ package com.tiedup.remake.v2.client; import java.util.ArrayList; 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; import java.util.WeakHashMap; -import java.util.function.Consumer; import java.util.function.Function; import net.minecraft.resources.ResourceLocation; @@ -102,7 +103,7 @@ import com.tiedup.remake.v2.bondage.datadriven.DataDrivenItemRegistry; * *

L'entrée publique {@link #rebuildBondageAnimations(Player)} est * side-gated mais non testable sans MC runtime (player réel, capability - * réelle, animator réel). La logique pure est extraite dans deux helpers + * réelle, animator réel). La logique pure est extraite dans plusieurs helpers * package-private :

*
    *
  • {@link #extractSortedDefinitions} — dedup identity + filter + @@ -111,6 +112,12 @@ import com.tiedup.remake.v2.bondage.datadriven.DataDrivenItemRegistry; * callbacks injectables ({@link Runnable} + * {@link LivingAnimationAdder}). Testable avec un fake adder qui * capture les appels dans une list.
    • + *
  • {@link #buildKeySnapshot} — construit le snapshot stable + * {@code (region → itemId)} utilisé pour le diff tick-to-tick + * résistant aux re-sync réseau.
    • + *
  • {@link #diffKeys} — calcule newly-entered entries d'une map + * {@code a \ b} par comparaison (key, value) — base de détection + * equip / unequip.
    • *
* * @see AnimationBindings @@ -154,29 +161,58 @@ public final class ClientRigEquipmentHandler { } /** - * Snapshot du set d'items équipés par player lors du dernier - * {@link #rebuildBondageAnimations} — utilisé pour calculer les diffs - * {@code newlyEquipped} et {@code newlyUnequipped} tick-to-tick afin de - * déclencher les one-shots {@link AnimationBindings#onEquip} / + * Snapshot stable {@code (region → itemId)} — utilisé pour calculer les + * diffs {@code newlyEquipped} / {@code newlyUnequipped} tick-to-tick afin + * de déclencher les one-shots {@link AnimationBindings#onEquip} / * {@link AnimationBindings#onUnequip}. * + *

Pourquoi key-based et non identity-based : le packet + * {@code PacketSyncV2Equipment} appelle {@code V2BondageEquipment.deserializeNBT} + * qui recrée systématiquement de nouvelles instances {@link ItemStack} via + * {@code ItemStack.of(stackTag)}. Les {@literal ~}19 call sites server-side + * qui appellent {@code V2EquipmentHelper.sync()} (respawn, dialogue, + * struggle, lock toggle, whip, etc.) produisent chacun un full resync, donc + * un diff basé sur l'identité des stacks verrait tous les items comme + * "nouvellement équipés" + "nouvellement déséquipés" à chaque sync — + * double-fire du son à chaque interaction (BUG-001 reviewer P3-07).

+ * + *

Le snapshot est maintenant keyé par {@code (BodyRegionV2, itemId)} + * où {@code itemId} est extrait via + * {@link DataDrivenItemRegistry#get(ItemStack)} → {@code def.id()}. Tant + * que le même item reste au même slot, le diff est vide même si + * l'{@link ItemStack} est une nouvelle instance après re-sync.

+ * + *

Limitation connue : une mutation NBT silencieuse (ex. padlock + * posé sur des cuffs existants qui ne change pas {@code tiedup_item_id}) + * ne déclenche PAS de diff. Si un tel usage devient critical, hash du + * NBT à inclure dans la key — TODO Phase 4.

+ * *

WeakHashMap : key = {@link Player}, weak ref → l'entrée est * GC'd automatiquement quand le player se déconnecte et n'est plus * référencé nulle part ailleurs. Évite la fuite mémoire sur serveur * intégré à la longue.

* - *

Semantics du set : identity-based (via - * {@link Collections#newSetFromMap(Map)} + {@link IdentityHashMap}). Deux - * {@link ItemStack} avec NBT identique mais instances distinctes sont - * traités comme différents items — convention V2 (un stack = une - * occurrence équipée). Permet de détecter {@code split stack} / - * {@code re-equip} même si le nouveau stack est content-equals à l'ancien.

- * *

Thread-safety : lu/écrit exclusivement depuis le client main * thread (même contrat que {@link #rebuildBondageAnimations}). Pas de * synchronisation nécessaire.

*/ - private static final WeakHashMap> LAST_EQUIPPED = + private static final WeakHashMap> + LAST_EQUIPPED_KEYS = new WeakHashMap<>(); + + /** + * Flag par-player indiquant que {@link #rebuildBondageAnimations} a déjà + * tourné une fois pour ce player. Skip des sounds + one-shots sur le + * premier rebuild — sinon, à chaque login / join, tous les items équipés + * seraient vus comme "nouveaux" (previous={}) et fireraient le son en + * cascade (RISK-001 reviewer P3-07 : 5 items = 5 sons cuir simultanés + * à l'arrivée, désagréable auditivement). + * + *

WeakHashMap : même justification que + * {@link #LAST_EQUIPPED_KEYS} — GC'd quand le player part. Un reconnect + * retraite donc comme un "premier rebuild" (prévu : on ne veut pas spammer + * le son au retour).

+ */ + private static final WeakHashMap FIRST_REBUILD_DONE = new WeakHashMap<>(); /** @@ -248,42 +284,37 @@ public final class ClientRigEquipmentHandler { Map equipped = V2EquipmentHelper.getAllEquipped(player); - // === P3-07 : diff detection tick-to-tick pour on_equip / on_unequip === + // === P3-07 : diff detection key-based résistant aux re-sync réseau === // - // On snapshot l'ensemble des items équipés avant le rebuild + on compare - // au dernier snapshot pour ce player. Semantic identity (IdentityHashMap) : - // un stack content-equals mais instance différente compte comme un - // nouvel item équipé (convention V2, un stack = une occurrence). - Set currentUnique = uniqueByIdentity(equipped.values()); - Set previousUnique = - LAST_EQUIPPED.getOrDefault(player, Collections.emptySet()); + // On keye par (region, itemId) plutôt que par identity d'ItemStack : + // V2BondageEquipment.deserializeNBT recrée systématiquement de nouvelles + // instances à chaque packet, donc une comparaison identity verrait tous + // les items comme "nouveaux" à chaque full-resync (dialogue, respawn, + // lock toggle, etc.) → son dédoublé. Key-based : même itemId au même + // slot → pas de change, silence, quelle que soit l'instance. + Map currentKeys = + buildKeySnapshot(equipped, DataDrivenItemRegistry::get); + Map previousKeys = + LAST_EQUIPPED_KEYS.getOrDefault(player, Map.of()); - Set newlyEquipped = diffBy(currentUnique, previousUnique); - Set newlyUnequipped = diffBy(previousUnique, currentUnique); + Map newlyEquipped = + diffKeys(currentKeys, previousKeys); + Map newlyUnequipped = + diffKeys(previousKeys, currentKeys); - // Unequip AVANT rebuild — l'anim one-shot joue sur "l'ancien état" du - // 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. - OneshotPlayer oneshotPlayer = animator::playAnimation; - Consumer soundPlayer = stack -> playEquipSound(player, stack); - - for (ItemStack stack : newlyUnequipped) { - triggerOneshot( - oneshotPlayer, - stack, - AnimationBindings::onUnequip, - DataDrivenItemRegistry::get, - TiedUpAnimationRegistry::resolveWithFallback - ); - soundPlayer.accept(stack); + // 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()); + } } - // 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.put(player, currentUnique); - // 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 @@ -291,6 +322,37 @@ public final class ClientRigEquipmentHandler { List sortedDefs = extractSortedDefinitions(equipped.values(), DataDrivenItemRegistry::get); + // Suppress sounds + one-shots sur le PREMIER rebuild par player : + // previousKeys sera empty au login → sans garde, tous les items seraient + // vus comme newly equipped → 5 items = 5 sons simultanés à l'arrivée + // (RISK-001 reviewer). Le flag FIRST_REBUILD_DONE évite ce spam. + boolean isFirstRebuild = !FIRST_REBUILD_DONE.containsKey(player); + + OneshotPlayer oneshotPlayer = animator::playAnimation; + + if (!isFirstRebuild) { + // Unequip AVANT rebuild — l'anim one-shot joue sur "l'ancien état" du + // 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. + for (Map.Entry entry : newlyUnequipped.entrySet()) { + ResourceLocation itemId = entry.getValue(); + triggerOneshotById( + oneshotPlayer, + itemId, + AnimationBindings::onUnequip, + DataDrivenItemRegistry::get, + TiedUpAnimationRegistry::resolveWithFallback + ); + playEquipSound(player); + } + } + + // 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). @@ -309,100 +371,185 @@ public final class ClientRigEquipmentHandler { TiedUpAnimationRegistry::resolveWithFallback ); - // 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é - // poussés dans livingAnimations. Cohérent avec l'intention visuelle : - // on voit l'item se mettre en place puis le one-shot l'enrichit. - for (ItemStack stack : newlyEquipped) { - triggerOneshot( - oneshotPlayer, - stack, - AnimationBindings::onEquip, - DataDrivenItemRegistry::get, - TiedUpAnimationRegistry::resolveWithFallback - ); - soundPlayer.accept(stack); + 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é + // poussés dans livingAnimations. Cohérent avec l'intention visuelle : + // on voit l'item se mettre en place puis le one-shot l'enrichit. + for (Map.Entry entry : newlyEquipped.entrySet()) { + ResourceLocation itemId = entry.getValue(); + triggerOneshotById( + oneshotPlayer, + itemId, + AnimationBindings::onEquip, + DataDrivenItemRegistry::get, + TiedUpAnimationRegistry::resolveWithFallback + ); + playEquipSound(player); + } } + FIRST_REBUILD_DONE.put(player, Boolean.TRUE); + TiedUpRigConstants.LOGGER.debug( "[ClientRigEquipmentHandler] Rebuilt livingAnimations for player {} " - + "({} data-driven items processed, {} equip / {} unequip one-shots)", + + "({} data-driven items processed, {} equip / {} unequip one-shots," + + " firstRebuild={})", player.getName().getString(), sortedDefs.size(), newlyEquipped.size(), - newlyUnequipped.size() + newlyUnequipped.size(), + isFirstRebuild ); } /** - * Collecte un {@link Set} identity-based à partir d'un iterable. Deux - * éléments {@code equals()} mais avec instances distinctes restent - * distincts. Null skippés. + * Extrait l'identifiant data-driven ({@link DataDrivenItemDefinition#id()}) + * d'un {@link ItemStack}. Retourne {@code null} pour les stacks + * vides, legacy ou sans definition. * - *

Encapsule le pattern {@code Collections.newSetFromMap(new IdentityHashMap<>())} - * + filter null pour la réutilisation dans {@link #rebuildBondageAnimations} - * et les tests.

+ *

Package-private + resolver injecté pour permettre un unit test sans + * bootstrap MC (test : {@code Object} stack + resolver custom ; prod : + * {@link ItemStack} + {@link DataDrivenItemRegistry#get(ItemStack)}).

* - *

Générique : prod l'utilise avec {@link ItemStack}, tests avec - * {@link Object} dummy (aucune méthode d'instance appelée — seule - * l'identité est utilisée).

- * - * @param type des éléments - * @param items iterable potentiellement avec duplicats identity + nulls - * @return nouveau set identity-based, ordre non garanti + * @param type de l'item (prod : {@link ItemStack}, tests : dummy) + * @param stack le stack à inspecter, null toléré + * @param resolver resolver {@code stack → DataDrivenItemDefinition} (null pour legacy) + * @return l'itemId, ou null si non data-driven */ - static Set uniqueByIdentity(Iterable items) { - Set out = Collections.newSetFromMap(new IdentityHashMap<>()); - for (T item : items) { - if (item == null) continue; - out.add(item); + static ResourceLocation extractItemId( + T stack, + Function resolver + ) { + if (stack == null) return null; + DataDrivenItemDefinition def = resolver.apply(stack); + return def != null ? def.id() : null; + } + + /** + * Construit un snapshot stable {@code (BodyRegionV2 → itemId)} à partir + * d'un {@code Map}. Les slots dont le stack n'est + * pas data-driven (resolver → null) sont omis. + * + *

Ce snapshot est la base du diff key-based : deux snapshots pris à + * deux instants différents sont comparés par {@code (region, itemId)}. Un + * resync réseau qui régénère les instances {@link ItemStack} produit le + * MÊME snapshot tant que les items occupant les slots restent les mêmes + * par itemId — d'où l'immunité au double-fire du son P3-07.

+ * + *

Package-private + generic + resolver injecté → testable sans bootstrap + * MC.

+ * + * @param type de l'item (prod : {@link ItemStack}, tests : dummy) + * @param equipped map du callsite ({@link V2EquipmentHelper#getAllEquipped} + * en prod) + * @param resolver resolver {@code stack → DataDrivenItemDefinition} + * @return map {@code (region → itemId)} possiblement vide, never null + */ + static Map buildKeySnapshot( + Map equipped, + Function resolver + ) { + Map out = new EnumMap<>(BodyRegionV2.class); + for (Map.Entry entry : equipped.entrySet()) { + ResourceLocation id = extractItemId(entry.getValue(), resolver); + if (id != null) out.put(entry.getKey(), id); } return out; } /** - * Retourne les éléments de {@code a} qui ne sont pas (par identité objet, - * {@code ==}) dans {@code b}. Utilisé pour calculer {@code newlyEquipped} - * = current - previous et {@code newlyUnequipped} = previous - current. + * Calcule les entrées de {@code a} qui n'existent pas à l'identique + * dans {@code b}, keyées {@code (region, itemId)}. Utilisé pour détecter + * {@code newlyEquipped = current \ previous} et + * {@code newlyUnequipped = previous \ current}. * - *

Identity semantics (pas {@link Object#equals}) : deux éléments - * {@code equals()} mais références distinctes sont considérés comme - * différents. Volontaire — convention V2 : un stack = une - * occurrence équipée. Permet de détecter correctement :

+ *

Sémantique : une entrée {@code (region, itemId)} de {@code a} est + * incluse dans le diff si {@code b} :

*
    - *
  • Un re-equip où le joueur déséquipe puis rééquipe le même item - * (le stack est potentiellement la même instance Minecraft ; - * dans ce cas pas de one-shot, ok).
    • - *
  • Un remplacement par un item visuellement identique mais stack - * différent (nouveau padlock, NBT patch) → on-shot fire, correct.
    • + *
  • ne contient pas la {@code region}, OU
    • + *
  • contient la {@code region} mais avec un {@code itemId} différent + * (via {@link Object#equals}).
    • *
* - *

Ordre de sortie : suit l'ordre d'itération de {@code a} (pour - * {@link IdentityHashMap} non-déterministe mais stable dans un même run). - * Suffisant pour le use case : les one-shots jouent indépendamment sur - * des layers distinctes, l'ordre ne change pas le rendu.

+ *

Ainsi :

+ *
    + *
  • {@code previous={ARMS→cuffs}, current={ARMS→cuffs}} → + * {@code diffKeys(current, previous) = {}} — pas de change malgré + * ItemStack réinstancié par {@code deserializeNBT}.
    • + *
  • {@code previous={ARMS→cuffs}, current={ARMS→shackles}} → + * {@code diffKeys(current, previous) = {ARMS→shackles}} — change + * détecté.
    • + *
* - * @param type des éléments - * @param a set d'origine - * @param b set à soustraire (identity-based) - * @return nouveau set {@code a \ b} (identity difference) + *

Package-private → testable.

+ * + * @param a la map source + * @param b la map à soustraire + * @return nouvelle map, entrées de {@code a} absentes de {@code b} */ - static Set diffBy(Set a, Set b) { - // IdentityHashMap-based set pour respecter la semantic identity. - Set out = Collections.newSetFromMap(new IdentityHashMap<>()); - for (T x : a) { - boolean found = false; - for (T y : b) { - if (x == y) { - found = true; - break; - } + static Map diffKeys( + Map a, + Map b + ) { + Map out = new EnumMap<>(BodyRegionV2.class); + for (Map.Entry entry : a.entrySet()) { + ResourceLocation bVal = b.get(entry.getKey()); + if (bVal == null || !bVal.equals(entry.getValue())) { + out.put(entry.getKey(), entry.getValue()); } - if (!found) out.add(x); } return out; } + /** + * Trigger un one-shot pour un {@code itemId} donné. Resolve la + * {@link DataDrivenItemDefinition} via le registry, extrait le trigger + * (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}).

+ * + *

No-op silencieux si :

+ *
    + *
  • L'{@code itemId} est inconnu du registry (item supprimé / mod + * unloaded) ;
    • + *
  • La définition n'a pas d'{@link AnimationBindings} ;
    • + *
  • Le binding ne déclare pas de one-shot pour ce trigger.
    • + *
+ * + *

Package-private + resolvers injectés → testable unit sans bootstrap.

+ * + * @param oneshotPlayer callback play(accessor, transitionTime) + * @param itemId identifiant de l'item ; null toléré (no-op) + * @param extractor fonction de sélection du trigger ({@code onEquip} ou {@code onUnequip}) + * @param defResolver resolver {@code itemId → DataDrivenItemDefinition} (null = inconnu) + * @param animResolver resolver {@code animId → AssetAccessor} + */ + static void triggerOneshotById( + OneshotPlayer oneshotPlayer, + ResourceLocation itemId, + Function extractor, + Function defResolver, + Function> animResolver + ) { + if (itemId == null) return; + DataDrivenItemDefinition def = defResolver.apply(itemId); + 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; + 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} @@ -465,25 +612,25 @@ public final class ClientRigEquipmentHandler { } /** - * Joue le son equip/unequip vanilla pour un stack donné au voisinage du - * player. Client-only ({@link net.minecraft.world.level.Level#playLocalSound} - * n'émet aucun paquet réseau). + * Joue le son equip/unequip vanilla au voisinage du player. Client-only + * ({@link net.minecraft.world.level.Level#playLocalSound} n'émet aucun + * paquet réseau). * - *

MVP : son {@link #DEFAULT_EQUIP_SOUND} pour tous les items. - * Future : lire un éventuel champ {@code equip_sound} dans - * {@link DataDrivenItemDefinition} pour customiser per-item.

+ *

MVP : son {@link #defaultEquipSound} pour tous les items. Le son + * n'a plus besoin du stack ({@code ItemStack}) en entrée depuis le switch + * key-based : le callsite unequip ne possède plus le stack (disparu), et + * le son vanilla choisi ne varie pas per-item. Future évolution : lire un + * champ {@code equip_sound} dans {@link DataDrivenItemDefinition} via + * l'itemId + registry lookup, sans besoin du stack concret.

* *

Non testable unit (nécessite {@link Player} live + level) — skip * dans la suite de tests.

* * @param player le player source du son (position utilisée pour la * spatialisation 3D) - * @param stack l'item en cours d'equip/unequip ; jamais null quand appelé - * depuis {@link #rebuildBondageAnimations} mais on check - * défensivement */ - private static void playEquipSound(Player player, ItemStack stack) { - if (player == null || stack == null || stack.isEmpty()) return; + private static void playEquipSound(Player player) { + if (player == null) return; player.level().playLocalSound( player.getX(), player.getY(), @@ -497,7 +644,8 @@ public final class ClientRigEquipmentHandler { } /** - * Vide le cache de snapshots {@link #LAST_EQUIPPED}. Utilisé dans : + * Vide les caches de snapshots {@link #LAST_EQUIPPED_KEYS} et + * {@link #FIRST_REBUILD_DONE}. Utilisé dans : *
    *
  • Tests unitaires — pour isoler les cas entre tests (sinon un test * qui popule le cache pollue les suivants via l'état statique).
    • @@ -509,7 +657,8 @@ public final class ClientRigEquipmentHandler { *

    Package-private — pas destiné à la prod.

    */ static void clearEquipmentSnapshotsForTesting() { - LAST_EQUIPPED.clear(); + LAST_EQUIPPED_KEYS.clear(); + FIRST_REBUILD_DONE.clear(); } /** 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 2e0dea8..8cc73d9 100644 --- a/src/test/java/com/tiedup/remake/v2/client/ClientRigEquipmentHandlerTest.java +++ b/src/test/java/com/tiedup/remake/v2/client/ClientRigEquipmentHandlerTest.java @@ -15,6 +15,7 @@ import static org.junit.jupiter.api.Assertions.assertTrue; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; +import java.util.EnumMap; import java.util.IdentityHashMap; import java.util.LinkedHashMap; import java.util.List; @@ -39,25 +40,32 @@ import com.tiedup.remake.v2.bondage.movement.MovementModifier; import com.tiedup.remake.v2.bondage.movement.MovementStyle; /** - * Tests unitaires pour {@link ClientRigEquipmentHandler} (P3-05). + * Tests unitaires pour {@link ClientRigEquipmentHandler} (P3-05 + P3-07). * *

    Stratégie de test

    *

    L'entrée publique {@link ClientRigEquipmentHandler#rebuildBondageAnimations} * nécessite un {@code Player} réel + capability MC — non testable sans * bootstrap. On vérifie uniquement son null-safety. La logique métier est - * extraite dans deux méthodes package-private pures :

    + * extraite dans plusieurs méthodes package-private pures :

    *
      *
    • {@link ClientRigEquipmentHandler#extractSortedDefinitions} — * dedup identity, filter null, sort by posePriority ASC.
      • *
    • {@link ClientRigEquipmentHandler#applyDefinitions} — itère + * adder pour chaque binding non-null/non-vide, reset d'abord.
      • + *
    • {@link ClientRigEquipmentHandler#buildKeySnapshot} — snapshot + * stable {@code (region → itemId)} résistant aux re-sync.
      • + *
    • {@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).
      • *
    * *

    Les ItemStack sont remplacés par des {@link Object} dummy : la logique * ne lit aucune méthode d'instance, seule l'identité objet compte pour la - * dédup. Les {@link DataDrivenItemDefinition} sont construites via le - * {@link #makeDef} helper avec des defaults neutres (seuls posePriority + - * animations comptent pour ces tests).

    + * dédup ou la key (itemId) pour le diff. Les {@link DataDrivenItemDefinition} + * sont construites via le {@link #makeDef} helper avec des defaults neutres + * (seuls posePriority + animations comptent pour ces tests).

    */ class ClientRigEquipmentHandlerTest { @@ -553,101 +561,260 @@ class ClientRigEquipmentHandlerTest { assertFalse(def.occupiedRegions().isEmpty()); } - // ========== P3-07 : diffBy identity semantics ========== + // ========== P3-07 review : extractItemId ========== - /** Diff de deux sets vides → set vide, no throw. */ + /** Stack {@code null} → id {@code null}, no throw. */ @Test - void diffBy_emptySets_returnsEmpty() { - Set result = ClientRigEquipmentHandler.diffBy(Set.of(), Set.of()); - assertNotNull(result); - assertTrue(result.isEmpty()); + void extractItemId_nullStack_returnsNull() { + ResourceLocation id = ClientRigEquipmentHandler.extractItemId( + (Object) null, + stack -> { throw new AssertionError("resolver ne doit pas etre appele"); } + ); + assertNull(id); + } + + /** Item legacy (resolver → null) → id {@code null}. */ + @Test + void extractItemId_nonDataDrivenStack_returnsNull() { + Object stack = new Object(); + ResourceLocation id = ClientRigEquipmentHandler.extractItemId( + stack, + any -> null + ); + assertNull(id); + } + + /** Stack data-driven → id extrait de la définition. */ + @Test + void extractItemId_dataDrivenStack_returnsDefinitionId() { + Object stack = new Object(); + DataDrivenItemDefinition def = makeDef(ITEM_A, 10, null); + ResourceLocation id = ClientRigEquipmentHandler.extractItemId( + stack, + any -> def + ); + assertEquals(ITEM_A, id); + } + + // ========== P3-07 review : buildKeySnapshot ========== + + /** Map vide → snapshot vide. */ + @Test + void buildKeySnapshot_emptyMap_returnsEmpty() { + Map snap = + ClientRigEquipmentHandler.buildKeySnapshot( + Collections.emptyMap(), + stack -> { throw new AssertionError("resolver ne doit pas etre appele"); } + ); + assertNotNull(snap); + assertTrue(snap.isEmpty()); } /** - * Un item présent dans {@code a} mais absent de {@code b} apparaît dans le - * diff. Cas {@code newlyEquipped = current \ previous}. + * Map {@code (region → stack)} → {@code (region → itemId)} pour les + * stacks data-driven. Les entrées dont le stack n'est pas data-driven + * (resolver → null) sont omises du snapshot. */ @Test - void diffBy_itemOnlyInA_appearsInResult() { - Object item1 = new Object(); - Object item2 = new Object(); + void buildKeySnapshot_extractsRegionToItemIdMap() { + Object stackArms = new Object(); + Object stackNeck = new Object(); + DataDrivenItemDefinition defArms = makeDef(ITEM_A, 10, null); + DataDrivenItemDefinition defNeck = makeDef(ITEM_B, 10, null); - Set a = Collections.newSetFromMap(new IdentityHashMap<>()); - a.add(item1); - a.add(item2); - Set b = Collections.newSetFromMap(new IdentityHashMap<>()); - b.add(item1); + IdentityHashMap resolverMap = new IdentityHashMap<>(); + resolverMap.put(stackArms, defArms); + resolverMap.put(stackNeck, defNeck); - Set result = ClientRigEquipmentHandler.diffBy(a, b); + Map equipped = new EnumMap<>(BodyRegionV2.class); + equipped.put(BodyRegionV2.ARMS, stackArms); + equipped.put(BodyRegionV2.NECK, stackNeck); - assertEquals(1, result.size(), "item2 nouveau doit etre detecte"); - assertTrue(result.contains(item2)); - assertFalse(result.contains(item1), "item1 present dans les deux ne doit PAS etre dans le diff"); + Map snap = + ClientRigEquipmentHandler.buildKeySnapshot(equipped, resolverMap::get); + + assertEquals(2, snap.size()); + assertEquals(ITEM_A, snap.get(BodyRegionV2.ARMS)); + assertEquals(ITEM_B, snap.get(BodyRegionV2.NECK)); } /** - * Deux instances {@link Object#equals equals} mais identity-different → - * considérées comme distinctes. Critical pour la convention "un stack = - * une occurrence". + * Les stacks non data-driven (legacy V2, resolver → null) ne sont pas + * dans le snapshot — ainsi le diff ne les considère jamais, et aucun + * sound ne joue pour leur equip/unequip (cohérent avec l'usage : les + * items sans JSON n'ont aucun binding, pas d'intérêt à sounder). */ @Test - void diffBy_identitySemantics_equalsObjectsAreDistinct() { - // Deux String content-equal mais avec new String(...) pour garantir - // des instances distinctes (String pool interning contourné). - String a1 = new String("same"); - String b1 = new String("same"); - assertTrue(a1.equals(b1)); - assertFalse(a1 == b1, "guard : les deux Strings doivent etre des instances distinctes"); + void buildKeySnapshot_skipsNonDataDrivenStacks() { + Object stackArms = new Object(); + Object stackLegs = new Object(); // non data-driven + DataDrivenItemDefinition defArms = makeDef(ITEM_A, 10, null); - Set setA = Collections.newSetFromMap(new IdentityHashMap<>()); - setA.add(a1); - Set setB = Collections.newSetFromMap(new IdentityHashMap<>()); - setB.add(b1); + IdentityHashMap resolverMap = new IdentityHashMap<>(); + resolverMap.put(stackArms, defArms); + // stackLegs n'est pas dans le resolver → resolver.apply(stackLegs) == null - Set result = ClientRigEquipmentHandler.diffBy(setA, setB); + Map equipped = new EnumMap<>(BodyRegionV2.class); + equipped.put(BodyRegionV2.ARMS, stackArms); + equipped.put(BodyRegionV2.LEGS, stackLegs); - assertEquals(1, result.size(), - "deux strings equals() mais != doivent etre traites comme distincts (identity)"); - assertSame(a1, result.iterator().next(), - "le diff doit retenir l'instance de setA"); + Map snap = + ClientRigEquipmentHandler.buildKeySnapshot(equipped, resolverMap::get); + + assertEquals(1, snap.size(), "stack legacy doit etre skip"); + assertEquals(ITEM_A, snap.get(BodyRegionV2.ARMS)); + assertFalse(snap.containsKey(BodyRegionV2.LEGS), + "LEGS ne doit pas etre dans le snapshot (item non data-driven)"); } - /** Quand a == b (mêmes instances), diff vide. */ + // ========== P3-07 review : diffKeys ========== + + /** Deux maps vides → diff vide. */ @Test - void diffBy_identicalSets_returnsEmpty() { - Object item = new Object(); - Set a = Collections.newSetFromMap(new IdentityHashMap<>()); - a.add(item); - Set b = Collections.newSetFromMap(new IdentityHashMap<>()); - b.add(item); - - Set result = ClientRigEquipmentHandler.diffBy(a, b); - assertTrue(result.isEmpty(), "meme instance dans les deux → diff vide"); + void diffKeys_emptyMaps_returnsEmpty() { + Map diff = + ClientRigEquipmentHandler.diffKeys(Map.of(), Map.of()); + assertNotNull(diff); + assertTrue(diff.isEmpty()); } - // ========== P3-07 : uniqueByIdentity ========== - - /** Iterable avec duplicats identity → un seul élément retenu. */ + /** + * previous={ARMS→cuffs}, current={ARMS→cuffs, NECK→collar} → + * {@code diffKeys(current, previous)} = {NECK→collar} — nouvelle entrée + * détectée. + */ @Test - void uniqueByIdentity_duplicateIdentity_dedups() { - Object item = new Object(); - Set result = - ClientRigEquipmentHandler.uniqueByIdentity(List.of(item, item, item)); - assertEquals(1, result.size()); - assertSame(item, result.iterator().next()); + void diffKeys_newEntry_returnsNewlyEquipped() { + Map previous = new EnumMap<>(BodyRegionV2.class); + previous.put(BodyRegionV2.ARMS, ITEM_A); + + Map current = new EnumMap<>(BodyRegionV2.class); + current.put(BodyRegionV2.ARMS, ITEM_A); + current.put(BodyRegionV2.NECK, ITEM_B); + + Map diff = + ClientRigEquipmentHandler.diffKeys(current, previous); + + assertEquals(1, diff.size()); + assertEquals(ITEM_B, diff.get(BodyRegionV2.NECK)); + assertFalse(diff.containsKey(BodyRegionV2.ARMS), + "ARMS inchangé ne doit PAS etre dans le diff"); } - /** Les null de l'iterable sont skippés silencieusement. */ + /** + * previous={ARMS→cuffs, NECK→collar}, current={ARMS→cuffs} → + * {@code diffKeys(previous, current)} = {NECK→collar} — entrée + * déséquipée détectée. + */ @Test - void uniqueByIdentity_nullsSkipped() { - Object item = new Object(); - List input = Arrays.asList(null, item, null); - Set result = ClientRigEquipmentHandler.uniqueByIdentity(input); - assertEquals(1, result.size()); - assertSame(item, result.iterator().next()); + void diffKeys_removedEntry_returnsNewlyUnequipped() { + Map previous = new EnumMap<>(BodyRegionV2.class); + previous.put(BodyRegionV2.ARMS, ITEM_A); + previous.put(BodyRegionV2.NECK, ITEM_B); + + Map current = new EnumMap<>(BodyRegionV2.class); + current.put(BodyRegionV2.ARMS, ITEM_A); + + Map diff = + ClientRigEquipmentHandler.diffKeys(previous, current); + + assertEquals(1, diff.size()); + assertEquals(ITEM_B, diff.get(BodyRegionV2.NECK)); } - // ========== P3-07 : triggerOneshot ========== + /** + * Remplacement in-place sur le même slot : previous={ARMS→cuffs}, + * current={ARMS→shackles} → (a) diff current\previous = {ARMS→shackles}, + * (b) diff previous\current = {ARMS→cuffs}. Les deux sens retournent + * l'entrée différente. + */ + @Test + void diffKeys_sameRegionDifferentItem_returnsBoth() { + Map previous = new EnumMap<>(BodyRegionV2.class); + previous.put(BodyRegionV2.ARMS, ITEM_A); + + Map current = new EnumMap<>(BodyRegionV2.class); + current.put(BodyRegionV2.ARMS, ITEM_B); + + Map newlyEquipped = + ClientRigEquipmentHandler.diffKeys(current, previous); + Map newlyUnequipped = + ClientRigEquipmentHandler.diffKeys(previous, current); + + assertEquals(1, newlyEquipped.size()); + assertEquals(ITEM_B, newlyEquipped.get(BodyRegionV2.ARMS), + "remplacement : shackles est newly equipped sur ARMS"); + + assertEquals(1, newlyUnequipped.size()); + assertEquals(ITEM_A, newlyUnequipped.get(BodyRegionV2.ARMS), + "remplacement : cuffs est newly unequipped sur ARMS"); + } + + /** + * FIX PRINCIPAL P3-07 (BUG-001) — previous={ARMS→cuffs}, + * current={ARMS→cuffs} avec des instances {@link ResourceLocation} + * distinctes (scénario {@link net.minecraft.world.item.ItemStack#of} + * qui recrée une ItemStack après deserializeNBT). Le diff doit être + * vide car les itemIds sont {@code equals()} même si les ItemStacks + * sous-jacents étaient différents. Sans ce comportement, chaque + * {@code V2EquipmentHelper.sync()} fire un son à chaque item — 19 + * call-sites server-side au total. + */ + @Test + void diffKeys_sameRegionSameItem_noChange() { + // ResourceLocation instances distinctes mais content-equal + ResourceLocation prevId = + ResourceLocation.fromNamespaceAndPath("tiedup", "cuffs"); + ResourceLocation currId = + ResourceLocation.fromNamespaceAndPath("tiedup", "cuffs"); + assertEquals(prevId, currId, "sanity : les deux ResourceLocation sont equals"); + // Note: Forge peut intern-er les ResourceLocation, donc == peut aussi + // etre true. L'important est que equals() l'est (garanti par le + // contract de ResourceLocation). + + Map previous = new EnumMap<>(BodyRegionV2.class); + previous.put(BodyRegionV2.ARMS, prevId); + + Map current = new EnumMap<>(BodyRegionV2.class); + current.put(BodyRegionV2.ARMS, currId); + + Map newlyEquipped = + ClientRigEquipmentHandler.diffKeys(current, previous); + Map newlyUnequipped = + ClientRigEquipmentHandler.diffKeys(previous, current); + + assertTrue(newlyEquipped.isEmpty(), + "meme item sur meme region → pas de newly equipped (BUG-001 fix)"); + assertTrue(newlyUnequipped.isEmpty(), + "meme item sur meme region → pas de newly unequipped (BUG-001 fix)"); + } + + /** + * Cas bizarre mais possible : un même itemId apparaît dans deux régions + * différentes entre previous et current (ex. un item multi-region dont + * le set d'occupied regions change après refactor data-pack). Le diff + * doit traiter chaque region indépendamment. + */ + @Test + void diffKeys_sameItemDifferentRegion_treatsAsMovement() { + Map previous = new EnumMap<>(BodyRegionV2.class); + previous.put(BodyRegionV2.ARMS, ITEM_A); + + Map current = new EnumMap<>(BodyRegionV2.class); + current.put(BodyRegionV2.LEGS, ITEM_A); + + Map newlyEquipped = + ClientRigEquipmentHandler.diffKeys(current, previous); + Map newlyUnequipped = + ClientRigEquipmentHandler.diffKeys(previous, current); + + assertEquals(1, newlyEquipped.size()); + assertEquals(ITEM_A, newlyEquipped.get(BodyRegionV2.LEGS)); + assertEquals(1, newlyUnequipped.size()); + assertEquals(ITEM_A, newlyUnequipped.get(BodyRegionV2.ARMS)); + } + + // ========== P3-07 review : triggerOneshotById ========== /** * Fake {@link ClientRigEquipmentHandler.OneshotPlayer} qui capture chaque @@ -666,6 +833,110 @@ class ClientRigEquipmentHandlerTest { } } + /** {@code itemId == null} → skip silencieux, aucun trigger. */ + @Test + void triggerOneshotById_nullItemId_skipsSilently() { + CapturingOneshotPlayer player = new CapturingOneshotPlayer(); + + ClientRigEquipmentHandler.triggerOneshotById( + player, + /* itemId */ null, + AnimationBindings::onUnequip, + id -> { throw new AssertionError("resolver ne doit pas etre appele pour null"); }, + PASSTHROUGH_RESOLVER + ); + + assertTrue(player.calls.isEmpty()); + } + + /** Registry lookup retourne null (itemId inconnu) → skip silencieux. */ + @Test + void triggerOneshotById_unknownItemId_skipsSilently() { + CapturingOneshotPlayer player = new CapturingOneshotPlayer(); + + ClientRigEquipmentHandler.triggerOneshotById( + player, + ITEM_A, + AnimationBindings::onUnequip, + id -> null, // inconnu + PASSTHROUGH_RESOLVER + ); + + assertTrue(player.calls.isEmpty()); + } + + /** Définition sans {@code animations} → skip silencieux. */ + @Test + void triggerOneshotById_nullBindings_skipsSilently() { + CapturingOneshotPlayer player = new CapturingOneshotPlayer(); + DataDrivenItemDefinition def = makeDef(ITEM_A, 10, /* animations */ null); + + ClientRigEquipmentHandler.triggerOneshotById( + player, + ITEM_A, + AnimationBindings::onUnequip, + id -> def, + PASSTHROUGH_RESOLVER + ); + + assertTrue(player.calls.isEmpty()); + } + + /** {@code onUnequip == null} dans les bindings → skip silencieux. */ + @Test + void triggerOneshotById_onUnequipNull_skipsSilently() { + CapturingOneshotPlayer player = new CapturingOneshotPlayer(); + AnimationBindings bindings = new AnimationBindings( + Map.of(LivingMotions.WALK, ANIM_WALK_A), + /* onEquip */ null, + /* onUnequip */ null + ); + DataDrivenItemDefinition def = makeDef(ITEM_A, 10, bindings); + + ClientRigEquipmentHandler.triggerOneshotById( + player, + ITEM_A, + AnimationBindings::onUnequip, + id -> def, + PASSTHROUGH_RESOLVER + ); + + assertTrue(player.calls.isEmpty()); + } + + /** + * Happy path : bindings.onUnequip non-null → oneshotPlayer appelé avec + * l'accessor résolu + transition time canonique. + */ + @Test + void triggerOneshotById_happyPath_callsPlayerWithResolvedAccessor() { + CapturingOneshotPlayer player = new CapturingOneshotPlayer(); + + 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.triggerOneshotById( + player, + ITEM_A, + AnimationBindings::onUnequip, + id -> def, + PASSTHROUGH_RESOLVER + ); + + assertEquals(1, player.calls.size()); + assertEquals(unequipId, player.calls.get(0).getKey()); + assertEquals(0.15F, player.calls.get(0).getValue(), 0.0001F, + "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.