NotEvil/TiedUp-
4
2
Fork 0
Code Issues 1 Pull Requests Actions Packages Projects Releases Wiki Activity

P3 cross-check fixes : composite motion + dead code

Browse Source 
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<T> 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.
This commit is contained in:
notevil
2026-04-24 10:48:57 +02:00
parent 4648107ebe
commit dd30a8d4f9
5 changed files with 86 additions and 197 deletions
Download Patch File Download Diff File Expand all files Collapse all files

27
src/main/java/com/tiedup/remake/rig/patch/PlayerPatch.java
View File

@@ -133,7 +133,7 @@ public abstract class PlayerPatch<T extends Player> extends LivingEntityPatch<T>
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<T extends Player> extends LivingEntityPatch<T>
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;
}
/**

32
src/main/java/com/tiedup/remake/rig/util/ExtendableEnumManager.java
View File

@@ -43,11 +43,17 @@ public class ExtendableEnumManager<T extends ExtendableEnum> {
}
/**
* 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 <clinit>} 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 <clinit>} déclenchés par l'invoke reflective
* ci-dessous (re-entrant, no deadlock risk).
*/
public synchronized void loadEnum() {
List<String> orderByModid = new ArrayList<>(this.enums.keySet());
@@ -76,14 +82,14 @@ public class ExtendableEnumManager<T extends ExtendableEnum> {
}
/**
* 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;

86
src/main/java/com/tiedup/remake/v2/client/ClientRigEquipmentHandler.java
View File

@@ -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<BodyRegionV2, ResourceLocation> 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<ResourceLocation, ItemStack> itemIdToStack = new LinkedHashMap<>();
for (Map.Entry<BodyRegionV2, ItemStack> 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}.
*
* <p>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}).</p>
* <p>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}).</p>
*
* <p>No-op silencieux si :</p>
* <ul>
@@ -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}.
*
* <p>No-op silencieux si :</p>
* <ul>
* <li>Le stack n'a pas de {@link DataDrivenItemDefinition} (item V2 legacy) ;</li>
* <li>La définition n'a pas d'{@link AnimationBindings} (animations==null) ;</li>
* <li>Le binding ne déclare pas de one-shot pour ce trigger (onEquip ou
* onUnequip == null dans le JSON).</li>
* </ul>
*
* <p>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}.</p>
*
* <p><b>Priority forwarding</b> : 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.</p>
*
* <p>Package-private + générique {@code <T>} pour unit test avec
* callbacks injectés et stacks {@link Object} dummy (la prod passe
* {@link ItemStack}).</p>
*
* @param <T> 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 <T> void triggerOneshot(
OneshotPlayer oneshotPlayer,
T stack,
Function<AnimationBindings, ResourceLocation> extractor,
Function<T, DataDrivenItemDefinition> defResolver,
Function<ResourceLocation, AssetAccessor<? extends StaticAnimation>> 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<? extends StaticAnimation> 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

10
src/test/java/com/tiedup/remake/rig/patch/PlayerPatchUpdateMotionTest.java
View File

@@ -25,6 +25,16 @@ import com.tiedup.remake.rig.anim.TiedUpLivingMotions;
* <p>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.</p>
*
* <p><b>Composite motion assignment (P3 cross-check fix)</b> : {@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}.</p>
*/
class PlayerPatchUpdateMotionTest {

128
src/test/java/com/tiedup/remake/v2/client/ClientRigEquipmentHandlerTest.java
View File

@@ -57,8 +57,9 @@ import com.tiedup.remake.v2.bondage.movement.MovementStyle;
* <li>{@link ClientRigEquipmentHandler#diffKeys} — diff key-based,
* base du détection equip / unequip post-P3-07.</li>
* <li>{@link ClientRigEquipmentHandler#triggerOneshotById} — fire
* one-shot à partir d'un itemId (cas unequip post-P3-07 où le
* stack est parti).</li>
* one-shot à partir d'un itemId (utilisé pour equip et unequip
* post-P3-07 : le callsite key-based ne manipule plus de stack
* concret).</li>
* </ul>
*
* <p>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());
}
}