Sync Level Strip Renumber Changes with XSheet

[ghost]

Currently when you insert a new frame, duplicate a frame or basically do anything in the level strip that changes the number order of the drawings, any drawings exposed on the xsheet aren't automatically updated with the new drawing number. This can result in orphaned frames or undesired and unexpected changes in the animation.

This adds an option in Preferences to sync changes in the level strip with the xsheet. As requested, this feature is disabled by default.

Fix for #471

[blurymind]

thank you!

[shun-iwasawa]

For now updateXsheet() checks the current preference value of Sync Level Strip Drawing Number Changes with the Xsheet before execution.

Which means that, if you toggle the preference value just after editing the level, undo will not revert the scene properly. I think this value should be remembered by each undo instance.

[ghost]

@shun-iwasawa Good catch. I think I fixed it.

[shun-iwasawa]

Jenkins

[ghost]

Fixed conflicts

[shun-iwasawa]

Jenkins again

[shun-iwasawa]

@turtleTooth Sorry for the late notice, but I found a ( minor ) bug here.
In this line you are checking whether the subxsheet level is already renumbered by using the list childLevels .
However, there may be a case that the same subxsheet level is used in different hierarchies of subxsheets.
For example like this:

In the above example, the subxsheet sub_B is used both in the parent xhseet and in the subxsheet sub_A .
When renumbering the level A , this condition cannot catch such usage and sub_B will be renumbered twice.

I think for the renumbering of subxsheet it would better to share the list among the hierarchy.

[ghost]

Got it. But the bug I posted in the issues makes it hard to test. But if you look at the actual frame numbers in the xsheet- they show the correct number.

[shun-iwasawa]

Jenkins again

[shun-iwasawa]

It's not your fault, but is blamed for bad naming of class (already done in Toonz Harlequin's source, not me!).
Sorry to say, I realized that there are two RenumberUndo for now.

I imagine you meant to fix RenumberUndo in filmstripcommand.cpp .
This RenumberUndo in cellselection.cpp is used for Autorenumber command and is not need to be changed -
except the confusing class name.
Regarding the RenumberUndo in filmstripcommand.cpp, it seems that you don't need to add any additional modification as it calls updateXSheet() .

[ghost]

@shun-iwasawa The renumber issue also occurs on the autorenumber command from the xsheet. I was aware that I was updating the the command for that as well. Without this, changes made to the level numbering don't always propagate to the same level in subxsheets.

[shun-iwasawa]

Sorry for not taking note of it. I'll review this again.

[shun-iwasawa]

The function updateSubXsheet() seems to be almost the same as updateXSheet() .
To refactor, how about unifying them as follows:?

