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 a2e983c..21846b0 100644 --- a/src/main/java/com/tiedup/remake/v2/client/ClientRigEquipmentHandler.java +++ b/src/main/java/com/tiedup/remake/v2/client/ClientRigEquipmentHandler.java @@ -144,6 +144,26 @@ public final class ClientRigEquipmentHandler { * encore bootstrap) * * + *

Null-safety : seuls {@code player}, {@code patch} et + * {@code animator} sont null-checkés ici. Les autres scénarios null + * (bindings null, stack empty dans l'equipment, id null) sont gérés + * inline dans {@link #applyDefinitions} et + * {@link #extractSortedDefinitions} — chacun skip silencieusement sans + * throw.

+ * + *

Contract – ne JAMAIS appeler + * {@code animator.setCurrentMotionsAsDefault()} ailleurs dans le + * pipeline TiedUp. Cette méthode EF consolide les bindings custom + * courants dans la snapshot {@code defaultLivingAnimations} ; un + * {@code resetLivingAnimations()} ultérieur les restaurerait alors + * comme "defaults" après un unbind, et l'anim custom fuiterait au-delà + * de la durée de vie de l'item équipé. En prod EF, + * {@code setCurrentMotionsAsDefault} n'est appelé qu'une fois depuis + * {@code ClientAnimator.postInit()} pendant le bootstrap de + * l'animator, capturant les IDLE/WALK/RUN vanilla comme snapshot + * pérenne. Le rebuild TiedUp compte sur cet invariant : tout rebuild + * part d'un état EF vanilla stable.

+ * * @param player le player dont on rebuild les anims ; null tolérée */ public static void rebuildBondageAnimations(Player player) { @@ -164,6 +184,17 @@ public final class ClientRigEquipmentHandler { List sortedDefs = extractSortedDefinitions(equipped.values(), DataDrivenItemRegistry::get); + // 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). + // + // IMPORTANT : ne jamais appeler animator.setCurrentMotionsAsDefault() + // ailleurs dans le pipeline TiedUp — cela consoliderait les custom + // bindings dans les defaults, et un unbind ultérieur laisserait les + // anims en place (le reset restaurerait les ex-custom comme "defaults"). + // Pour un rebuild propre, on compte sur le fait que defaultLivingAnimations + // reste la snapshot originale EF (IDLE/WALK/RUN vanilla, capturée une + // seule fois dans ClientAnimator.postInit() durant le bootstrap). applyDefinitions( animator::resetLivingAnimations, animator::addLivingAnimation, @@ -195,6 +226,19 @@ public final class ClientRigEquipmentHandler { * même {@link LivingMotion}, on doit itérer le priorité 10 d'abord et * le priorité 20 ensuite (il écrase).

* + *

Sort déterministe :

+ *
    + *
  1. Primary : {@code posePriority} ASC → highest-priority itère en + * dernier → gagne le conflit (last put wins dans + * {@code addLivingAnimation} map semantics).
  2. + *
  3. Secondary : {@code id.toString()} lexicographique → tiebreaker + * stable indépendant de l'ordre de déclaration de + * {@link BodyRegionV2}. Deux items avec même {@code posePriority} + * résolvent sur leur {@link ResourceLocation} — un dev qui + * réordonne l'enum ne change plus silencieusement quelle anim + * gagne les conflits priority-tied.
  4. + *
+ * *

Note sur la généricité : l'API est paramétrée {@code } * plutôt que fixée à {@link ItemStack} pour permettre un unit test sans * bootstrap MC (le test passe des {@code Object} dummy, la prod passe @@ -231,7 +275,17 @@ public final class ClientRigEquipmentHandler { defs.add(def); } - defs.sort(Comparator.comparingInt(DataDrivenItemDefinition::posePriority)); + // Sort déterministe : + // 1. Primary : posePriority ASC → highest-priority itère en dernier → + // gagne le conflit (last put wins dans addLivingAnimation map + // semantics). + // 2. Secondary : id.toString() lexicographique → tiebreaker stable + // indépendant de BodyRegionV2 enum declaration order. Deux items + // avec même posePriority résolvent sur leur ResourceLocation. + defs.sort( + Comparator.comparingInt(DataDrivenItemDefinition::posePriority) + .thenComparing(d -> d.id().toString()) + ); return defs; } 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 c1e4225..97ce04e 100644 --- a/src/test/java/com/tiedup/remake/v2/client/ClientRigEquipmentHandlerTest.java +++ b/src/test/java/com/tiedup/remake/v2/client/ClientRigEquipmentHandlerTest.java @@ -245,6 +245,44 @@ class ClientRigEquipmentHandlerTest { assertSame(high, result.get(1), "posePriority=20 doit sortir en dernier (ASC)"); } + /** + * Deux items distincts avec la MÊME {@code posePriority} doivent être + * triés de manière déterministe via le tiebreaker secondaire + * {@code id.toString()} lexicographique. Ainsi l'ordre final ne dépend + * pas de {@link BodyRegionV2} declaration order (invariant fragile + * qu'un refactor pourrait casser silencieusement). + */ + @Test + void extractSortedDefinitions_equalPriority_usesIdLexicographicTiebreaker() { + ResourceLocation idAlpha = + ResourceLocation.fromNamespaceAndPath("tiedup", "alpha_cuffs"); + ResourceLocation idBeta = + ResourceLocation.fromNamespaceAndPath("tiedup", "beta_cuffs"); + ResourceLocation idCharlie = + ResourceLocation.fromNamespaceAndPath("tiedup", "charlie_cuffs"); + + DataDrivenItemDefinition alpha = makeDef(idAlpha, /* priority */ 10, null); + DataDrivenItemDefinition beta = makeDef(idBeta, 10, null); + DataDrivenItemDefinition charlie = makeDef(idCharlie, 10, null); + + // Input volontairement dans l'ordre inverse — [charlie, alpha, beta] + // — pour vérifier que le sort le réorganise bien sur id + // lexicographique et non sur ordre d'insertion. + List sorted = + ClientRigEquipmentHandler.extractSortedDefinitions( + List.of(charlie, alpha, beta), + Function.identity() + ); + + assertEquals(3, sorted.size()); + assertSame(alpha, sorted.get(0), + "alpha_cuffs doit sortir en premier (id.toString() min)"); + assertSame(beta, sorted.get(1), + "beta_cuffs doit sortir au milieu"); + assertSame(charlie, sorted.get(2), + "charlie_cuffs doit sortir en dernier (id.toString() max)"); + } + /** Un {@code null} dans l'iterable est silencieusement skip. */ @Test void extractSortedDefinitions_nullItemInList_skips() { @@ -462,19 +500,25 @@ class ClientRigEquipmentHandlerTest { } /** - * Sanity : si animResolver retourne un accessor pour l'ID fourni, le - * handler ne suppose pas de non-nullité — contract de - * {@link com.tiedup.remake.rig.TiedUpAnimationRegistry#resolveWithFallback} - * garantit non-null en prod, mais on vérifie qu'aucune assertion plus - * stricte n'a été introduite dans le handler. + * Sanity : si {@code animResolver} retourne {@code null} pour un id, le + * handler le forwarde tel quel à l'adder sans dereferencer. La gestion + * réelle des accessors null est la responsabilité de + * {@link com.tiedup.remake.rig.anim.client.ClientAnimator#addLivingAnimation} + * (out of scope de ce test — seule l'absence de NPE côté handler est + * vérifiée ici). + * + *

En prod, {@link com.tiedup.remake.rig.TiedUpAnimationRegistry#resolveWithFallback} + * garantit non-null (fallback {@code EMPTY_ANIMATION} + WARN). Le test + * simule un resolver "buggy" pour s'assurer qu'aucune assertion plus + * stricte n'a été introduite dans le handler côté TiedUp.

*/ @Test - void applyDefinitions_respectsAnimResolverContract() { + void applyDefinitions_nullAnimAccessor_forwardsToAdder() { AtomicInteger resolverCalls = new AtomicInteger(); ClientRigEquipmentHandler.LivingAnimationAdder adder = (motion, accessor) -> { // accessor vient du resolver, peut être null si resolver est buggy — // le handler ne doit pas le dereferencer lui-même - assertNull(accessor, "resolver renvoie null ici"); + assertNull(accessor, "resolver renvoie null ici, le handler forward tel quel"); }; AnimationBindings bindings = new AnimationBindings(