Refactor Scene - Final fixes

[firthm01]

• Intermittent crash when plugin is placed on a track

  • Cause: Initialisation order of the MetadataThread messages vector and thread, so occasionally the thread would attempt to access the vector before it had been initialised.
  • Fix: So as to not be dependent on class member ordering (which might get accidently rearranged in a tidy up) the thread is wrapped in a unique_ptr and constructed in the MetadataThread constructor body. thread is already stopped in MetadataThread destructor which ensures it will not access the messages vector as it is destroyed.

• Programmes not being added to Scene for some imports

  • Cause: If the first update from an input plugin already has it's correct routing applied, the logic in AutoModeController would not add it to the default programme. PendingStore was waiting to see everything it needs in the default programme before setting the store.
  • Fix: PendingStore should not be reliant on AutoModeController and the default programme. Now, it instead listens for any itemAdded or itemUpdated event (as an aside, the same logic was applied for missing elements in RestoredPendingStore too). The logic in AutoModeController was reworked, which essentially led to the "Refactor AutoModeController" task.

• Items which do not belong in the first programme sometimes get added to the first programme anyway during import

  • Cause: The ordering/timing of events and firing of listeners - because there are lots of threads going on, this can't be guaranteed. In some cases on import, the store would be set which would subsequently set AutoMode disabled. AutoModeController was listening for this event to toggle it's internal enabled state. Sometimes, an inputUpdated event would occur before the autoModeChanged listener was fired, so AutoModeController internal enabled state was still true, and it just stuck this item in the first programme anyway.
  • Fix: "Refactor AutoModeController" task

• Refactor AutoModeController

  • Issue: It is not ideal for AutoModeController to track item state or auto mode enabled state when this is already held by the store as it could lead to tricky bugs with timing (see above) and with state synchronisation.

  • Initial Fix: AutoModeController significantly stripped back. Some methods implemented in Metadata class for directly querying the store. AutoModeController only listens for inputAdded and inputUpdated events which fire a new addItemIfNecessary method. This checks the AutoMode enabled state directly from the store (Metadata::getAutoMode()), and will add the item only if it does not already exist in the selected programme (using Metadata::getSelectedProgrammeIndex() and Metadata::programmeHasElement(...)). It does not do any ordering. A skeleton method is waiting in Metadata (sortProgrammeElements) - this was not populated as it is to be discussed whether sorting is actually a good thing or more of a hinderance.

  • Final Fix: This fix does not require query methods to the store, since that also wasn't ideal. The AutoModeController is still significantly stripped back. It does not do any item reordering as we discussed this was probably no help and could in fact be a hindrance. AutoModeController just listens to inputAdded and this event was modified to pass the automode enablement state to listeners. AutoModeController does not need to concern itself with whether the active programme already contains the item, firstly due to the event type it is listening for, and secondly because addItemsToSelectedProgramme (via Metadata::doAddItemsToSelectedProgramme) already does the existence check, as it should never be allowed to happen anyway.

  • Items displayed in programmes do not reflect updated item state (e.g, name, colour, config). This can also leave "no name" items in programmes loaded from saved projects until the GUI is forcibly refreshed.

  • Cause: No mechanism for ObjectView to respond to item updates - they only get their initial state.

  • Fix: ProgrammesContainer::programmeItemUpdated listener retrieves the ElementsContainer for the programme and then the ObjectView for the item (via the new ElementsContainer::getObjectView(...) method). The existing code to set the ObjectView has been broken up in to two stages, responding to Data::object changes (i.e, interaction settings) and Data::item changes (i.e, object metadata). This reduces the amount of work done for a particular type of update. The new ObjectView::setInputItemMetadata(...) method only concerns the latter, and is called from ProgrammesContainer::programmeItemUpdated. repaint() is only called if changes were actually made.

  • An alternative approach would be to have every ObjectView inherit MetadataListener. This was decided against since it would make events harder to debug with many extra listeners, and the implemented method more closely resembles the mechanism implemented in ItemContainer already. Also, we would need to pass the MetadataDispatcher object down through the classes to apply the listeners which would be a little untidy.

