From 39f6177595a53d6b8e7050f60020a00a05575f02 Mon Sep 17 00:00:00 2001
From: notevil <notevil@noreply.com>
Date: Wed, 22 Apr 2026 22:27:01 +0200
Subject: [PATCH] Phase 2.4 review fixes : P0-BUG-001 joint order + P0-BUG-002
 singleton + tests
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

P0-BUG-001 — Ordre IDs joints Elbow/Hand/Tool ≠ EF
  TiedUpArmatures.buildBiped() assignait Arm_R=11, Elbow_R=12, Hand_R=13,
  Tool_R=14 (idem gauche 16/17/18/19) alors que VanillaModelTransformer.RIGHT_ARM
  encode upperJoint=11, lowerJoint=12, middleJoint=14 — donc Arm_R=11, Hand_R=12,
  Elbow_R=14. Résultat : lowerJoint=17 (SimpleTransformer qui attache les
  vertices à Hand) pointait en fait vers Elbow_L → bras tordus au rendu.
  Fix : réassigner les IDs dans buildBiped() pour matcher le layout EF
  (Arm_R=11, Hand_R=12, Tool_R=13, Elbow_R=14 ; symétrique 16-19).
  VanillaModelTransformer non touché (source de vérité EF).

P0-BUG-002 — Singleton BIPED non thread-safe
  if (BIPED_INSTANCE == null) BIPED_INSTANCE = buildBiped() est un double-init
  race. En SP intégré (client + server threads sur la même JVM), deux threads
  peuvent entrer simultanément dans le if null et créer deux HumanoidArmature
  distincts — pose matrices incohérentes selon les call sites.
  Fix : Holder idiome (static inner class). Le class-init lock JVM garantit
  (JLS §12.4.1) qu'une seule init, visible à tous les threads, sans
  synchronized ni volatile. Zero overhead après init.

P1 — Nouveau TiedUpArmaturesTest (4 tests)
  - bipedHas20Joints : BIPED.get().getJointNumber() == 20
  - searchBipedJointByNameReturnsNonNull : vérifie les 20 noms EF
  - jointIdsMatchEfLayout : verrou P0-BUG-001 (id=12→Hand_R, id=14→Elbow_R,
    etc.) — aurait attrapé le bug en review initiale
  - bipedSingleton : BIPED.get() == BIPED.get() (verrou P0-BUG-002)

P2 backlog tracé dans docs/plans/rig/PHASE0_DEGRADATIONS.md :
  biped collapsed visuellement jusqu'à Phase 2.7, PlayerPatch.yBodyRot sans
  lerp, ridingEntity non géré, isFirstPersonHidden nommage ambigu,
  ServerPlayerPatch hérite méthodes client-only sans @OnlyIn.

Tests : 15 GREEN (11 bridge pré-existants + 4 nouveaux biped).
Compile clean.
---
 .../tiedup/remake/rig/TiedUpArmatures.java    |  94 +++++++++------
 .../remake/rig/TiedUpArmaturesTest.java       | 114 ++++++++++++++++++
 2 files changed, 173 insertions(+), 35 deletions(-)
 create mode 100644 src/test/java/com/tiedup/remake/rig/TiedUpArmaturesTest.java

diff --git a/src/main/java/com/tiedup/remake/rig/TiedUpArmatures.java b/src/main/java/com/tiedup/remake/rig/TiedUpArmatures.java
index c0f7d34..e004a66 100644
--- a/src/main/java/com/tiedup/remake/rig/TiedUpArmatures.java
+++ b/src/main/java/com/tiedup/remake/rig/TiedUpArmatures.java
@@ -41,16 +41,20 @@ import com.tiedup.remake.rig.math.OpenMatrix4f;
  * <h2>Hiérarchie biped EF (20 joints)</h2>
  *
  * <pre>
- *   Root
- *   ├─ Thigh_R ── Leg_R ── Knee_R
- *   ├─ Thigh_L ── Leg_L ── Knee_L
- *   └─ Torso
- *      └─ Chest
- *         ├─ Head
- *         ├─ Shoulder_R ── Arm_R ── Elbow_R ── Hand_R ── Tool_R
- *         └─ Shoulder_L ── Arm_L ── Elbow_L ── Hand_L ── Tool_L
+ *   Root                                                    id=0
+ *   ├─ Thigh_R ── Leg_R ── Knee_R                           id=1,2,3
+ *   ├─ Thigh_L ── Leg_L ── Knee_L                           id=4,5,6
+ *   └─ Torso                                                id=7
+ *      └─ Chest                                             id=8
+ *         ├─ Head                                           id=9
+ *         ├─ Shoulder_R ── Arm_R ── Elbow_R ── Hand_R ── Tool_R   ids=10,11,14,12,13
+ *         └─ Shoulder_L ── Arm_L ── Elbow_L ── Hand_L ── Tool_L   ids=15,16,19,17,18
  * </pre>
  *
