feat(animation): add feature plan for advanced layering, mixing, effe…#47
feat(animation): add feature plan for advanced layering, mixing, effe…#47KristofferKarlAxelEkstrand merged 48 commits intomainfrom
Conversation
…cts, and BPM sync
There was a problem hiding this comment.
Pull request overview
This PR adds a comprehensive feature plan document for advanced VJ capabilities and implements the first step: automatic 1-bit conversion for channel 4 bitmask animations. While the code changes are solid, the feature plan document contains sections that describe planned functionality as if it's currently implemented, which could cause confusion.
Key Changes
- Implemented automatic 1-bit black & white conversion for channel 4 animations in the build pipeline
- Added bitmask detection and processing logic to
optimize.js - Added console reporting for bitmask conversion count
- Added comprehensive feature plan document outlining future VJ enhancements (multi-layer mixing, effects, BPM sync)
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
scripts/animations/lib/optimize.js |
Adds bitmask channel constant, detection function, and 1-bit conversion logic using Sharp's grayscale+threshold pipeline. Correctly prioritizes correctness over file size for bitmasks. |
scripts/animations/index.js |
Adds bitmask count reporting to the optimization step console output. |
.github/prompts/features-plan.prompt.md |
Comprehensive 603-line feature plan document outlining advanced VJ features. Some sections describe functionality as currently implemented when it's actually planned for future phases (see comments). |
…t configurable depths and improve pipeline processing
…ced visual effects handling - Added EffectsManager to manage visual effects for A/B and global channels, supporting multiple effects and intensity control. - Introduced LayerGroup to manage animation slots for layers, handling MIDI note events and velocity-based layer selection. - Created MaskManager to manage bitmask animations for layer mixing, with latching behavior and transition types based on MIDI notes. - Developed integration tests for LayerManager, LayerGroup, MaskManager, and EffectsManager to ensure correct functionality and interaction. - Enhanced validation tests for animation properties, including bitDepth and beatsPerFrame, ensuring robust error handling and compliance with expected formats.
- Add status indicators (✅ CURRENT /⚠️ PLANNED) throughout document - Bit depth conversion for channel 4 and meta.json is IMPLEMENTED - Runtime mixing, effects, BPM sync, and multi-layer are PLANNED - Resolves reviewer feedback about ambiguous implementation status
|
Both review comments regarding the documentation clarity have been addressed in commit e5f3a9e ("docs(features-plan): clarify current vs planned implementation"). The document now has clear markers:
All code examples now accurately reflect the current implementation state. |
Addresses PR review feedback: - Mark bitDepth meta.json config as PLANNED (build code exists but not active) - Clarify that current implementation only auto-converts channel 4 to 1-bit - Mark getTargetBitDepth code examples as planned infrastructure - Remove ambiguous 'CURRENT IMPLEMENTATION' markers from planned features Current: Channel 4 auto-converts to 1-bit for hard-cut bitmask mixing Planned: Configurable bitDepth (2, 4, 8-bit) via meta.json for smooth blending
…cture for better asset management
- Renderer.js: use consistent Math.max alpha handling for 1-bit mixing - Renderer.js: add TODO comments for unused _intensity params in effects - Renderer.js: save/restore imageSmoothingEnabled after zoom effect - scripts/animations/index.js: fix bitDepthCounts[1] access (was bitmaskCount) - MaskManager.js: add TODO for retrieving bitDepth from animation metadata - AnimationLayer.js: document playToContext frame advancement behavior
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 23 changed files in this pull request and generated 13 comments.
Comments suppressed due to low confidence (1)
scripts/animations/lib/optimize.js:168
- [nitpick] In the bit depth conversion logic, when
bitDepth !== null, the optimized version is always kept regardless of file size (line 161). This could result in larger files being deployed. While the comment explains this is for "correctness over size", consider logging a warning when the converted version is significantly larger:
if (bitDepth !== null || tempStats.size < originalSize) {
if (bitDepth !== null && tempStats.size > originalSize * 1.5) {
console.warn(`Warning: ${sourcePath} - bit depth conversion increased size by ${((tempStats.size / originalSize - 1) * 100).toFixed(1)}%`);
}
await fs.rename(tempPath, cachePath);
// ...
}This would help identify cases where the conversion is producing unexpectedly large files.
// For bit depth conversions, always use the converted version (correctness over size)
// For regular images, only keep optimized version if it's smaller
if (bitDepth !== null || tempStats.size < originalSize) {
await fs.rename(tempPath, cachePath);
optimizedSize = tempStats.size;
cleanupNeeded = false;
} else {
// Original is smaller, just copy it
await fs.unlink(tempPath);
cleanupNeeded = false;
- MaskManager: Remove outdated TODO, clarify bitDepth comment - validate.js: Improve beatsPerFrame error messages (separate type/value checks) - Renderer: Add clarifying comments for glitch effect (intentional vertical displacement) - Renderer: Enhance mirror/split effect comments (reserved params for future use) Note: Alpha channel consistency in 1-bit mixing and imageSmoothingEnabled restore were already fixed in previous commits.
… glitch offset; improve effect TODOs
- AnimationLayer: prevent division by zero when BPM is 0 - optimize.js: add NaN check for channel parsing in getTargetBitDepth
PR Review Response SummaryThank you for the thorough review! Here's my response to the unresolved comments: Fixes Applied (commit 2c45caf)
Already Addressed in Existing CodeSeveral concerns were already addressed by prior commits in this PR:
Declined (by design or out of scope)
All threads have been reviewed and resolved appropriately. |
Code review cleanups: - LayerGroup: Remove unused #name field, name/channels getters, handlesChannel(), getActiveLayerCount(), and cleanupFinishedLayers() methods - LayerManager: Update LayerGroup constructor calls (name param removed) - MaskManager: Remove unused #currentNote and #currentVelocity fields and getters - Renderer: Remove redundant isFinished check (getActiveLayers already filters), remove unused getStats() method, clean up TODOs in effect methods - Update tests to match new LayerGroup constructor signature and filtering behavior
…g, DAW setup, and troubleshooting tips
…dd debug overlay for real-time monitoring
- Replace Math.random() strobe with timestamp-based deterministic flashing for consistent, reproducible VJ performance - Add try-catch to disconnectedCallback for error logging during cleanup - Move effect magic numbers to settings.effectParams for configurability - Add division-by-zero guard in AnimationLayer BPM calculation
|
Addressed 2 unresolved review threads:
|
…fensive checks for canvas contexts fix: prevent division by zero in BPM deviation calculation refactor: enhance DebugOverlay key handling and log entry timestamping
…erFrame handling and validation
…d outlier detection
Overview
Adds a comprehensive feature plan document for multi-layer VJ capabilities and establishes the foundation for advanced animation features.
What's Implemented
1. Build Pipeline Enhancements
bitDepth: 1, 2, 4, or 8beatsPerFramefor tempo-synced playbackbitDepthandbeatsPerFramefields2. Core Architecture
3. Visual Features
Feature Plan Document
The PR includes a detailed 760-line feature plan (
.github/prompts/features-plan.prompt.md) documenting:Testing
All core functionality tested with 100% coverage:
Performance
Breaking Changes
None - fully backwards compatible: