#13 Prevent echo feedback - final implementation

helgoboss · helgoboss · commit f1b9df9e28ee · 2020-08-19T22:36:26.000+02:00
Now works within one mapping only. Much more straightforward,
no hidden pitfalls, also easier to implement. In future this
could still be expanded to affect multiple mappings, if
absolutely necessary.
diff --git a/main/src/application/mapping_model_data.rs b/main/src/application/mapping_model_data.rs
@@ -12,6 +12,7 @@ pub struct MappingModelData {
     target: TargetModelData,
     control_is_enabled: bool,
     feedback_is_enabled: bool,
+    prevent_echo_feedback: bool,
 }
 
 impl Default for MappingModelData {
@@ -23,6 +24,7 @@ impl Default for MappingModelData {
             target: Default::default(),
             control_is_enabled: true,
             feedback_is_enabled: true,
+            prevent_echo_feedback: false,
         }
     }
 }
@@ -36,6 +38,7 @@ impl MappingModelData {
             target: TargetModelData::from_model(&model.target_model, context),
             control_is_enabled: model.control_is_enabled.get(),
             feedback_is_enabled: model.feedback_is_enabled.get(),
+            prevent_echo_feedback: model.prevent_echo_feedback.get(),
         }
     }
 
@@ -57,5 +60,8 @@ impl MappingModelData {
         model
             .feedback_is_enabled
             .set_without_notification(self.feedback_is_enabled);
+        model
+            .prevent_echo_feedback
+            .set_without_notification(self.prevent_echo_feedback);
     }
 }
diff --git a/main/src/application/target_model_data.rs b/main/src/application/target_model_data.rs
@@ -139,7 +139,7 @@ impl TargetModelData {
                             "Track not found by GUID {} and name {}, falling back to <This>",
                             guid.to_string_with_braces(),
                             name.map(|n| format!("\"{}\"", n))
-                                .unwrap_or("-".to_string())
+                                .unwrap_or_else(|| "-".to_string())
                         )),
                     }
                     VirtualTrack::This
diff --git a/main/src/domain/main_processor.rs b/main/src/domain/main_processor.rs
@@ -10,10 +10,7 @@ use reaper_medium::ControlSurface;
 use rxrust::prelude::*;
 use slog::debug;
 use smallvec::SmallVec;
-use std::cell::RefCell;
-use std::collections::hash_map::Entry;
 use std::collections::{HashMap, HashSet};
-use std::time::{Duration, Instant};
 
 const NORMAL_TASK_BULK_SIZE: usize = 32;
 const FEEDBACK_TASK_BULK_SIZE: usize = 32;
