Support mute/solo/volume for tracks#19
Conversation
342000c to
9c0df38
Compare
|
Update: I've now implemented my first lit element for the track volume. The goal was not to be ultra generic here, but learning how to implement it. With this change we now have a volume slider that works. There is some duplicated code between knob.js and the new trackvolume.js for drag / pointer grab support |
abf79e1 to
4801503
Compare
Signed-off-by: Stefan Westerfeld <stefan@space.twc.de>
Signed-off-by: Stefan Westerfeld <stefan@space.twc.de>
Signed-off-by: Stefan Westerfeld <stefan@space.twc.de>
Signed-off-by: Stefan Westerfeld <stefan@space.twc.de>
Signed-off-by: Stefan Westerfeld <stefan@space.twc.de>
d32b9fe to
29e5bac
Compare
Signed-off-by: Stefan Westerfeld <stefan@space.twc.de>
Signed-off-by: Stefan Westerfeld <stefan@space.twc.de>
Signed-off-by: Stefan Westerfeld <stefan@space.twc.de>
Signed-off-by: Stefan Westerfeld <stefan@space.twc.de>
…ltip Signed-off-by: Stefan Westerfeld <stefan@space.twc.de>
Signed-off-by: Stefan Westerfeld <stefan@space.twc.de>
Signed-off-by: Stefan Westerfeld <stefan@space.twc.de>
Signed-off-by: Stefan Westerfeld <stefan@space.twc.de>
Signed-off-by: Stefan Westerfeld <stefan@space.twc.de>
|
Update: I've now migrated the PR to use Track properties instead of Track methods for mute / solo / volume. To support automation in the future, the properties that do support automation (mute and volume) are now part of the AudioProcessor and no longer part of the Track, so these can receive messages while playing. Its not sample accurate yet, but this should be fairly easy to add later on. The solo logic is a bit tricky because to know whether a track is muted or not, the track needs its own mute state, but also the information whether another track is solo, so this has kind of global state. Since we don't need to automate solo, I can compute the solo state is the non-RT code. Everything works as far as I can see. The trackvolume.js file has duplicated code with knob.js, this should be moved elsewhere. Other than that, let me know if you see something that you'd like to be improved for merging the PR. |
Signed-off-by: Stefan Westerfeld <stefan@space.twc.de>
|
@greptile |
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR implements initial mute/solo/volume support for tracks in Anklang, adding essential mixing controls to the DAW. The implementation spans both the backend audio engine and the frontend UI components. On the backend side, it extends the Track class with solo state management and integrates volume/mute/solo parameters into the AudioChain audio processor with proper smoothing to prevent audio clicks. The frontend introduces new UI components (trackbutton.js, trackvolume.js) that provide interactive controls for these mixing parameters and integrates them into the track view.
The architecture uses a property-based system to bridge the UI and backend, with property change notifications enabling reactive updates. The volume control implements cubic scaling for perceptually better volume curves and includes sophisticated drag interaction with pointer lock functionality. Solo functionality correctly handles the interdependency between tracks - when any track is soloed, all non-solo tracks are muted through a global state management system.
While functional, the developer acknowledges this as an interim solution. The current approach adds methods directly to the Track class rather than using the AudioProcessor parameter framework, which would be necessary for future automation support.
Changed Files
| Filename | Score | Overview |
|---|---|---|
| ase/properties.hh | 5/5 | Adds property change notifications by emitting notify signals when values are successfully set |
| ase/track.hh | 3/5 | Adds solo member variable and related methods, plus property access infrastructure for track controls |
| ui/b/trackview.js | 4/5 | Integrates mute/solo/volume UI controls into the track view with flexbox layout |
| ase/combo.hh | 4/5 | Adds volume/mute/solo parameter enums and volume smoothing infrastructure to AudioChain |
| ui/b/trackbutton.js | 2/5 | New component for mute/solo toggle buttons with async property binding and notification handling |
| ase/track.cc | 4/5 | Implements solo method with global state coordination and temporary property serialization |
| ase/combo.cc | 4/5 | Adds volume processing with cubic scaling, smooth transitions, and mute/solo audio logic |
| ui/b/trackvolume.js | 2/5 | New volume slider component with complex drag handling and property integration |
Confidence score: 3/5
- This PR introduces functional mixing controls but has several implementation gaps and potential runtime issues that require careful testing
- Score reflects incomplete UI synchronization, missing error handling in async property operations, potential infinite loops in notification systems, and acknowledged temporary architectural choices
- Pay close attention to ui/b/trackbutton.js and ui/b/trackvolume.js for async property handling, null pointer safety, and notification loop prevention
Sequence Diagram
sequenceDiagram
participant User
participant UI as UI Components (trackbutton.js, trackvolume.js)
participant Track as TrackImpl
participant AudioChain as AudioChain
participant AudioProcessor as AudioProcessor
participant DSP as DSP Engine
User->>UI: "Click mute/solo button"
UI->>Track: "access_property('mute'/'solo')"
Track->>Track: "create_properties()"
Track->>UI: "Return property object"
UI->>Track: "prop.set_value(new_value)"
Track->>Track: "solo(new_solo) or mute property setter"
Track->>Track: "set_solo_states()"
Track->>AudioChain: "send_param(SOLO_STATE, state_value)"
AudioChain->>AudioProcessor: "apply_event(param_event)"
AudioProcessor->>DSP: "Process audio with volume adjustment"
User->>UI: "Drag volume slider"
UI->>Track: "access_property('volume')"
Track->>AudioChain: "Return volume parameter"
UI->>AudioChain: "prop.set_normalized(volume_value)"
AudioChain->>AudioChain: "volume_smooth_.set(new_volume)"
AudioChain->>DSP: "Apply volume multiplication in render()"
DSP->>AudioChain: "Process audio frames with volume"
AudioChain->>Track: "Update telemetry (dbspl levels)"
Track->>UI: "Telemetry notifications"
UI->>User: "Update level meters visually"
8 files reviewed, 16 comments
| #define __ASE_COMBO_HH__ | ||
|
|
||
| #include <ase/processor.hh> | ||
| #include <devices/blepsynth/linearsmooth.hh> |
There was a problem hiding this comment.
style: Including a device-specific header for LinearSmooth creates a dependency on the blepsynth device. Consider moving LinearSmooth to a more general location like ase/ to avoid coupling AudioChain to specific device implementations.
Prompt To Fix With AI
This is a comment left during a code review.
Path: ase/combo.hh
Line: 6:6
Comment:
**style:** Including a device-specific header for LinearSmooth creates a dependency on the blepsynth device. Consider moving LinearSmooth to a more general location like `ase/` to avoid coupling AudioChain to specific device implementations.
How can I resolve this? If you propose a fix, please make it concise.| * The <b-trackvolume> element is an editor for the track volume, implemented as thin wrapper | ||
| * arount <b-toggle>. |
There was a problem hiding this comment.
logic: JSDoc description incorrectly mentions track volume and <b-trackvolume> but this component handles mute/solo buttons
Prompt To Fix With AI
This is a comment left during a code review.
Path: ui/b/trackbutton.js
Line: 9:10
Comment:
**logic:** JSDoc description incorrectly mentions track volume and `<b-trackvolume>` but this component handles mute/solo buttons
How can I resolve this? If you propose a fix, please make it concise.| let property; | ||
| if (this.label == "M") | ||
| property = "mute"; | ||
| if (this.label == "S") | ||
| property = "solo"; |
There was a problem hiding this comment.
logic: Property mapping has no default case - if label is neither 'M' nor 'S', property remains undefined causing line 67 to fail
Prompt To Fix With AI
This is a comment left during a code review.
Path: ui/b/trackbutton.js
Line: 62:66
Comment:
**logic:** Property mapping has no default case - if label is neither 'M' nor 'S', property remains undefined causing line 67 to fail
How can I resolve this? If you propose a fix, please make it concise.| this.prop.on ("notify", args => { | ||
| if (args.detail == property) | ||
| this.update_value(); | ||
| }); |
There was a problem hiding this comment.
logic: Potential infinite loop: property notify triggers update_value() which could cause recursive calls if property changes during get_value
Prompt To Fix With AI
This is a comment left during a code review.
Path: ui/b/trackbutton.js
Line: 69:72
Comment:
**logic:** Potential infinite loop: property notify triggers `update_value()` which could cause recursive calls if property changes during get_value
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
@greptile, How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
You can prevent the infinite loop by adding a flag to track when an update is already in progress:
| this.prop.on ("notify", args => { | |
| if (args.detail == property) | |
| this.update_value(); | |
| }); | |
| this.prop.on ("notify", args => { | |
| if (args.detail == property && !this.updating) | |
| this.update_value(); | |
| }); |
And modify the update_value method to set the flag:
| this.prop.on ("notify", args => { | |
| if (args.detail == property) | |
| this.update_value(); | |
| }); | |
| async update_value() | |
| { | |
| if (this.updating) return; | |
| this.updating = true; | |
| if (!this.prop) | |
| { | |
| let property; | |
| if (this.label == "M") | |
| property = "mute"; | |
| if (this.label == "S") | |
| property = "solo"; | |
| let prop = await this.track.access_property (property); | |
| this.prop = prop; | |
| this.prop.on ("notify", args => { | |
| if (args.detail == property && !this.updating) | |
| this.update_value(); | |
| }); | |
| } | |
| this.value = await this.prop.get_value(); | |
| this.updating = false; | |
| } |
This prevents recursive calls by ensuring only one update can happen at a time.
| // @ts-check | ||
|
|
||
| import { LitComponent, html, JsExtract, live, docs, ref } from '../little.js'; | ||
| import * as Util from '../util.js'; |
There was a problem hiding this comment.
style: Util import is unused and can be removed
Prompt To Fix With AI
This is a comment left during a code review.
Path: ui/b/trackbutton.js
Line: 5:5
Comment:
**style:** Util import is unused and can be removed
How can I resolve this? If you propose a fix, please make it concise.| this.prop.on ("notify", args => { | ||
| if (args.detail == "volume") | ||
| this.update_value(); | ||
| }); |
There was a problem hiding this comment.
logic: Recursive call to update_value() in event handler could cause infinite loop if property updates trigger more notifications
Prompt To Fix With AI
This is a comment left during a code review.
Path: ui/b/trackvolume.js
Line: 76:79
Comment:
**logic:** Recursive call to `update_value()` in event handler could cause infinite loop if property updates trigger more notifications
How can I resolve this? If you propose a fix, please make it concise.| element.addEventListener ('pointermove', spin_drag.pointermove); | ||
| element.addEventListener ('pointerup', spin_drag.stop); | ||
| element[SPIN_DRAG] = spin_drag; | ||
| Shell.data_bubble.force (element); |
There was a problem hiding this comment.
logic: Direct access to global Shell object without null check - component will fail if Shell is not initialized
Prompt To Fix With AI
This is a comment left during a code review.
Path: ui/b/trackvolume.js
Line: 150:150
Comment:
**logic:** Direct access to global `Shell` object without null check - component will fail if Shell is not initialized
How can I resolve this? If you propose a fix, please make it concise.| let dist = (s ? +1 : -1) * Math.sqrt (dx * dx + dy * dy) * spin_drag.ptraccel; | ||
| // convert to physical pixel movements, so knob behaviour is unrelated to display resolution | ||
| if (!has_ptrlock || | ||
| (has_ptrlock && CONFIG.dpr_movement)) |
There was a problem hiding this comment.
logic: Global CONFIG object accessed without null/undefined check
Prompt To Fix With AI
This is a comment left during a code review.
Path: ui/b/trackvolume.js
Line: 230:230
Comment:
**logic:** Global `CONFIG` object accessed without null/undefined check
How can I resolve this? If you propose a fix, please make it concise.| return true; // spin drag started | ||
| } | ||
|
|
||
| /** Stop sping drag event handlers and pointer grab. |
There was a problem hiding this comment.
syntax: Typo in comment: 'sping' should be 'spin'
Prompt To Fix With AI
This is a comment left during a code review.
Path: ui/b/trackvolume.js
Line: 154:154
Comment:
**syntax:** Typo in comment: 'sping' should be 'spin'
How can I resolve this? If you propose a fix, please make it concise.| Shell.data_bubble.unforce (element); | ||
| } | ||
|
|
||
| /** Handle sping drag pointer motion. |
There was a problem hiding this comment.
syntax: Typo in comment: 'sping' should be 'spin'
Prompt To Fix With AI
This is a comment left during a code review.
Path: ui/b/trackvolume.js
Line: 182:182
Comment:
**syntax:** Typo in comment: 'sping' should be 'spin'
How can I resolve this? If you propose a fix, please make it concise.
This is my first attempt to implement mute/solo/volume support for tracks.
What is good:
What is incomplete:
Other issues:
Right now the mute/solo/volume interface between the UI and core is done by adding methods to the track. However, these values should support automation in the future. So these values really looks like
AudioProcessor parameters. These have a minimum, maximum, default, non-linear mapping between UI values and actual DSP values, the ability to enter a value directly, and these are serialized automatically by the framework. However, as far as I can see Track is not an AudioProcessor, so I cannot make these parameters here.
Btw, panning isn't implemented yet because we don't have a mixer view, it should be added once we have that.