Phase 2.4 review fixes : P0-BUG-001 joint order + P0-BUG-002 singleton + tests
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.
This commit is contained in:
notevil
@@ -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.
|
||||
|
||||
114
src/test/java/com/tiedup/remake/rig/TiedUpArmaturesTest.java
Normal file
114
src/test/java/com/tiedup/remake/rig/TiedUpArmaturesTest.java
Normal file
@@ -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)");
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user