Instrument editor undo (minus FixedQueue change) (#2094)

* add undo to instrument editor (check for diffs on the current DivInstrument in insEdit, record them in a stack)

* style fixes

* accidentally left some logs in

* typo in style fix

* cheat to avoid warning -Werror=class-memaccess on linux

* warn instead of assert on case where MemPatch application would exceed size of target buffer (which should never happen, if you're applying the patch to the same type it was generated from)

* instrument editor undo: don't check delta if no user input has come in that could potentially have dirtied the editor

* don't run a delta against cached instrument if not insEditOpen

* revert fixedQueue to before my 'fix' (if i touch it again i'll add unit tests)

* explicitly cast to (DivInstrumentPOD*) when memsetting DivInstrumentPOD in DivInstrument constructor, rather than relying on implicit memory layout

* use delete[] instead of free (whoops)

* MemPatch/DivInstrumentUndoStep -- remove clear() function (ambiguous whether it should free data, it only existed to set data to null after the swap, so just do that directly now). Also set data to null after delete.

* DivInstrument -- fix dangling undo-step pointers being created on duplicate (potentially leading to use-after-free), fix undo-step objects being shamelessly leaked

---------

Co-authored-by: Adam Lederer <adam@adamlederer.com>

src/engine/instrument.cpp

@@ -363,6 +363,141 @@ void DivInstrument::writeFeatureFM(SafeWriter* w, bool fui) {
   FEATURE_END;
 }
 
+bool MemPatch::calcDiff(const void* pre, const void* post, size_t inputSize) {
+  bool diffValid=false;
+  size_t firstDiff=0;
+  size_t lastDiff=0;
+  const unsigned char* preBytes=(const unsigned char*)pre;
+  const unsigned char* postBytes=(const unsigned char*)post;
+
+  // @NOTE: consider/profile using a memcmp==0 check to early-out, if it's potentially faster
+  // for the common case, which is no change
+  for (size_t ii=0; ii<inputSize; ++ii) {
+    if (preBytes[ii] != postBytes[ii]) {
+      lastDiff=ii;
+      firstDiff=diffValid ? firstDiff : ii;
+      diffValid=true;
+    }
+  }
+
+  if (diffValid) {
+    offset=firstDiff;
+    size=lastDiff - firstDiff + 1;
+    data=new unsigned char[size];
+
+    // the diff is to make pre into post (MemPatch is general, not specific to
+    // undo), so copy from postBytes
+    memcpy(data, postBytes+offset, size);
+  }
+
+  return diffValid;
+}
+
+void MemPatch::applyAndReverse(void* target, size_t targetSize) {
+  if (size==0) return;
+  if (offset+size>targetSize) {
+    logW("MemPatch (offset %d, size %d) exceeds target size (%d), can't apply!",offset,size,targetSize);
+    return;
+  }
+
+  unsigned char* targetBytes=(unsigned char*)target;
+
+  // swap this->data and its segment on target
+  for (size_t ii=0; ii<size; ++ii) {
+    unsigned char tmp=targetBytes[offset+ii];
+    targetBytes[offset+ii] = data[ii];
+    data[ii] = tmp;
+  }
+}
+
+void DivInstrumentUndoStep::applyAndReverse(DivInstrument* target) {
+  if (nameValid) {
+    name.swap(target->name);
+  }
+  podPatch.applyAndReverse((DivInstrumentPOD*)target, sizeof(DivInstrumentPOD));
+}
+
+bool DivInstrumentUndoStep::makeUndoPatch(size_t processTime_, const DivInstrument* pre, const DivInstrument* post) {
+  processTime=processTime_;
+
+  // create the patch that will make post into pre
+  podPatch.calcDiff((const DivInstrumentPOD*)post, (const DivInstrumentPOD*)pre, sizeof(DivInstrumentPOD));
+  if (pre->name!=post->name) {
+    nameValid=true;
+    name=pre->name;
+  }
+
+  return nameValid || podPatch.isValid();
+}
+
+bool DivInstrument::recordUndoStepIfChanged(size_t processTime, const DivInstrument* old) {
+  DivInstrumentUndoStep step;
+
+  // generate a patch to go back to old
+  if (step.makeUndoPatch(processTime, old, this)) {
+    
+      // make room
+    if (undoHist.size()>=undoHist.capacity()) {
+      delete undoHist.front();
+      undoHist.pop_front();
+    }
+
+    // clear redo
+    while (!redoHist.empty()) {
+      delete redoHist.back();
+      redoHist.pop_back();
+    }
+
+    DivInstrumentUndoStep* stepPtr=new DivInstrumentUndoStep;
+    *stepPtr=step;
+    step.podPatch.data=NULL; // don't let it delete the data ptr that's been copied!
+    undoHist.push_back(stepPtr);
+
+    // logI("DivInstrument::undoHist push (%u off, %u size)", stepPtr->podPatch.offset, stepPtr->podPatch.size);
+    return true;
+  }
+
+  return false;
+}
+
+int DivInstrument::undo() {
+  if (undoHist.empty()) return 0;
+
+  DivInstrumentUndoStep* step=undoHist.back();
+  undoHist.pop_back();
+  // logI("DivInstrument::undo (%u off, %u size)", step->podPatch.offset, step->podPatch.size);
+  step->applyAndReverse(this);
+
+  // make room
+  if (redoHist.size()>=redoHist.capacity()) {
+      DivInstrumentUndoStep* step=redoHist.front();
+      delete step;
+      redoHist.pop_front();
+  }
+  redoHist.push_back(step);
+
+  return 1;
+}
+
+int DivInstrument::redo() {
+  if (redoHist.empty()) return 0;
+
+  DivInstrumentUndoStep* step = redoHist.back();
+  redoHist.pop_back();
+  // logI("DivInstrument::redo (%u off, %u size)", step->podPatch.offset, step->podPatch.size);
+  step->applyAndReverse(this);
+
+  // make room
+  if (undoHist.size()>=undoHist.capacity()) {
+      DivInstrumentUndoStep* step=undoHist.front();
+      delete step;
+      undoHist.pop_front();
+  }
+  undoHist.push_back(step);
+
+  return 1;
+}
+
 void DivInstrument::writeMacro(SafeWriter* w, const DivInstrumentMacro& m) {
   if (!m.len) return;
 
@@ -3327,3 +3462,28 @@ bool DivInstrument::saveDMP(const char* path) {
   w->finish();
   return true;
 }
+
+DivInstrument::~DivInstrument() {
+  // free undoHist/redoHist
+  while (!undoHist.empty()) {
+    delete undoHist.back();
+    undoHist.pop_back();
+  }
+  while (!redoHist.empty()) {
+    delete redoHist.back();
+    redoHist.pop_back();
+  }
+}
+
+DivInstrument::DivInstrument( const DivInstrument& ins ) {
+  // undo/redo history is specifically not copied
+  *(DivInstrumentPOD*)this=ins;
+  name=ins.name;
+}
+
+DivInstrument& DivInstrument::operator=( const DivInstrument& ins ) {
+  // undo/redo history is specifically not copied
+  *(DivInstrumentPOD*)this=ins;
+  name=ins.name;
+  return *this;
+}

src/engine/instrument.h

@@ -23,8 +23,10 @@
 #include "dataErrors.h"
 #include "../ta-utils.h"
 #include "../pch.h"
+#include "../fixedQueue.h"
 
 struct DivSong;
+struct DivInstrument;
 
 // NOTICE!
 // before adding new instrument types to this struct, please ask me first.
@@ -860,8 +862,7 @@ struct DivInstrumentSID2 {
     noiseMode(0) {}
 };
 
-struct DivInstrument {
-  String name;
+struct DivInstrumentPOD {
   DivInstrumentType type;
   DivInstrumentFM fm;
   DivInstrumentSTD std;
@@ -880,6 +881,77 @@ struct DivInstrument {
   DivInstrumentPowerNoise powernoise;
   DivInstrumentSID2 sid2;
 
+  DivInstrumentPOD() :
+    type(DIV_INS_FM) {
+  }
+};
+
+struct MemPatch {
+  MemPatch() :
+    data(NULL)
+    , offset(0)
+    , size(0) {
+  }
+
+  ~MemPatch() {
+    if (data) {
+      delete[] data;
+      data=NULL;
+    }
+  }
+
+  bool calcDiff(const void* pre, const void* post, size_t size);
+  void applyAndReverse(void* target, size_t inputSize);
+  bool isValid() const { return size>0; }
+
+  unsigned char* data;
+  size_t offset;
+  size_t size;
+};
+
+struct DivInstrumentUndoStep {
+  DivInstrumentUndoStep() :
+    name(""),
+    nameValid(false),
+    processTime(0) {
+  }
+
+  MemPatch podPatch;
+  String name;
+  bool nameValid;
+  size_t processTime;
+
+  void applyAndReverse(DivInstrument* target);
+  bool makeUndoPatch(size_t processTime_, const DivInstrument* pre, const DivInstrument* post);
+};
+
+struct DivInstrument : DivInstrumentPOD {
+  String name;
+
+  DivInstrument() :
+    name("") {
+      // clear and construct DivInstrumentPOD so it doesn't have any garbage in the padding
+      memset((unsigned char*)(DivInstrumentPOD*)this, 0, sizeof(DivInstrumentPOD));
+      new ((DivInstrumentPOD*)this) DivInstrumentPOD;
+  }
+
+  ~DivInstrument();
+
+  /**
+   * copy/assignment to specifically avoid leaking or dangling pointers to undo step
+   */
+  DivInstrument( const DivInstrument& ins );
+  DivInstrument& operator=( const DivInstrument& ins );
+
+  /**
+   * undo stuff
+   */
+  FixedQueue<DivInstrumentUndoStep*, 128> undoHist;
+  FixedQueue<DivInstrumentUndoStep*, 128> redoHist;
+  bool recordUndoStepIfChanged(size_t processTime, const DivInstrument* old);
+  int undo();
+  int redo();
+
   /**
    * these are internal functions.
    */
@@ -964,9 +1036,5 @@ struct DivInstrument {
    * @return whether it was successful.
    */
   bool saveDMP(const char* path);
-  DivInstrument():
-    name(""),
-    type(DIV_INS_FM) {
-  }
 };
 #endif

src/fixedQueue.h

@@ -42,6 +42,7 @@ template<typename T, size_t items> struct FixedQueue {
   void clear();
   bool empty();
   size_t size();
+  size_t capacity();
   FixedQueue():
     readPos(0),
     writePos(0) {}
@@ -177,4 +178,8 @@ template <typename T, size_t items> size_t FixedQueue<T,items>::size() {
   return writePos-readPos;
 }
 
+template <typename T, size_t items> size_t FixedQueue<T,items>::capacity() {
+  return items-1;
+}
+
 #endif

src/gui/doAction.cpp

@@ -73,6 +73,8 @@ void FurnaceGUI::doAction(int what) {
     case GUI_ACTION_UNDO:
       if (curWindow==GUI_WINDOW_SAMPLE_EDIT) {
         doUndoSample();
+      } else if (curWindow==GUI_WINDOW_INS_EDIT) {
+        doUndoInstrument();
       } else {
         doUndo();
       }
@@ -80,6 +82,8 @@ void FurnaceGUI::doAction(int what) {
     case GUI_ACTION_REDO:
       if (curWindow==GUI_WINDOW_SAMPLE_EDIT) {
         doRedoSample();
+      } else if (curWindow==GUI_WINDOW_INS_EDIT) {
+        doRedoInstrument();
       } else {
         doRedo();
       }

src/gui/gui.cpp

@@ -3739,6 +3739,7 @@ bool FurnaceGUI::loop() {
       ImGui::GetIO().AddKeyEvent(ImGuiKey_Backspace,false);
       injectBackUp=false;
     }
+
     while (SDL_PollEvent(&ev)) {
       WAKE_UP;
       ImGui_ImplSDL2_ProcessEvent(&ev);
@@ -3755,13 +3756,16 @@ bool FurnaceGUI::loop() {
         }
         case SDL_MOUSEBUTTONUP:
           pointUp(ev.button.x,ev.button.y,ev.button.button);
+          insEditMayBeDirty=true;
           break;
         case SDL_MOUSEBUTTONDOWN:
           pointDown(ev.button.x,ev.button.y,ev.button.button);
+          insEditMayBeDirty=true;
           break;
         case SDL_MOUSEWHEEL:
           wheelX+=ev.wheel.x;
           wheelY+=ev.wheel.y;
+          insEditMayBeDirty=true;
           break;
         case SDL_WINDOWEVENT:
           switch (ev.window.event) {
@@ -3838,12 +3842,14 @@ bool FurnaceGUI::loop() {
           if (!ImGui::GetIO().WantCaptureKeyboard) {
             keyDown(ev);
           }
+          insEditMayBeDirty=true;
 #ifdef IS_MOBILE
           injectBackUp=true;
 #endif
           break;
         case SDL_KEYUP:
           // for now
+          insEditMayBeDirty=true;
           break;
         case SDL_DROPFILE:
           if (ev.drop.file!=NULL) {
@@ -7241,6 +7247,11 @@ bool FurnaceGUI::loop() {
       willCommit=false;
     }
 
+    // To check for instrument editor modification, we need an up-to-date `insEditMayBeDirty`
+    // (based on incoming user input events), and we want any possible instrument modifications
+    // to already have been made.
+    checkRecordInstrumentUndoStep();
+
     if (shallDetectScale) {
       if (--shallDetectScale<1) {
         if (settings.dpiScale<0.5f) {
@@ -8414,6 +8425,8 @@ FurnaceGUI::FurnaceGUI():
   localeRequiresChineseTrad(false),
   localeRequiresKorean(false),
   prevInsData(NULL),
+  cachedCurInsPtr(NULL),
+  insEditMayBeDirty(false),
   pendingLayoutImport(NULL),
   pendingLayoutImportLen(0),
   pendingLayoutImportStep(0),

src/gui/gui.h

@@ -2269,6 +2269,9 @@ class FurnaceGUI {
   std::vector<ImWchar> localeExtraRanges;
 
   DivInstrument* prevInsData;
+  DivInstrument cachedCurIns;
+  DivInstrument* cachedCurInsPtr;
+  bool insEditMayBeDirty;
 
   unsigned char* pendingLayoutImport;
   size_t pendingLayoutImportLen;
@@ -2935,6 +2938,10 @@ class FurnaceGUI {
   void doUndoSample();
   void doRedoSample();
 
+  void checkRecordInstrumentUndoStep();
+  void doUndoInstrument();
+  void doRedoInstrument();
+
   void play(int row=0);
   void setOrder(unsigned char order, bool forced=false);
   void stop();

src/gui/insEdit.cpp

@@ -5250,6 +5250,7 @@ void FurnaceGUI::drawInsEdit() {
     ImGui::SetNextWindowSizeConstraints(ImVec2(440.0f*dpiScale,400.0f*dpiScale),ImVec2(canvasW,canvasH));
   }
   if (ImGui::Begin("Instrument Editor",&insEditOpen,globalWinFlags|(settings.allowEditDocking?0:ImGuiWindowFlags_NoDocking),_("Instrument Editor"))) {
+    DivInstrument* ins=NULL;
     if (curIns==-2) {
       ImGui::SetCursorPosY(ImGui::GetCursorPosY()+(ImGui::GetContentRegionAvail().y-ImGui::GetFrameHeightWithSpacing()+ImGui::GetStyle().ItemSpacing.y)*0.5f);
       CENTER_TEXT(_("waiting..."));
@@ -5277,6 +5278,7 @@ void FurnaceGUI::drawInsEdit() {
                 curIns=i;
                 wavePreviewInit=true;
                 updateFMPreview=true;
+                ins = e->song.ins[curIns];
               }
             }
             ImGui::EndCombo();
@@ -5299,7 +5301,7 @@ void FurnaceGUI::drawInsEdit() {
         ImGui::EndTable();
       }
     } else {
-      DivInstrument* ins=e->song.ins[curIns];
+      ins=e->song.ins[curIns];
       if (updateFMPreview) {
         renderFMPreview(ins);
         updateFMPreview=false;
@@ -7739,6 +7741,63 @@ void FurnaceGUI::drawInsEdit() {
       ImGui::EndPopup();
     }
   }
+
   if (ImGui::IsWindowFocused(ImGuiFocusedFlags_ChildWindows)) curWindow=GUI_WINDOW_INS_EDIT;
   ImGui::End();
 }
+
+void FurnaceGUI::checkRecordInstrumentUndoStep() {
+  if (insEditOpen && curIns>=0 && curIns<(int)e->song.ins.size()) {
+    DivInstrument* ins=e->song.ins[curIns];
+
+    // invalidate cachedCurIns/any possible changes if the cachedCurIns was referencing a different
+    // instrument altgoether
+    bool insChanged=ins!=cachedCurInsPtr;
+    if (insChanged) {
+      insEditMayBeDirty=false;
+      cachedCurInsPtr=ins;
+      cachedCurIns=*ins;
+    }
+
+    cachedCurInsPtr=ins;
+
+    // check against the last cached to see if diff -- note that modifications to instruments
+    // happen outside drawInsEdit (e.g. cursor inputs are processed and can directly modify
+    // macro data).  but don't check until we think the user input is complete.
+    bool delayDiff=ImGui::IsMouseDown(ImGuiMouseButton_Left) || ImGui::IsMouseDown(ImGuiMouseButton_Right) || ImGui::GetIO().WantCaptureKeyboard;
+    if (!delayDiff && insEditMayBeDirty) {
+      bool hasChange=ins->recordUndoStepIfChanged(e->processTime, &cachedCurIns);
+      if (hasChange) {
+        cachedCurIns=*ins;
+      }
+      insEditMayBeDirty=false;
+    }
+  } else {
+    cachedCurInsPtr=NULL;
+    insEditMayBeDirty=false;
+  }
+}
+
+void FurnaceGUI::doUndoInstrument() {
+  if (!insEditOpen) return;
+  if (curIns<0 || curIns>=(int)e->song.ins.size()) return;
+  DivInstrument* ins=e->song.ins[curIns];
+  // is locking the engine necessary? copied from doUndoSample
+  e->lockEngine([this,ins]() {
+    ins->undo();
+    cachedCurInsPtr=ins;
+    cachedCurIns=*ins;
+  });
+}
+
+void FurnaceGUI::doRedoInstrument() {
+  if (!insEditOpen) return;
+  if (curIns<0 || curIns>=(int)e->song.ins.size()) return;
+  DivInstrument* ins=e->song.ins[curIns];
+  // is locking the engine necessary? copied from doRedoSample
+  e->lockEngine([this,ins]() {
+    ins->redo();
+    cachedCurInsPtr=ins;
+    cachedCurIns=*ins;
+  });
+}