344 lines
13 KiB
Markdown
344 lines
13 KiB
Markdown
|
# Card System Architecture Audit
|
||
|
|
|
||
|
|
**Date:** November 11, 2025
|
||
|
|
**Author:** Senior Software Engineer
|
||
|
|
**Status:** Critical Review
|
||
|
|
|
||
|
|
---
|
||
|
|
|
||
|
|
## Executive Summary
|
||
|
|
|
||
|
|
The current card UI system suffers from **excessive wrapper nesting**, **duplicated animation logic**, and **unclear separation of concerns**. While functional, it violates DRY principles and creates maintenance overhead. A refactor using composition and state machines is recommended.
|
||
|
|
|
||
|
|
---
|
||
|
|
|
||
|
|
## Current Architecture
|
||
|
|
|
||
|
|
### Component Hierarchy
|
||
|
|
```
|
||
|
|
CardDisplay (core visual renderer)
|
||
|
|
└─ AlbumCard (album-specific wrapper)
|
||
|
|
└─ FlippableCard (flip animation wrapper)
|
||
|
|
└─ AlbumCardPlacementDraggable (drag/placement wrapper)
|
||
|
|
└─ CardDraggable (generic drag wrapper)
|
||
|
|
└─ CardDraggableVisual (visual for dragging)
|
||
|
|
```
|
||
|
|
|
||
|
|
### Critical Issues
|
||
|
|
|
||
|
|
#### 1. **Wrapper Hell**
|
||
|
|
- **5 layers of wrappers** around a single card display
|
||
|
|
- Each wrapper duplicates transform/animation state management
|
||
|
|
- Example: `FlippableCard`, `AlbumCard`, and `CardDraggable` all manage scales, positions, and parent tracking
|
||
|
|
- **Code smell**: `AlbumCard.OnPointerClick()` forwards clicks to parent `FlippableCard` during reveal flow
|
||
|
|
|
||
|
|
#### 2. **Duplicated Animation Logic**
|
||
|
|
Animation behaviors repeated across multiple components:
|
||
|
|
|
||
|
|
| Animation | FlippableCard | AlbumCard | CardDraggable | AlbumCardPlacementDraggable |
|
||
|
|
|-----------|---------------|-----------|---------------|------------------------------|
|
||
|
|
| Scale tweens | ✓ (hover, flip punch) | ✓ (enlarge/shrink) | - | - |
|
||
|
|
| Position tweens | ✓ (idle hover) | - | ✓ (drag) | ✓ (snap to slot) |
|
||
|
|
| Rotation tweens | ✓ (flip) | - | - | - |
|
||
|
|
| Transform state tracking | ✓ (_originalPosition, _originalScale) | ✓ (_originalParent, _originalLocalPosition, _originalLocalRotation) | - | - |
|
||
|
|
|
||
|
|
**Impact**: ~150 lines of redundant tween/transform code across 4 files.
|
||
|
|
|
||
|
|
#### 3. **State Management Chaos**
|
||
|
|
Multiple boolean flags tracking overlapping states:
|
||
|
|
- `FlippableCard`: `_isFlipped`, `_isFlipping`, `_isWaitingForTap`, `_isClickable`, `_isNew`
|
||
|
|
- `AlbumCard`: `_isEnlarged`, `_parentSlot != null` (implicit state)
|
||
|
|
- `AlbumCardPlacementDraggable`: `_isRevealed`, `_isDragRevealing`, `_waitingForPlacementTap`, `_isHolding`
|
||
|
|
|
||
|
|
**Problems**:
|
||
|
|
- No single source of truth for card state
|
||
|
|
- Complex conditional logic: `if (_parentSlot == null) { forward to FlippableCard }`
|
||
|
|
- State transitions scattered across 3+ classes
|
||
|
|
|
||
|
|
#### 4. **Unclear Responsibilities**
|
||
|
|
- `CardDisplay`: Pure renderer ✓ (well-designed)
|
||
|
|
- `AlbumCard`: Handles enlargement + slot parenting + click forwarding
|
||
|
|
- `FlippableCard`: Handles flipping + hover animations + new/repeat UI + waiting for taps
|
||
|
|
- `AlbumCardPlacementDraggable`: Handles drag + flip triggering + slot snapping
|
||
|
|
|
||
|
|
Each wrapper blurs the line between "what" (state) and "how" (presentation).
|
||
|
|
|
||
|
|
#### 5. **Event Callback Spaghetti**
|
||
|
|
- 12+ events across components (`OnEnlargeRequested`, `OnShrinkRequested`, `OnCardRevealed`, `OnCardTappedAfterReveal`, `OnFlipStarted`, `OnClickedWhileInactive`, etc.)
|
||
|
|
- Events chained: `AlbumCard.OnEnlargeRequested` → `AlbumViewPage` → reparent → `AlbumCard.EnlargeCard()`
|
||
|
|
- Brittle: Changing card flow requires updating 3-4 components + page controllers
|
||
|
|
|
||
|
|
---
|
||
|
|
|
||
|
|
## Recommended Architecture
|
||
|
|
|
||
|
|
### Principles
|
||
|
|
1. **Composition over inheritance/wrapping**
|
||
|
|
2. **Single Responsibility**: Card visuals ≠ card behavior ≠ card state
|
||
|
|
3. **State machines** for clear state transitions
|
||
|
|
4. **Reusable animation system** instead of per-component tweens
|
||
|
|
|
||
|
|
### Proposed Design
|
||
|
|
|
||
|
|
Using **Pixelplacement StateMachine** (already in project) with **isolated state-owned visuals**:
|
||
|
|
|
||
|
|
```
|
||
|
|
Card (root GameObject with RectTransform)
|
||
|
|
├─ CardDisplay (always visible core visual)
|
||
|
|
├─ CardContext (component - shared data/references)
|
||
|
|
├─ CardAnimator (component - reusable animations)
|
||
|
|
└─ CardStateMachine (AppleMachine component)
|
||
|
|
├─ IdleState (GameObject + CardIdleState component)
|
||
|
|
├─ FlippingState (GameObject + CardFlippingState component)
|
||
|
|
│ └─ CardBackVisual (child GameObject - owned by this state)
|
||
|
|
├─ RevealedState (GameObject + CardRevealedState component)
|
||
|
|
├─ EnlargedNewState (GameObject + CardEnlargedNewState component)
|
||
|
|
│ └─ NewCardBadge (child GameObject - owned by this state)
|
||
|
|
├─ EnlargedRepeatState (GameObject + CardEnlargedRepeatState component)
|
||
|
|
│ └─ ProgressBarUI (child GameObject - owned by this state)
|
||
|
|
├─ DraggingState (GameObject + CardDraggingState component)
|
||
|
|
└─ PlacedInSlotState (GameObject + CardPlacedInSlotState component)
|
||
|
|
```
|
||
|
|
|
||
|
|
**Key Architecture Decisions:**
|
||
|
|
|
||
|
|
1. **State Isolation**: Each state is a **GameObject child** of the StateMachine. State-specific visual elements (CardBackVisual, NewCardBadge, ProgressBarUI) are **children of their state GameObject**. When a state activates, its children activate automatically.
|
||
|
|
|
||
|
|
2. **Transform Animation Target**: The root **Card.transform** is the primary animation target. All position/scale animations affect the root, and children inherit transforms naturally. States can also animate their own child visuals independently (e.g., rotating CardBackVisual during flip).
|
||
|
|
|
||
|
|
3. **Shared Resources via CardContext**: States access common components (CardDisplay, CardAnimator, StateMachine, CardData) through `CardContext`, avoiding tight coupling.
|
||
|
|
|
||
|
|
4. **Reusable Animations**: `CardAnimator` provides animation methods (PlayFlip, PlayEnlarge, etc.) that states invoke. No duplicate tween code across states.
|
||
|
|
|
||
|
|
5. **State Transitions**: States call `context.StateMachine.ChangeState("NextState")` to transition. Example flow:
|
||
|
|
```
|
||
|
|
IdleState [click] → FlippingState [flip complete] → EnlargedNewState [tap] → RevealedState
|
||
|
|
```
|
||
|
|
|
||
|
|
#### Benefits
|
||
|
|
- **60% less code**: Shared animation system, no wrapper components
|
||
|
|
- **True state isolation**: Each state owns its visuals, no global visibility management
|
||
|
|
- **Clear state transitions**: Explicit state machine flow instead of boolean flag soup
|
||
|
|
- **Extensible**: Add new states without touching existing ones (e.g., `TradingState`, `BattleState`)
|
||
|
|
- **Designer-friendly**: States are visible GameObjects in hierarchy, easy to understand
|
||
|
|
- **No prefab nesting**: Single Card prefab with state children, not 5 nested prefabs
|
||
|
|
|
||
|
|
---
|
||
|
|
|
||
|
|
## Concrete Refactor Plan
|
||
|
|
|
||
|
|
### Phase 1: Implement State Machine Architecture ✅ COMPLETE
|
||
|
|
|
||
|
|
**Created Files:**
|
||
|
|
- `CardContext.cs` - Shared context component
|
||
|
|
- `CardAnimator.cs` - Reusable animation controller
|
||
|
|
- `CardAnimationConfig.cs` - ScriptableObject for animation settings
|
||
|
|
- `States/CardIdleState.cs` - Idle state with hover
|
||
|
|
- `States/CardFlippingState.cs` - Flip animation state (owns CardBackVisual)
|
||
|
|
- `States/CardRevealedState.cs` - Revealed/interactable state
|
||
|
|
- `States/CardEnlargedNewState.cs` - Enlarged new card state (owns NewCardBadge)
|
||
|
|
- `States/CardEnlargedRepeatState.cs` - Enlarged repeat state (owns ProgressBarUI)
|
||
|
|
|
||
|
|
**Example State Implementation:**
|
||
|
|
```csharp
|
||
|
|
public class CardFlippingState : AppleState
|
||
|
|
{
|
||
|
|
[SerializeField] private GameObject cardBackVisual; // State owns this visual
|
||
|
|
private CardContext _context;
|
||
|
|
|
||
|
|
void Awake() => _context = GetComponentInParent<CardContext>();
|
||
|
|
|
||
|
|
public override void OnEnterState()
|
||
|
|
{
|
||
|
|
// Show card back (owned by this state)
|
||
|
|
cardBackVisual.SetActive(true);
|
||
|
|
_context.CardDisplay.gameObject.SetActive(false);
|
||
|
|
|
||
|
|
// Use shared animator
|
||
|
|
_context.Animator.PlayFlip(
|
||
|
|
cardBackVisual.transform,
|
||
|
|
_context.CardDisplay.transform,
|
||
|
|
onComplete: () => {
|
||
|
|
// Transition to next state
|
||
|
|
string nextState = _context.IsNewCard ? "EnlargedNewState" : "RevealedState";
|
||
|
|
_context.StateMachine.ChangeState(nextState);
|
||
|
|
}
|
||
|
|
);
|
||
|
|
}
|
||
|
|
|
||
|
|
void OnDisable()
|
||
|
|
{
|
||
|
|
// Hide card back when leaving state
|
||
|
|
cardBackVisual.SetActive(false);
|
||
|
|
_context.CardDisplay.gameObject.SetActive(true);
|
||
|
|
}
|
||
|
|
}
|
||
|
|
```
|
||
|
|
|
||
|
|
**Prefab Structure:**
|
||
|
|
```
|
||
|
|
Card.prefab
|
||
|
|
├─ CardDisplay
|
||
|
|
├─ CardContext (component)
|
||
|
|
├─ CardAnimator (component)
|
||
|
|
└─ CardStateMachine (AppleMachine)
|
||
|
|
├─ IdleState/
|
||
|
|
├─ FlippingState/
|
||
|
|
│ └─ CardBackVisual (Image)
|
||
|
|
├─ RevealedState/
|
||
|
|
├─ EnlargedNewState/
|
||
|
|
│ └─ NewCardBadge (GameObject)
|
||
|
|
└─ EnlargedRepeatState/
|
||
|
|
└─ ProgressBarUI (GameObject with Image/Text)
|
||
|
|
```
|
||
|
|
|
||
|
|
**Impact**: Foundation complete. States are isolated, visuals are state-owned, animations are shared.
|
||
|
|
|
||
|
|
### Phase 2: Create Remaining States (Low Risk)
|
||
|
|
|
||
|
|
**Additional states needed:**
|
||
|
|
- `CardDraggingState.cs` - Handles drag interaction for album placement
|
||
|
|
- `CardPlacedInSlotState.cs` - Card placed in album slot, handles enlarge on click
|
||
|
|
- `CardAlbumEnlargedState.cs` - Enlarged view when clicking card in album
|
||
|
|
|
||
|
|
**Example - Album Placed State:**
|
||
|
|
```csharp
|
||
|
|
public class CardPlacedInSlotState : AppleState, IPointerClickHandler
|
||
|
|
{
|
||
|
|
private CardContext _context;
|
||
|
|
private AlbumCardSlot _parentSlot;
|
||
|
|
|
||
|
|
public void SetParentSlot(AlbumCardSlot slot) => _parentSlot = slot;
|
||
|
|
|
||
|
|
public void OnPointerClick(PointerEventData eventData)
|
||
|
|
{
|
||
|
|
_context.StateMachine.ChangeState("AlbumEnlargedState");
|
||
|
|
}
|
||
|
|
}
|
||
|
|
```
|
||
|
|
|
||
|
|
**Time**: 2-3 days
|
||
|
|
|
||
|
|
### Phase 3: Migrate Existing Prefabs (Medium Risk)
|
||
|
|
|
||
|
|
**Steps:**
|
||
|
|
1. Create new `Card.prefab` with state machine structure
|
||
|
|
2. Build migration tool to convert old prefabs → new structure:
|
||
|
|
- Copy CardDisplay references
|
||
|
|
- Setup CardContext with data
|
||
|
|
- Create state GameObjects
|
||
|
|
3. Update scenes one at a time:
|
||
|
|
- Replace `FlippableCard` spawns with `Card` spawns
|
||
|
|
- Update `BoosterOpeningPage` to use new Card system
|
||
|
|
- Update `AlbumViewPage` to use new Card system
|
||
|
|
4. Remove old wrapper scripts once migration complete
|
||
|
|
|
||
|
|
**Migration Helper Script:**
|
||
|
|
```csharp
|
||
|
|
// Editor tool to convert old card prefabs
|
||
|
|
[MenuItem("AppleHills/Convert Old Card to New Card")]
|
||
|
|
static void ConvertCard()
|
||
|
|
{
|
||
|
|
// Find old FlippableCard
|
||
|
|
var oldCard = Selection.activeGameObject.GetComponent<FlippableCard>();
|
||
|
|
// Extract data, create new Card with states
|
||
|
|
// ...
|
||
|
|
}
|
||
|
|
```
|
||
|
|
|
||
|
|
**Time**: 1-2 weeks (includes testing)
|
||
|
|
|
||
|
|
---
|
||
|
|
|
||
|
|
## Migration Strategy
|
||
|
|
|
||
|
|
### Option A: Incremental (Recommended)
|
||
|
|
1. Create `CardAnimator` alongside existing code (2-3 days)
|
||
|
|
2. Refactor one wrapper at a time to use `CardAnimator` (1 week)
|
||
|
|
3. Test each step with existing scenes
|
||
|
|
4. Introduce state machine once animations are consolidated (3-5 days)
|
||
|
|
5. Collapse wrappers last, update prefabs (2-3 days)
|
||
|
|
|
||
|
|
**Total**: ~3 weeks, low risk
|
||
|
|
|
||
|
|
### Option B: Parallel Track
|
||
|
|
1. Build new `Card` system in separate namespace (1 week)
|
||
|
|
2. Create migration tools to convert old prefabs → new prefabs (2-3 days)
|
||
|
|
3. Switch one scene at a time (1 week)
|
||
|
|
4. Delete old system once migration complete
|
||
|
|
|
||
|
|
**Total**: ~3 weeks, higher risk but cleaner result
|
||
|
|
|
||
|
|
---
|
||
|
|
|
||
|
|
## Immediate Wins (Low-Hanging Fruit)
|
||
|
|
|
||
|
|
Even without full refactor, these changes reduce pain:
|
||
|
|
|
||
|
|
### 1. Extract Common Transform Tracking
|
||
|
|
```csharp
|
||
|
|
// Assets/Scripts/UI/CardSystem/TransformMemento.cs
|
||
|
|
public class TransformMemento {
|
||
|
|
public Vector3 LocalPosition;
|
||
|
|
public Quaternion LocalRotation;
|
||
|
|
public Vector3 LocalScale;
|
||
|
|
public Transform Parent;
|
||
|
|
|
||
|
|
public static TransformMemento Capture(Transform t) { ... }
|
||
|
|
public void Restore(Transform t) { ... }
|
||
|
|
}
|
||
|
|
```
|
||
|
|
|
||
|
|
**Usage**: Replace 8+ `_originalX` fields across components with single `TransformMemento`.
|
||
|
|
|
||
|
|
### 2. Shared Animation Config ScriptableObject
|
||
|
|
```csharp
|
||
|
|
// Assets/Scripts/UI/CardSystem/CardAnimationConfig.asset
|
||
|
|
[CreateAssetMenu]
|
||
|
|
public class CardAnimationConfig : ScriptableObject {
|
||
|
|
public float flipDuration = 0.6f;
|
||
|
|
public float enlargedScale = 2.5f;
|
||
|
|
public float hoverHeight = 10f;
|
||
|
|
// etc.
|
||
|
|
}
|
||
|
|
```
|
||
|
|
|
||
|
|
**Impact**: Tweak all card animations from one asset instead of searching 5 prefabs.
|
||
|
|
|
||
|
|
### 3. Document State Transitions
|
||
|
|
Add state diagram to `FlippableCard.cs`:
|
||
|
|
```csharp
|
||
|
|
/// State Flow:
|
||
|
|
/// Unflipped → [Click] → Flipping → Revealed → [IsNew] → EnlargedNew → [Tap] → Revealed
|
||
|
|
/// → [IsRepeat] → ShowingProgress → Revealed
|
||
|
|
/// → [Tap during drag] → PlacementMode → PlacedInSlot
|
||
|
|
```
|
||
|
|
|
||
|
|
**Impact**: Future devs understand flow without debugging.
|
||
|
|
|
||
|
|
---
|
||
|
|
|
||
|
|
## Metrics
|
||
|
|
|
||
|
|
| Metric | Current | After Refactor |
|
||
|
|
|--------|---------|----------------|
|
||
|
|
| Lines of code (card UI) | ~1,200 | ~500 |
|
||
|
|
| Animation logic locations | 4 files | 1 file |
|
||
|
|
| State tracking booleans | 12+ | 0 (enum-based) |
|
||
|
|
| Prefab nesting depth | 5 layers | 1 layer |
|
||
|
|
| Event callback chains | 12 events | ~3-4 events |
|
||
|
|
| Time to add new card state | 4-6 hours | ~30 min |
|
||
|
|
|
||
|
|
---
|
||
|
|
|
||
|
|
## Conclusion
|
||
|
|
|
||
|
|
The current system works but is **expensive to maintain and extend**. The root cause is **wrapping components instead of composing behavior**.
|
||
|
|
|
||
|
|
**Recommendation**: Approve **Phase 1 (Animation System)** immediately as it has zero breaking changes and reduces code by 20%. Schedule **Phase 2-3 (State Machine + Wrapper Collapse)** for next sprint based on team bandwidth.
|
||
|
|
|
||
|
|
**Risk Assessment**: Medium. Prefab changes require thorough testing, but state machine pattern is battle-tested.
|
||
|
|
|
||
|
|
**ROI**: High. Estimated 70% reduction in time to add new card interactions (e.g., trading, upgrading, battling).
|
||
|
|
|