------------------- toonz/sources/toonz/filmstripcommand.cpp -------------------
-void updateSubXSheet(TXshSimpleLevel *sl, std::vector<TFrameId> oldFids,
+void doUpdateXSheet(TXshSimpleLevel *sl, std::vector<TFrameId> oldFids,
                      std::vector<TFrameId> newFids, TXsheet *xsh,
-                     std::vector<TXshChildLevel *> *childLevels) {
+                     std::vector<TXshChildLevel *> &childLevels) {
   for (int c = 0; c < xsh->getColumnCount(); ++c) {
     int r0, r1;
     int n = xsh->getCellRange(c, r0, r1);
@@ -71,11 +71,11 @@ void updateSubXSheet(TXshSimpleLevel *sl, std::vector<TFrameId> oldFids,
           TXshChildLevel *level = cells[i].m_level->getChildLevel();
           // make sure we haven't already checked the level
           if (level &&
-              std::find(childLevels->begin(), childLevels->end(), level) ==
-                  childLevels->end()) {
-            childLevels->push_back(level);
+              std::find(childLevels.begin(), childLevels.end(), level) ==
+                  childLevels.end()) {
+            childLevels.push_back(level);
             TXsheet *subXsh = level->getXsheet();
-            updateSubXSheet(sl, oldFids, newFids, subXsh, childLevels);
+            doUpdateXSheet(sl, oldFids, newFids, subXsh, childLevels);
           }
         }
         for (int j = 0; j < oldFids.size(); j++) {
@@ -103,49 +103,8 @@ void updateSubXSheet(TXshSimpleLevel *sl, std::vector<TFrameId> oldFids,
 
 void updateXSheet(TXshSimpleLevel *sl, std::vector<TFrameId> oldFids,
                   std::vector<TFrameId> newFids, TXsheet *xsh) {
-  std::vector<TXshChildLevel *> *childLevels =
-      new std::vector<TXshChildLevel *>();
-  for (int c = 0; c < xsh->getColumnCount(); ++c) {
-    int r0, r1;
-    int n = xsh->getCellRange(c, r0, r1);
-    if (n > 0) {
-      bool changed = false;
-      std::vector<TXshCell> cells(n);
-      xsh->getCells(r0, c, n, &cells[0]);
-      for (int i = 0; i < n; i++) {
-        TXshCell currCell = cells[i];
-        // check the sub xsheets too
-        if (!cells[i].isEmpty() &&
-            cells[i].m_level->getType() == CHILD_XSHLEVEL) {
-          TXshChildLevel *level = cells[i].m_level->getChildLevel();
-          // make sure we haven't already checked the level
-          if (level &&
-              std::find(childLevels->begin(), childLevels->end(), level) ==
-                  childLevels->end()) {
-            childLevels->push_back(level);
-            TXsheet *subXsh = level->getXsheet();
-            updateSubXSheet(sl, oldFids, newFids, subXsh, childLevels);
-          }
-        }
-        for (int j = 0; j < oldFids.size(); j++) {
-          if (oldFids.at(j) == newFids.at(j)) continue;
-          TXshCell tempCell(sl, oldFids.at(j));
-          bool sameSl  = tempCell.getSimpleLevel() == currCell.getSimpleLevel();
-          bool sameFid = tempCell.getFrameId() == currCell.getFrameId();
-          if (sameSl && sameFid) {
-            TXshCell newCell(sl, newFids.at(j));
-            cells[i] = newCell;
-            changed  = true;
-            break;
-          }
-        }
-      }
-      if (changed) {
-        xsh->setCells(r0, c, n, &cells[0]);
-        TApp::instance()->getCurrentXsheet()->notifyXsheetChanged();
-      }
-    }
-  }
+  std::vector<TXshChildLevel *> childLevels;
+  doUpdateXSheet(sl, oldFids, newFids, xsh, childLevels);
 }

[ghost]

Good idea. I fixed this.

[shun-iwasawa]

In your latest modification, the function updateXsheet() will not be called recursively anymore.
Now it is always called after TXsheet *xsh = TApp::instance()->getCurrentScene()->getScene()->getTopXsheet(); , right?
If so, IMO the argument TXsheet *xsh can be removed and obtaining it inside the function would be clear.

[ghost]

Good catch. I updated this.

[shun-iwasawa]

For now m_childLevels seems to be kept whole the lifetime of RenumberUndo instance. It will work properly at the first time when the function renumber() is called, but it becomes unavailable to work from the next time (on undo / redo is called) because the list is not initialized each time.
Also m_childLevels seems to need to be deleted on dtor.

Instead, how about holding a new list each time undo / redo is called as follows?:

-------------------- toonz/sources/toonz/cellselection.cpp --------------------
index 6a41e75..13a5202 100644
@@ -430,7 +430,6 @@ public:
 
 class RenumberUndo final : public TUndo {
   std::map<TXshCell, TXshCell> m_undoTable, m_redoTable;
-  std::vector<TXshChildLevel *> *m_childLevels;
 
 public:
   class RedoNotifier;
@@ -439,14 +438,14 @@ public:
 public:
   RenumberUndo(const std::map<TXshCell, TXshCell> &cellsMap)
       : m_redoTable(cellsMap) {
-    m_childLevels = new std::vector<TXshChildLevel *>();
     // Invert the redo table to obtain the undo table
     std::map<TXshCell, TXshCell>::iterator it, end = m_redoTable.end();
     for (it = m_redoTable.begin(); it != end; ++it)
       m_undoTable.insert(std::make_pair(it->second, it->first));
   }
 
-  void renumber(const std::map<TXshCell, TXshCell> &table, TXsheet *xsh) const {
+  void renumber(const std::map<TXshCell, TXshCell> &table, TXsheet *xsh,
+                std::vector<TXshChildLevel *> &childLevels) const {
     for (int c = 0; c < xsh->getColumnCount(); ++c) {
       int r0, r1;
       int n = xsh->getCellRange(c, r0, r1);
@@ -461,11 +460,11 @@ public:
             TXshChildLevel *level = cells[i].m_level->getChildLevel();
             // make sure we haven't already checked the level
             if (level &&
-                std::find(m_childLevels->begin(), m_childLevels->end(),
-                          level) == m_childLevels->end()) {
-              m_childLevels->push_back(level);
+                std::find(childLevels.begin(), childLevels.end(), level) ==
+                    childLevels.end()) {
+              childLevels.push_back(level);
               TXsheet *subXsh = level->getXsheet();
-              renumber(table, subXsh);
+              renumber(table, subXsh, childLevels);
             }
           }
           std::map<TXshCell, TXshCell>::const_iterator it =
@@ -479,14 +478,16 @@ public:
   }
 
   void undo() const override {
+    std::vector<TXshChildLevel *> childLevels;
     TXsheet *xsh =
         TApp::instance()->getCurrentScene()->getScene()->getTopXsheet();
-    renumber(m_undoTable, xsh);
+    renumber(m_undoTable, xsh, childLevels);
   }
   void redo() const override {
+    std::vector<TXshChildLevel *> childLevels;
     TXsheet *xsh =
         TApp::instance()->getCurrentScene()->getScene()->getTopXsheet();
-    renumber(m_redoTable, xsh);
+    renumber(m_redoTable, xsh, childLevels);
   }

I realized that this PR need more modification regarding the Autorenumber command.
I'll write another comment for it.

[shun-iwasawa]

With this option being activated, Autorenumber command does not work properly.

This option and Autorenumber is very similar, but work an opposite way.

• This option will reflect the Level Strip order to Xsheet.
• Autorenumber reflects the order in Xsheet to the Level Strip.

For now this option seems to effect to the target Xsheet of Autorenumber command as well, which is already arranged and it causes overlapping of renumbering.

[ghost]

Alright I think I have it figured out.

I removed the renumber command from cellselection - this way there is only once place to update the logic for renumbering. The renumber command in filmstripcommand was already being called from autorenumber. This would trigger the updatexsheetcommand if the new option was enabled - making the xsheet get updated twice. I made autorenumber call updatexsheet even if the new command wasn't enabled to get the desired result.

I have tested autorenumber and the film strip commands in isolation, in subxsheets, and in nested subxsheets and the results seem to be correct.

Thanks @shun-iwasawa for your help and patience with this one.

[shun-iwasawa]

Jenkins

[shun-iwasawa]

LGTM Thank you @turtleTooth for adding this option!