Best Practices

Code Organization

Shader Profile Management

DO: Define explicit shader profiles for known shaders

["Shaders/SkinnedMesh/Diffuse_Tint"] = new ShaderProfile
{
    ColorTintParam = "Tint_Color",  // Explicit, shader-specific
    DiffuseTexture = "Diffuse_Texture",
    HasBloom = false
}

DON'T: Use generic fallbacks in DefaultProfile

// ❌ BAD
DefaultProfile = new ShaderProfile
{
    ColorTintParam = "Color",  // Too generic!
    ...
}

Parameter Resolution

DO: Check multiple parameter name variations

BloomStrength = TryGetFloat(profile.BloomIntensityParam) 
             ?? TryGetFloat(DefaultProfile.BloomIntensityParam)
             ?? TryGetFloat("BloomIntensity")      // Variation
             ?? TryGetFloat("Emissive_Strength")    // Alternative
             ?? 0;

DON'T: Only check one parameter name

// ❌ BAD - Misses variations
BloomStrength = TryGetFloat("Bloom_Strength") ?? 0;

Texture Handling

DO: Always clone before modifying

const clonedMap = material.map.clone();
material.map = clonedMap;
clonedMap.repeat.set(newRepeatX, newRepeatY);

DON'T: Modify shared instances

// ❌ BAD - Modifies shared texture
material.map.repeat.set(newRepeatX, newRepeatY);

Procedural Shader Handling

DO: Generate placeholder textures

if (magickImage == null && staticMaterialForTexture != null && 
    staticMaterialForTexture.IsProceduralShader())
{
    // Generate 1x1 texture from color params
    magickImage = new MagickImage(MagickColor.FromRgb(r, g, b), 1, 1);
}

DON'T: Skip material creation

// ❌ BAD - Material never created
if (magickImage == null) {
    continue;
}

Testing

Always Test These Cases

1. Soraka Skin16: Previously had incorrect red tint on body (generic "Color" param)
2. Renata Skin0: Has Weapon_Glass and Canister_Glass (procedural shaders)
3. Annie Skin31: Has time-based auto-animated flipbooks on body
4. Milio Skin0: Has animation-synced flipbooks on eyes/mouth

Verification Scripts

Use the Python scripts in common-pitfalls.md to verify:

• No incorrect baseColorFactor tinting
• Glass materials have isGlass: true and glassParams
• Flipbook materials have flipbookSpeed when needed

Documentation

When Adding New Shader Profiles

1. Check shaders.bin.json for parameter names
2. Verify parameter usage (is it for base tint or VFX?)
3. Add explicit profile in StaticMaterial.cs
4. Test with actual skin that uses the shader
5. Document in this file if it's a special case

When Fixing Bugs

1. Document the mistake in common-pitfalls.md
2. Add verification steps
3. Update this file with the correct pattern
4. Add to daily log

Frontend Material Handling

Glass Material Conversion

DO: Convert to MeshPhysicalMaterial with all parameters

if (userData.isGlass) {
    const glassMaterial = new MeshPhysicalMaterial({
        color: originalMaterial.color,
        map: originalMaterial.map,
        transparent: true,
        opacity: opacity,
        transmission: transmission,
        roughness: roughness,
        ior: ior,
        thickness: thickness,
        depthWrite: false,
    });
    object.material = glassMaterial;
}

Flipbook Animation

DO: Handle both animation-synced and time-based

// Animation-synced (from keyframes)
if (flipbookAccessoryHash) {
    // Update from animation keyframes
}

// Time-based (auto-animated)
if (flipbookSpeed) {
    // Update based on elapsed time
    elapsedTime += delta;
    const frameDuration = 1 / flipbookSpeed;
    if (elapsedTime >= frameDuration) {
        // Advance frame
    }
}

Loop Grace Period (Model Viewer Engine)

• Problem: Submesh visibility flickers when animations loop (events restart at frame 0).
• Fix: Track lastVisibilityFrame; detect loop when currentFrame < lastVisibilityFrame - 1; set loopGraceFrames ≈ 2.5 to skip visibility updates for a few frames after a loop.
• Reset: Clear lastVisibilityFrame and loopGraceFrames on model load, animation start, and stop.
• Scope: Only affects models with submeshVisibilityEvents; guarded, so no overhead elsewhere.

Extractor-Specific Practices

Hash Table Management

DO: Always handle hash table loading failures gracefully

if (!await HashTables.TryLoadLatest(logger))
{
    logger.Warning("Failed to load latest hash tables, trying cached...");
    if (!await HashTables.TryLoadExisting(path, logger))
    {
        logger.Fatal("No hash tables available");
        return false;
    }
}

DON'T: Assume hash tables are always available

// ❌ BAD - Will crash if hash tables not loaded
var path = HashTables.Game[hash];

WAD Downloading

DO: Clear PBE cache to ensure fresh downloads

if (patchline.Equals("pbe", StringComparison.OrdinalIgnoreCase))
{
    // Clear cache for PBE to get latest content
    if (File.Exists(cachedPath))
        File.Delete(cachedPath);
}

DON'T: Use stale PBE cache

// ❌ BAD - PBE changes frequently, cache may be outdated
if (File.Exists(cachedPath))
    return cachedPath; // Without checking patchline

Path Resolution

DO: Support both file paths and champion names

if (LeaguePathResolver.IsFilePath(input))
{
    // Use as file path
}
else
{
    // Resolve champion name to WAD path
    var resolved = await LeaguePathResolver.ResolveInputAsync(input, ...);
}

DON'T: Assume input is always a file path

// ❌ BAD - Fails if user provides champion name
var wad = new StringWad(input);

Error Handling

DO: Provide clear error messages with context

logger.Fatal("Couldn't open '{Path}'. Ensure the WAD file exists and is not corrupted.", filePath);

DON'T: Use generic error messages

// ❌ BAD - Not helpful
logger.Fatal("Error");

Error Prevention

Before Making Changes

1. Read common-pitfalls.md
2. Check if similar changes were made before (daily logs)
3. Test with known problematic skins
4. Verify no regressions
5. Test with both PBE and live patchlines
6. Verify hash tables load correctly

Code Review Checklist

• [ ] No generic parameter fallbacks
• [ ] Textures cloned before modification
• [ ] Procedural shaders handled correctly
• [ ] Multiple parameter name variations checked
• [ ] Transparency uses both alphaMode and baseColorFactor
• [ ] Flipbook speed extracted when needed
• [ ] Hash table loading handled gracefully
• [ ] PBE cache cleared appropriately
• [ ] Path resolution supports both files and names