• Switching to a blank programme still monitors previous programme

  • Issue: In the SceneStore::programmeSelected listener, the changed flag is only set by the setMonitoringItemFrom method (called via addMonitoringItem). If there are no items in the newly selected programme, this is not called so the changed flag is never set.
  • Fix: Always set changed flag in SceneStore::programmeSelected regardless of what the new programme contains.

• Programme delete dialog shows empty string for programme

  • Issue: Was using EarTabbedComponent::getName() which is actually just the JUCE base class method for getting the component name (if present)
  • Fix: Implement methods to get displayed name from tabs in EarTabbedComponent

• Adding a new item mid-playback does not make that item contribute to the monitoring mix

  • Issue: SceneStore::itemsAddedToProgramme did not set the overall changed flag, so triggerMetadataSend did not bother sending the update.
  • Fix: Flag set. Note that this flag was factored out in later fixes which introduced the itemsChangedSinceLastSend set.

• Related bugs: "Switching to a different programme whilst playback is stopped makes items from the previously selected programme audible in monitoring" and "Removing an item from a programme does not remove it from monitoring"

  • Issue: Items that were "new" to the SceneGainsCalculator but not changed (as reported by incoming metadata) were never added to routingCache, and therefore gains were not reset when those items were removed.
  • Fix: Refactor SceneGainsCalculator::update with comments to make the flow more logical and easier to understand. There is now a cachedIdsChecklist vector which is populated with every item we already know about. We then check these off as we go through items in the incoming metadata. Anything then remaining in cachedIdsChecklist must have been removed, and so we remove them from routingCache and reset gains for them (via removeItem call.) Anything marked as changed is also removed via removeItem to force it to be re-evaluated. Then we get to the actual processing of items, which is now greatly simplified - we iterate through items in the incoming metadata and see if it's in the routingCache - if not, it's processed via the new addOrUpdateItem method.

• Monitoring plugins not updating to new object static location if location was changed whilst stopped.

  • Issue: Metadata is only sent to the monitoring plugins during playback via processBlock. It refers to the state of the changed flag to determine whether to recalculate gains. However, we can not rely on the state of the changed flag sent in metadata from the input plugins, because not all of those updates reach the monitoring plugins. If the Input plugins send a follow-up when nothing has changed, the changed flag is reset to false and overwrites the item metadata in the Scene plugin. This is the one that gets sent to the monitoring plugins when playback starts. They see changed is false and don't recalculate gains even though it should actually be true from their perspective, because it has changed since they were last notified.
  • Fix: Solution is to track which items have changed since the last send to monitoring plugins (itemsChangedSinceLastSend set), and use that tracking to override changed flag state before sending on to monitoring plugins. Removes need for the overall changed state tracking too (count of items in itemsChangedSinceLastSend set being >0 tells you this.)

• Switching to a new, blank programme does not remove items from previous programme from the monitoring mix.

  • Issue/Fix: Initially corrected with the aforementioned routingCache fix
  • Issue: This bug was reintroduced during the itemsChangedSinceLastSend work - same symptoms, different cause - removed items were not being added to itemsChangedSinceLastSend
  • Fix: itemsChangedSinceLastSend is populated with everything about to be removed before calling store_.clear_monitoring_items()

• Selecting a programme monitors incorrect programme after programmes have been reordered.

  • Issue: With a 2-programme scene, if you reorder them and select the "other" programme, an event is triggered for programmeSelection, but the index matches the index last time that was fired, so no action is taken.
  • Fix: Programmes are identified by ID rather than index in the internal data model - this will avoid falling in to any traps in future caused by indices shifting around. See https://github.com/ebu/ear-production-suite/pull/200 for full break-down (squashed in to this PR)