@@ -28,7 +25,6 @@ pub struct MainProcessor {
     mappings: HashMap<MappingId, MainProcessorMapping>,
     feedback_buffer: FeedbackBuffer,
     feedback_subscriptions: FeedbackSubscriptions,
-    ephemeral_source_cache: EphemeralSourceCache,
     self_feedback_sender: crossbeam_channel::Sender<FeedbackMainTask>,
     normal_task_receiver: crossbeam_channel::Receiver<NormalMainTask>,
     feedback_task_receiver: crossbeam_channel::Receiver<FeedbackMainTask>,
@@ -57,7 +53,6 @@ impl ControlSurface for MainProcessor {
                         Reaper::get().logger(),
                         "Main processor: Updating all mappings..."
                     );
-                    self.ephemeral_source_cache.clear();
                     let mut unused_sources = self.currently_feedback_enabled_sources();
                     // Put into hash map in order to quickly look up mappings by ID
                     self.mappings = mappings
@@ -97,7 +92,6 @@ impl ControlSurface for MainProcessor {
                         "Main processor: Updating mapping {:?}...",
                         mapping.id()
                     );
-                    self.ephemeral_source_cache.clear();
                     // (Re)subscribe to or unsubscribe from feedback
                     match mapping.target() {
                         Some(target) if mapping.feedback_is_enabled() => {
@@ -169,11 +163,7 @@ impl ControlSurface for MainProcessor {
                         // selected") and the real-time processor doesn't yet know about it, there
                         // might be a short amount of time where we still receive control
                         // statements. We filter them here.
-                        if m.control_is_enabled() {
-                            // TODO-high Only add if prevent immediate feedback enabled
-                            self.ephemeral_source_cache.add(m.source().clone());
-                            m.control(value);
-                        }
+                        m.control_if_enabled(value);
                     };
                 }
             }
@@ -196,11 +186,6 @@ impl ControlSurface for MainProcessor {
         if let Some(mapping_ids) = self.feedback_buffer.poll() {
             let source_values = mapping_ids.iter().filter_map(|mapping_id| {
                 let mapping = self.mappings.get(mapping_id)?;
-                if mapping.feedback_is_enabled()
-                    && self.ephemeral_source_cache.contains(mapping.source())
-                {
-                    return None;
-                }
                 mapping.feedback_if_enabled()
             });
             self.send_feedback(source_values);
@@ -226,7 +211,6 @@ impl MainProcessor {
             feedback_buffer: Default::default(),
             feedback_subscriptions: Default::default(),
             session,
-            ephemeral_source_cache: Default::default(),
         }
     }
 
@@ -379,43 +363,3 @@ impl Drop for MainProcessor {
         debug!(Reaper::get().logger(), "Dropping main processor...");
     }
 }
-
-const EPHEMERAL_SOURCE_CACHE_LIFESPAN: Duration = Duration::from_millis(20);
-
-/// Allows to add sources and keeps them just for a few milliseconds.
-#[derive(Debug, Default)]
-struct EphemeralSourceCache {
-    entries: RefCell<HashMap<MidiSource, Instant>>,
-}
-
-impl EphemeralSourceCache {
-    /// Adds the given source
-    pub fn add(&mut self, source: MidiSource) {
-        self.entries.borrow_mut().insert(source, Instant::now());
-    }
-
-    /// Checks if the given source is in this cache and not yet expired.
-    ///
-    /// Also removes the cache entry if expired.
-    pub fn contains(&self, source: &MidiSource) -> bool {
-        use Entry::*;
-        let expired = {
-            if let Some(source) = self.entries.borrow().get(source) {
-                source.elapsed() > EPHEMERAL_SOURCE_CACHE_LIFESPAN
-            } else {
-                return false;
-            }
-        };
-        if expired {
-            self.entries.borrow_mut().remove(source);
-            false
-        } else {
-            true
-        }
-    }
-
-    /// Clears the cache
-    fn clear(&mut self) {
-        self.entries.borrow_mut().clear();
-    }
-}
diff --git a/main/src/domain/mapping.rs b/main/src/domain/mapping.rs
@@ -2,6 +2,7 @@ use crate::domain::{MainProcessorTargetUpdate, Mode, ReaperTarget};
 use helgoboss_learn::{ControlValue, MidiSource, MidiSourceValue, Target};
 use helgoboss_midi::RawShortMessage;
 
+use std::time::{Duration, Instant};
 use uuid::Uuid;
 
 #[derive(Debug)]
@@ -12,6 +13,7 @@ pub struct ProcessorMapping {
     target: Option<ReaperTarget>,
     control_is_enabled: bool,
     feedback_is_enabled: bool,
+    prevent_echo_feedback: bool,
 }
 
 impl ProcessorMapping {
@@ -22,6 +24,7 @@ impl ProcessorMapping {
         target: Option<ReaperTarget>,
         control_is_enabled: bool,
         feedback_is_enabled: bool,
+        prevent_echo_feedback: bool,
     ) -> ProcessorMapping {
         ProcessorMapping {
             id,
@@ -30,6 +33,7 @@ impl ProcessorMapping {
             target,
             control_is_enabled,
             feedback_is_enabled,
+            prevent_echo_feedback,
         }
     }
 
@@ -46,6 +50,7 @@ impl ProcessorMapping {
             self.target.clone(),
             self.control_is_enabled,
             self.feedback_is_enabled && feedback_is_globally_enabled,
+            self.prevent_echo_feedback,
         );
         (real_time_mapping, main_mapping)
     }
@@ -109,6 +114,8 @@ impl RealTimeProcessorMapping {
     }
 }
 
+const MAX_ECHO_FEEDBACK_DELAY: Duration = Duration::from_millis(20);
+
 #[derive(Debug)]
 pub struct MainProcessorMapping {
     id: MappingId,
@@ -117,6 +124,8 @@ pub struct MainProcessorMapping {
     target: Option<ReaperTarget>,
     control_is_enabled: bool,
     feedback_is_enabled: bool,
+    prevent_echo_feedback: bool,
+    time_of_last_control: Option<Instant>,
 }
 
 impl MainProcessorMapping {
@@ -125,16 +134,19 @@ impl MainProcessorMapping {
         source: MidiSource,
         mode: Mode,
         target: Option<ReaperTarget>,
-        control: bool,
-        feedback: bool,
+        control_is_enabled: bool,
+        feedback_is_enabled: bool,
+        prevent_echo_feedback: bool,
     ) -> MainProcessorMapping {
         MainProcessorMapping {
             id,
             source,
             mode,
             target,
-            control_is_enabled: control,
-            feedback_is_enabled: feedback,
+            control_is_enabled,
+            feedback_is_enabled,
+            prevent_echo_feedback,
+            time_of_last_control: None,
         }
     }
 
@@ -165,12 +177,18 @@ impl MainProcessorMapping {
         self.feedback_is_enabled
     }
 
-    pub fn control(&mut self, value: ControlValue) {
+    pub fn control_if_enabled(&mut self, value: ControlValue) {
+        if !self.control_is_enabled {
+            return;
+        }
         let target = match &self.target {
             None => return,
             Some(t) => t,
         };
         if let Some(final_value) = self.mode.control(value, target) {
+            if self.prevent_echo_feedback {
+                self.time_of_last_control = Some(Instant::now());
+            }
             target.control(final_value).unwrap();
         }
     }
@@ -179,6 +197,11 @@ impl MainProcessorMapping {
         if !self.feedback_is_enabled {
             return None;
         }
+        if let Some(t) = self.time_of_last_control {
+            if t.elapsed() <= MAX_ECHO_FEEDBACK_DELAY {
+                return None;
+            }
+        }
         let target = match &self.target {
             None => return None,
             Some(t) => t,
diff --git a/main/src/domain/mapping_model.rs b/main/src/domain/mapping_model.rs
@@ -15,6 +15,7 @@ pub struct MappingModel {
     pub name: Prop<String>,
     pub control_is_enabled: Prop<bool>,
     pub feedback_is_enabled: Prop<bool>,
+    pub prevent_echo_feedback: Prop<bool>,
     pub source_model: MidiSourceModel,
     pub mode_model: ModeModel,
     pub target_model: TargetModel,
@@ -27,6 +28,7 @@ impl Clone for MappingModel {
             name: self.name.clone(),
             control_is_enabled: self.control_is_enabled.clone(),
             feedback_is_enabled: self.feedback_is_enabled.clone(),
+            prevent_echo_feedback: self.prevent_echo_feedback.clone(),
             source_model: self.source_model.clone(),
             mode_model: self.mode_model.clone(),
             target_model: self.target_model.clone(),
@@ -41,6 +43,7 @@ impl Default for MappingModel {
             name: Default::default(),
             control_is_enabled: prop(true),
             feedback_is_enabled: prop(true),
+            prevent_echo_feedback: prop(false),
             source_model: Default::default(),
             mode_model: Default::default(),
             target_model: Default::default(),
@@ -107,6 +110,7 @@ impl MappingModel {
             .merge(self.target_model.changed())
             .merge(self.control_is_enabled.changed())
             .merge(self.feedback_is_enabled.changed())
+            .merge(self.prevent_echo_feedback.changed())
     }
 }
 
@@ -133,6 +137,7 @@ impl<'a> MappingModelWithContext<'a> {
             target,
             self.mapping.control_is_enabled.get() && target_conditions_are_met,
             self.mapping.feedback_is_enabled.get() && target_conditions_are_met,
+            self.mapping.prevent_echo_feedback.get(),
         )
     }
 
diff --git a/main/src/infrastructure/common/bindings.rs b/main/src/infrastructure/common/bindings.rs
@@ -120,6 +120,7 @@ pub mod root {
     pub const ID_SOURCE_MIDI_MESSAGE_TYPE_LABEL_TEXT: u32 = 40074;
     pub const ID_OK: u32 = 40075;
     pub const ID_TARGET_ACTION_LABEL_TEXT: u32 = 40076;
+    pub const ID_MAPPING_PREVENT_ECHO_FEEDBACK_CHECK_BOX: u32 = 40078;
     pub const ID_CMD: u32 = 236;
     pub const ID_PSRESTARTWINDOWS: u32 = 2;
     pub const ID_PSREBOOTSYSTEM: u32 = 3;
diff --git a/main/src/infrastructure/common/realearn.rc b/main/src/infrastructure/common/realearn.rc
@@ -118,7 +118,7 @@ BEGIN
     AUTOCHECKBOX    "Rotate", ID_SETTINGS_ROTATE_CHECK_BOX, 124, 317, 37, 8, 0, WS_EX_LEFT
     PUSHBUTTON      "Pick", ID_TARGET_PICK_ACTION_BUTTON, 410, 95, 26, 14, 0, WS_EX_LEFT
     LTEXT           "Action name", ID_TARGET_ACTION_LABEL_TEXT, 220, 99, 189, 9, SS_LEFT, WS_EX_LEFT
-    AUTOCHECKBOX    "Prevent feedback echo", ID_MAPPING_CONTROL_ENABLED_CHECK_BOX, 348, 18, 91, 8, 0, WS_EX_LEFT
+    AUTOCHECKBOX    "Prevent echo feedback", ID_MAPPING_PREVENT_ECHO_FEEDBACK_CHECK_BOX, 348, 18, 91, 8, 0, WS_EX_LEFT
 END
 
 
diff --git a/main/src/infrastructure/common/resource.h b/main/src/infrastructure/common/resource.h
@@ -113,3 +113,4 @@
 #define ID_SOURCE_MIDI_MESSAGE_TYPE_LABEL_TEXT  40074
 #define ID_OK                                   40075
 #define ID_TARGET_ACTION_LABEL_TEXT             40076
+#define ID_MAPPING_PREVENT_ECHO_FEEDBACK_CHECK_BOX 40078
diff --git a/main/src/infrastructure/ui/mapping_panel.rs b/main/src/infrastructure/ui/mapping_panel.rs
@@ -353,6 +353,14 @@ impl<'a> MutableMappingPanel<'a> {
         );
     }
 
+    fn update_mapping_prevent_echo_feedback(&mut self) {
+        self.mapping.prevent_echo_feedback.set(
+            self.view
+                .require_control(root::ID_MAPPING_PREVENT_ECHO_FEEDBACK_CHECK_BOX)
+                .is_checked(),
+        );
+    }
+
     fn update_mapping_name(&mut self) -> Result<(), &'static str> {
         let value = self
             .view
@@ -866,6 +874,7 @@ impl<'a> ImmutableMappingPanel<'a> {
         self.invalidate_mapping_name_edit_control();
         self.invalidate_mapping_control_enabled_check_box();
         self.invalidate_mapping_feedback_enabled_check_box();
+        self.invalidate_mapping_prevent_echo_feedback_check_box();
         self.invalidate_source_controls();
         self.invalidate_target_controls();
         self.invalidate_mode_controls();
@@ -907,6 +916,13 @@ impl<'a> ImmutableMappingPanel<'a> {
         cb.set_text(format!("{} Feedback enabled", symbols::ARROW_LEFT_SYMBOL));
     }
 
+    fn invalidate_mapping_prevent_echo_feedback_check_box(&self) {
+        let cb = self
+            .view
+            .require_control(root::ID_MAPPING_PREVENT_ECHO_FEEDBACK_CHECK_BOX);
+        cb.set_checked(self.mapping.prevent_echo_feedback.get());
+    }
+
     fn invalidate_source_controls(&self) {
         self.invalidate_source_control_appearance();
         self.invalidate_source_type_combo_box();
@@ -1423,6 +1439,10 @@ impl<'a> ImmutableMappingPanel<'a> {
             .when_do_sync(self.mapping.feedback_is_enabled.changed(), |view| {
                 view.invalidate_mapping_feedback_enabled_check_box();
             });
+        self.panel
+            .when_do_sync(self.mapping.prevent_echo_feedback.changed(), |view| {
+                view.invalidate_mapping_prevent_echo_feedback_check_box();
+            });
     }
 
     fn register_source_listeners(&self) {
@@ -2072,6 +2092,9 @@ impl View for MappingPanel {
             ID_MAPPING_FEEDBACK_ENABLED_CHECK_BOX => {
                 self.write(|p| p.update_mapping_feedback_enabled())
             }
+            ID_MAPPING_PREVENT_ECHO_FEEDBACK_CHECK_BOX => {
+                self.write(|p| p.update_mapping_prevent_echo_feedback())
+            }
             ID_MAPPING_FIND_IN_LIST_BUTTON => {
                 self.scroll_to_mapping_in_main_panel();
             }

Original file line number	Diff line number	Diff line change
`@@ -12,6 +12,7 @@ pub struct MappingModelData {`
`12`	`12`	`target: TargetModelData,`
`13`	`13`	`control_is_enabled: bool,`
`14`	`14`	`feedback_is_enabled: bool,`
	`15`	`+ prevent_echo_feedback: bool,`
`15`	`16`	`}`
`16`	`17`
`17`	`18`	`impl Default for MappingModelData {`
`@@ -23,6 +24,7 @@ impl Default for MappingModelData {`
`23`	`24`	`target: Default::default(),`
`24`	`25`	`control_is_enabled: true,`
`25`	`26`	`feedback_is_enabled: true,`
	`27`	`+ prevent_echo_feedback: false,`
`26`	`28`	`}`
`27`	`29`	`}`
`28`	`30`	`}`
`@@ -36,6 +38,7 @@ impl MappingModelData {`
`36`	`38`	`target: TargetModelData::from_model(&model.target_model, context),`
`37`	`39`	`control_is_enabled: model.control_is_enabled.get(),`
`38`	`40`	`feedback_is_enabled: model.feedback_is_enabled.get(),`
	`41`	`+ prevent_echo_feedback: model.prevent_echo_feedback.get(),`
`39`	`42`	`}`
`40`	`43`	`}`
`41`	`44`
`@@ -57,5 +60,8 @@ impl MappingModelData {`
`57`	`60`	`model`
`58`	`61`	`.feedback_is_enabled`
`59`	`62`	`.set_without_notification(self.feedback_is_enabled);`
	`63`	`+ model`
	`64`	`+ .prevent_echo_feedback`
	`65`	`+ .set_without_notification(self.prevent_echo_feedback);`
`60`	`66`	`}`
`61`	`67`	`}`
Original file line number	Diff line number	Diff line change
`@@ -139,7 +139,7 @@ impl TargetModelData {`
`139`	`139`	`"Track not found by GUID {} and name {}, falling back to <This>",`
`140`	`140`	`guid.to_string_with_braces(),`
`141`	`141`	`name.map(\|n\| format!("\"{}\"", n))`
`142`		`- .unwrap_or("-".to_string())`
	`142`	`+ .unwrap_or_else(\|\| "-".to_string())`
`143`	`143`	`)),`
`144`	`144`	`}`
`145`	`145`	`VirtualTrack::This`