+ * <p>Les IDs des bras ne suivent pas l'ordre hiérarchique parent→enfant : c'est
+ * voulu pour rester aligné avec le layout attendu par {@code VanillaModelTransformer}
+ * (upperJoint=Arm, lowerJoint=Hand, middleJoint=Elbow). Voir {@link #buildBiped()}.</p>
+ *
  * <p><b>Noms conservés verbatim EF</b> (pas renommés en TiedUp style) car :</p>
  * <ul>
  *   <li>Le {@code VanillaModelTransformer} forké EF (Phase 2.2) référence ces
@@ -69,19 +73,29 @@ public final class TiedUpArmatures {
         ResourceLocation.fromNamespaceAndPath(TiedUpRigConstants.MODID, "armature/biped");
 
     /**
-     * Singleton biped — chargé à la première ref (init order :
-     * {@link TiedUpRigConstants} → chaînes d'ids → ici via
-     * {@link com.tiedup.remake.rig.patch.PlayerPatch#getArmature()}).
+     * Holder idiome pour init lazy + thread-safe sans synchronized.
      *
-     * <p>Si construit plus tôt (ex. static init combat-lourd avant FMLCommonSetup)
-     * risquerait de toucher des classes client — garder cet accès lazy si les
-     * call sites évoluent.</p>
+     * <p>Le class loader JVM garantit qu'une classe est initialisée au plus une
+     * fois, et que l'init est visible à tous les threads (JLS §12.4.1 — the class
+     * initialization lock est acquis automatiquement). Deux threads qui touchent
+     * {@code Holder.INSTANCE} simultanément ne peuvent pas observer l'instance
+     * non-initialisée ni en créer deux exemplaires. Intégré SP (client + server
+     * threads concurrents sur la même JVM) safe.</p>
+     *
+     * <p>Raison du fix (review Phase 2.4, P0-BUG-002) : le pattern précédent
+     * {@code if (BIPED_INSTANCE == null) BIPED_INSTANCE = buildBiped();} est
+     * un double-init race — deux threads entrent tous les deux dans le if,
+     * les deux créent un HumanoidArmature distinct, dernier gagne et pollue
+     * le cache.</p>
      */
-    private static HumanoidArmature BIPED_INSTANCE;
+    private static final class Holder {
+        static final HumanoidArmature INSTANCE = buildBiped();
+        private Holder() {}
+    }
 
     /**
-     * AssetAccessor biped biped TiedUp. Construit à la première lecture de
-     * {@link AssetAccessor#get()}.
+     * AssetAccessor biped TiedUp. L'instance est construite lazy à la première
+     * référence {@code Holder.INSTANCE} (thread-safe via class-init lock JVM).
      *
      * <p>Utilisé par {@link com.tiedup.remake.rig.patch.PlayerPatch#getArmature()}
      * et par les futurs {@code StaticAnimation(… , BIPED)} Phase 2.7+.</p>
@@ -89,10 +103,7 @@ public final class TiedUpArmatures {
     public static final AssetAccessor<HumanoidArmature> BIPED = new AssetAccessor<>() {
         @Override
         public HumanoidArmature get() {
-            if (BIPED_INSTANCE == null) {
-                BIPED_INSTANCE = buildBiped();
-            }
-            return BIPED_INSTANCE;
+            return Holder.INSTANCE;
         }
 
         @Override
@@ -110,17 +121,24 @@ public final class TiedUpArmatures {
     };
 
     /**
-     * Build procédural de la hiérarchie biped EF. 20 joints, IDs 0..19 dans
-     * l'ordre d'insertion (important pour {@link Joint#getId()} stabilité
-     * inter-runs).
+     * Build procédural de la hiérarchie biped EF. 20 joints, IDs 0..19 assignés
+     * explicitement pour matcher le layout attendu par
+     * {@link com.tiedup.remake.rig.mesh.transformer.VanillaModelTransformer}
+     * (cf. constantes {@code RIGHT_ARM}/{@code LEFT_ARM} — upperJoint/lowerJoint/middleJoint).
+     *
+     * <p>Ordre d'insertion LinkedHashMap ≠ ordre des IDs pour les bras (hand
+     * vient AVANT elbow dans les IDs, pour aligner sur la sémantique EF où
+     * {@code middleJoint = elbow}). Voir commentaires inline.</p>
      *
      * <p>Toutes les transforms sont identity — Phase 2.7 remplacera par les
      * offsets Blender mesurés (cf. doc header).</p>
      */
     private static HumanoidArmature buildBiped() {
-        // Tous les joints (ID == ordre d'insertion dans la map).
-        // On utilise LinkedHashMap pour garantir l'ordre d'itération stable
-        // (OpenMatrix4f.allocateMatrixArray dimensionne sur jointCount).
+        // ID Joint assigné explicitement au constructeur (pas par position dans
+        // la map) — Armature.jointById est construit depuis joint.getId().
+        // On utilise LinkedHashMap malgré tout pour garantir un ordre d'itération
+        // stable (utile pour le debug et pour OpenMatrix4f.allocateMatrixArray
+        // qui dimensionne sur jointCount).
         Map<String, Joint> joints = new LinkedHashMap<>(20);
 
         // Pattern EF : tous les joints démarrent avec une localTransform identity.
@@ -141,19 +159,25 @@ public final class TiedUpArmatures {
         Joint chest = joint(joints, "Chest", 8);
         Joint head = joint(joints, "Head", 9);
 
-        // Bras droit
+        // Bras droit — IDs alignés sur le layout EF (VanillaModelTransformer.RIGHT_ARM
+        // encode upperJoint=11, lowerJoint=12, middleJoint=14, cf.
+        // VanillaModelTransformer:50). L'insertion dans le LinkedHashMap reste
+        // dans l'ordre hiérarchique (shoulder → arm → elbow → hand → tool) pour
+        // préserver la lisibilité de l'iteration ; les IDs déterminent le
+        // mapping jointById utilisé par VanillaModelTransformer + SimpleTransformer.
         Joint shoulderR = joint(joints, "Shoulder_R", 10);
         Joint armR = joint(joints, "Arm_R", 11);
-        Joint elbowR = joint(joints, "Elbow_R", 12);
-        Joint handR = joint(joints, "Hand_R", 13);
-        Joint toolR = joint(joints, "Tool_R", 14);
+        Joint handR = joint(joints, "Hand_R", 12);
+        Joint toolR = joint(joints, "Tool_R", 13);
+        Joint elbowR = joint(joints, "Elbow_R", 14);
 
-        // Bras gauche
+        // Bras gauche — symétrique : Arm_L=16, Hand_L=17, Tool_L=18, Elbow_L=19
+        // (VanillaModelTransformer.LEFT_ARM upperJoint=16, lowerJoint=17, middleJoint=19).
         Joint shoulderL = joint(joints, "Shoulder_L", 15);
         Joint armL = joint(joints, "Arm_L", 16);
-        Joint elbowL = joint(joints, "Elbow_L", 17);
-        Joint handL = joint(joints, "Hand_L", 18);
-        Joint toolL = joint(joints, "Tool_L", 19);
+        Joint handL = joint(joints, "Hand_L", 17);
+        Joint toolL = joint(joints, "Tool_L", 18);
+        Joint elbowL = joint(joints, "Elbow_L", 19);
 
         // Hiérarchie. addSubJoints est idempotent (skip si déjà présent) — safe
         // de le réappeler, utile si on étend plus tard.
diff --git a/src/test/java/com/tiedup/remake/rig/TiedUpArmaturesTest.java b/src/test/java/com/tiedup/remake/rig/TiedUpArmaturesTest.java
new file mode 100644
index 0000000..ebb1da8
--- /dev/null
+++ b/src/test/java/com/tiedup/remake/rig/TiedUpArmaturesTest.java
@@ -0,0 +1,114 @@
+/*
+ * © 2026 TiedUp! Remake Contributors, distributed under GPLv3.
+ */
+
+package com.tiedup.remake.rig;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertSame;
+
+import org.junit.jupiter.api.Test;
+
+import com.tiedup.remake.rig.armature.HumanoidArmature;
+
+/**
+ * Tests du biped procédural TiedUp.
+ *
+ * <p>Ces tests verrouillent en particulier le mapping {@code id → joint name}
+ * qui doit rester aligné sur les constantes {@code RIGHT_ARM} / {@code LEFT_ARM}
+ * de {@code VanillaModelTransformer} — sinon les bras rendent tordus (ex :
+ * {@code lowerJoint=17} pointerait vers Elbow_L au lieu de Hand_L).</p>
+ *
+ * <p>La review Phase 2.4 (P0-BUG-001) a montré que l'ordre précédent
+ * Arm_R=11, Elbow_R=12, Hand_R=13, Tool_R=14 cassait silencieusement le
+ * transformer. {@link #jointIdsMatchEfLayout()} aurait attrapé ce bug.</p>
+ */
+class TiedUpArmaturesTest {
+
+	@Test
+	void bipedHas20Joints() {
+		assertEquals(20, TiedUpArmatures.BIPED.get().getJointNumber(),
+			"Le biped EF standard doit avoir exactement 20 joints");
+	}
+
+	@Test
+	void searchBipedJointByNameReturnsNonNull() {
+		HumanoidArmature biped = TiedUpArmatures.BIPED.get();
+
+		String[] expectedNames = {
+			"Root", "Head",
+			"Torso", "Chest",
+			"Shoulder_L", "Shoulder_R",
+			"Arm_L", "Arm_R",
+			"Hand_L", "Hand_R",
+			"Elbow_L", "Elbow_R",
+			"Tool_L", "Tool_R",
+			"Thigh_L", "Thigh_R",
+			"Leg_L", "Leg_R",
+			"Knee_L", "Knee_R"
+		};
+
+		for (String name : expectedNames) {
+			assertNotNull(biped.searchJointByName(name),
+				"Joint '" + name + "' introuvable dans le biped — alignement EF cassé");
+		}
+	}
+
+	/**
+	 * Verrou explicite contre P0-BUG-001. Les constantes EF
+	 * {@code VanillaModelTransformer.RIGHT_ARM} encodent
+	 * {@code upperJoint=11, lowerJoint=12, middleJoint=14}, i.e.
+	 * Arm_R=11, Hand_R=12, Elbow_R=14 (symétrique gauche : 16/17/19).
+	 *
+	 * <p>Ce test DOIT échouer si quelqu'un réordonne les IDs dans
+	 * {@link TiedUpArmatures#buildBiped()} sans mettre à jour
+	 * {@code VanillaModelTransformer} en miroir.</p>
+	 */
+	@Test
+	void jointIdsMatchEfLayout() {
+		HumanoidArmature biped = TiedUpArmatures.BIPED.get();
+
+		// Right arm layout (VanillaModelTransformer.RIGHT_ARM : 11, 12, _, 14)
+		assertEquals("Arm_R", biped.searchJointById(11).getName(),
+			"id=11 doit être Arm_R (VanillaModelTransformer.RIGHT_ARM.upperJoint)");
+		assertEquals("Hand_R", biped.searchJointById(12).getName(),
+			"id=12 doit être Hand_R (VanillaModelTransformer.RIGHT_ARM.lowerJoint)");
+		assertEquals("Elbow_R", biped.searchJointById(14).getName(),
+			"id=14 doit être Elbow_R (VanillaModelTransformer.RIGHT_ARM.middleJoint)");
+
+		// Left arm layout (VanillaModelTransformer.LEFT_ARM : 16, 17, _, 19)
+		assertEquals("Arm_L", biped.searchJointById(16).getName(),
+			"id=16 doit être Arm_L (VanillaModelTransformer.LEFT_ARM.upperJoint)");
+		assertEquals("Hand_L", biped.searchJointById(17).getName(),
+			"id=17 doit être Hand_L (VanillaModelTransformer.LEFT_ARM.lowerJoint)");
+		assertEquals("Elbow_L", biped.searchJointById(19).getName(),
+			"id=19 doit être Elbow_L (VanillaModelTransformer.LEFT_ARM.middleJoint)");
+
+		// Tool_R=13, Tool_L=18 (pas dans les LimbPartTransformer mais utilisé
+		// par StaticAnimation itemInHand — on verrouille l'emplacement aussi).
+		assertEquals("Tool_R", biped.searchJointById(13).getName(),
+			"id=13 doit être Tool_R (attaché après Hand_R dans la chaîne)");
+		assertEquals("Tool_L", biped.searchJointById(18).getName(),
+			"id=18 doit être Tool_L (attaché après Hand_L dans la chaîne)");
+	}
+
+	/**
+	 * Verrou contre P0-BUG-002. La première implémentation utilisait un
+	 * double-null-check non thread-safe — SP intégré (client + server threads
+	 * concurrent) pouvait créer deux instances distinctes.
+	 *
+	 * <p>Le Holder idiome (static inner class) garantit via le class-init lock
+	 * JVM qu'il n'y a qu'une seule init. Ce test vérifie que deux appels
+	 * successifs de {@code BIPED.get()} renvoient bien la même référence —
+	 * ne prouve pas l'atomicité mais couvre le cas régression "retour à un
+	 * nouveau build à chaque get".</p>
+	 */
+	@Test
+	void bipedSingleton() {
+		HumanoidArmature first = TiedUpArmatures.BIPED.get();
+		HumanoidArmature second = TiedUpArmatures.BIPED.get();
+		assertSame(first, second,
+			"BIPED.get() doit retourner la même instance (singleton Holder)");
+	}
+}