• Editable text for EarSliders positions to wrong vertical location

  • Issue: Caused by JUCE 6 update where the default vertical position no longer seems to be "Top"
  • Fix: Explicitly set both vertical and horizontal text location. Also adjusted offsets to prevent the slight jump when you enter "edit" mode.

• Scene GUI seems quite hoggy on Protest ADM

  • Issue: Profiling revealed ElementOverview::paint using quite a lot of CPU time, so focused optimisations around there.
  • Fixes: Remove BG alpha - not required anyway. Render bottom-left orientation sphere as part of background image so it's not redrawn from individual drawing instructions on each paint.

• Meters in Scene are all mono

  • Issue: ProgrammesContainer::addObjectView had correct logic to set meter channel count initially, but then ProgrammesContainer::programmeItemUpdated had no channel-counting logic and just immediately reverted them all to single channel.
  • Fix: Consolidated the channel-counting logic in to a function that can be called from either ProgrammesContainer::addObjectView or ProgrammesContainer::programmeItemUpdated (or anywhere else we need it in future)

• Scenes bottom-left orientation representation rotates in the opposite direction to the main view

  • Issue: Pulling the wrong labels from array which makes it appear that the smaller view is rotating in the wrong direction. Due to getSectorIndex function returning incorrect label indices.
  • Fix: Correct return indices.

• Selecting any programme other than the first and then loading the Binaural plugin can cause a dead loop with rapid cycling of programmes.

  • Issue: Possible to set up multiple competing selectedProgramme events. In this case the problem was fired from a removeAllTabs call, which called removeTab multiple times. There was logic in removeTab code to select another programme via selectProgramme (this was wrong anyway - UI code should not make this decision). There is a loop in there where selectProgramme calls listeners, one of which calls selectTab, which calls selectProgramme. Normally the loop is broken when the second selectProgramme call realises that programme is already selected - but obviously this doesn't work when you have queued, competing events which are switching to other programmes.
  • Fix: First fix was to prevent removeTab selecting another programme. Instead, the UI responds to whatever the store code decides to select in that case. Second fix was to implement a juce::NotificationType parameter in selectTab, which prevents the calling of listeners (an therefore programmeSelected) if the notification type is dontSendNotification.

[firthm01]

• [x] To fix: Monitoring plugins still seem to get fed objects from other programmes when switching between them (intermittent issue, seen with DS items) d3e280b did not fix.

[firthm01]

• [x] To Fix: Clicking delete on a programme shows a blank string where the programme name should be in the confirmation dialog

[firthm01]

• [x] To fix: Selecting a programme monitors incorrect programme after programmes have been reordered. Seems to correct itself after a few selections.

[firthm01]

• [x] To fix: Adding a new item mid-playback does not make that item contribute to the monitoring mix

[firthm01]

• [x] To fix: Removing an item from a programme does not remove it from monitoring

[firthm01]

• [x] To Fix: Switching to a new, blank programme does not remove items from previous programme from the monitoring mix. (Likely related to "Removing an item from a programme does not remove it from monitoring".)

[firthm01]

• [x] To Fix: Monitoring plugins not updating. Reproducible steps:
• One static object in a programme. Set azimuth to 90.
• Play. Item in correct position. Stop.
• Set azimuth to -90 using the "text edit" method.
• Play. Item is still in old position.

[firthm01]

• [x] To Fix: Editable text for EarSliders positions to wrong vertical location (since JUCE 6 update)

[firthm01]

• [x] To Fix: Selecting any programme other than the first and then loading the Binaural plugin can cause a dead loop. If playing, the audio is rapidly switching between all programmes, the GUI fails to load, and REAPER is unresponsive and must be force-closed

[firthm01]

• [x] To Fix: Scene GUI seems quite hoggy on Protest ADM - investigate and optimise if easily possible

[firthm01]

• [x] To Fix: Meters in Scene are all mono

[firthm01]

• [x] To Fix: Scenes bottom-left orientation representation rotates in the opposite direction to the main view

[firthm01]

PR description updated with fixes for specific issues. Ready for review.