Skip to content

LFO2 sync draft#1978

Closed
nikodemus wants to merge 12 commits into
SynthstromAudible:communityfrom
nikodemus:lfo2-sync-draft
Closed

LFO2 sync draft#1978
nikodemus wants to merge 12 commits into
SynthstromAudible:communityfrom
nikodemus:lfo2-sync-draft

Conversation

@nikodemus

@nikodemus nikodemus commented May 20, 2024

Copy link
Copy Markdown
Collaborator

As per #813

TODO:

  • Figure out if LFO2 needs a resync like LFO1: right now the assumption is that since it retriggers with note-on, it's "good enough" without a resync. No idea if that's true. --> Decided that it's good enough for now at least: a drone with sync'ed LFO2 might experience some drift over time, though.
  • Figure out if LFO1 and LFO2 should set initial phase the same way. Currently LFO1 has more logic about this. Applying that would change LFO2 behaviour, but maybe doing it when sync'ed would be ok? --> Retain old behaviour for unsynced LFO2, match LFO1 behaviour when synced.
  • Cleanup: do we need resyncing setters for LFO2? (Do we actually need them for LFO1, could we just wait for the next natural resync?) --> Don't need them for LFO2, but should figure out a nicer way to handle them for LFO1.*
  • Implement initial phase behaviour matching LFO1 when synced.
  • Documentation.
  • Cleanup: we should be able to merge lfo::local::* and lfo::global::* classes.
  • Cleanup: nicer way to handle resyncing setters for LFO1. Do we need them, or could we wait for the next clock-driven resync instead? --> Clock based resync is still an option, but now there are less redundant calls.
  • Naming: localLFOConfig.waveType is a bit unwieldy. lfo1Config.waveType, etc, maybe? --> lfoConfig[] array, as part of the Rate/Sync/Type menu class unification.
  • Cleanup: should be able to merge logic for phase increment computation.
  • Cleanup / optimization: cache phase increment, instead of recomputing it every time. --> Nope, decided this is out of scope: there's more than a couple of cases when the cache would need to be invalidated, so if this is done it's better done separately.
  • Missing sync menu for LFO2.
  • More testing.

@nikodemus
nikodemus marked this pull request as draft May 20, 2024 21:14
@github-actions

github-actions Bot commented May 20, 2024

Copy link
Copy Markdown
Contributor

Test Results

28 tests   - 1   28 ✅  - 1   0s ⏱️ ±0s
 5 suites ±0    0 💤 ±0 
 5 files   ±0    0 ❌ ±0 

Results for commit c549b42. ± Comparison against base commit 3696aa0.

This pull request removes 5 and adds 4 tests. Note that renamed tests count towards both.
LFOTest ‑ renderGlobalSine
LFOTest ‑ renderGlobalTriangle
LFOTest ‑ renderLocalSine
LFOTest ‑ renderLocalTriangle
LFOTest ‑ renderSampleAndHold
LFOTest ‑ renderSyncedSine
LFOTest ‑ renderSyncedTriangle
LFOTest ‑ renderUnsyncedSine
LFOTest ‑ renderUnsyncedTriangle_noSync

♻️ This comment has been updated with latest results.

nikodemus added 11 commits May 21, 2024 18:56
* resyncGlobalLFO used to have a comment about how playbackhandler.isEitherClockActive()
  should true, and most call sites checked it was so--but not all.

  Locations which elided the check are all implementations of Output::resyncLFOs(),
  whose callers do check this.

* The check is a simple bit-and, so running it redundantly on occastion should not be a
  performance issue, given we're deleting more code than we're adding.
- LFO::init() folded into the default constructor.

- LFO::render() takes an LFOConfig, not LFOType.

- Rename resyncLFOsAndArpeggiators() to resyncLFOs() -- no arps there.

- Unify intial phase logic for synced LFOs into LFO::setInitialPhase()
- Store LFOConfigs in Sound in a array: the menu items can then
  index into that array, instead of using per-item classes.

- Instead of having global LFO setters resync, have the menu setters
  and save loading handle the resync explicitly: this also removes some
  unnecessary resyncs after setting a sync level or loading an LFO config.
@nikodemus

nikodemus commented May 23, 2024

Copy link
Copy Markdown
Collaborator Author

#2004 and #2005 supersede this.

@nikodemus nikodemus closed this May 23, 2024
@nikodemus
nikodemus deleted the lfo2-sync-draft branch June 1, 2024 09:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant