Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Sync Level Strip Renumber Changes with XSheet #1464

Merged
merged 11 commits into from Nov 24, 2017
Merged

Sync Level Strip Renumber Changes with XSheet #1464

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
84902f9
Sync Level Strip Changes with XSheet
Sep 8, 2017
318fe5e
All Commands (seem to) Sync
Sep 8, 2017
74bdb1b
Added sync level strip to preferences
Sep 9, 2017
9e1af19
merge master
Sep 9, 2017
247eab2
Fix merge conflicts
Oct 9, 2017
1f91f05
Merge master
Nov 15, 2017
79ce0c5
Requested changes
Nov 15, 2017
03b0274
Resolve conflicts
Nov 15, 2017
4730c79
Fix multi sub xsheet
Nov 17, 2017
359cceb
Requested changes
Nov 21, 2017
07b972e
Fix overlapping commands
Nov 21, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 7 additions & 1 deletion toonz/sources/include/toonz/preferences.h
View file
Open in desktop
Expand Up @@ -403,6 +403,11 @@ class DVAPI Preferences final : public QObject // singleton
void enableShowColumnNumbers(bool on);
bool isShowColumnNumbersEnabled() const { return m_showColumnNumbers; }

void enableSyncLevelRenumberWithXsheet(bool on);
bool isSyncLevelRenumberWithXsheetEnabled() const {
return m_syncLevelRenumberWithXsheet;
}

void enableShortcutCommandsWhileRenamingCell(bool on);
bool isShortcutCommandsWhileRenamingCellEnabled() const {
return m_shortcutCommandsWhileRenamingCellEnabled;
Expand Down Expand Up @@ -569,7 +574,8 @@ class DVAPI Preferences final : public QObject // singleton
m_sceneNumberingEnabled, m_animationSheetEnabled, m_inksOnly,
m_startupPopupEnabled;
bool m_fillOnlySavebox, m_show0ThickLines, m_regionAntialias;
bool m_onionSkinDuringPlayback, m_ignoreImageDpi;
bool m_onionSkinDuringPlayback, m_ignoreImageDpi,
m_syncLevelRenumberWithXsheet;
bool m_keepFillOnVectorSimplify, m_useHigherDpiOnVectorSimplify;
TPixel32 m_viewerBGColor, m_previewBGColor, m_chessboardColor1,
m_chessboardColor2;
Expand Down
89 changes: 66 additions & 23 deletions toonz/sources/toonz/cellselection.cpp
View file
Open in desktop
Expand Up @@ -39,6 +39,7 @@
#include "toonz/tobjecthandle.h"
#include "toonz/txsheet.h"
#include "toonz/txshsimplelevel.h"
#include "toonz/txshchildlevel.h"
#include "toonz/toonzscene.h"
#include "toonz/txshleveltypes.h"
#include "toonz/tcamera.h"
Expand Down Expand Up @@ -429,6 +430,7 @@ class InsertUndo final : public TUndo {

class RenumberUndo final : public TUndo {
std::map<TXshCell, TXshCell> m_undoTable, m_redoTable;
std::vector<TXshChildLevel *> *m_childLevels;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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() .

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


public:
class RedoNotifier;
Expand All @@ -437,14 +439,14 @@ class RenumberUndo final : public TUndo {
public:
RenumberUndo(const std::map<TXshCell, TXshCell> &cellsMap)
: m_redoTable(cellsMap) {
m_childLevels = new std::vector<TXshChildLevel *>();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// 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) const {
TXsheet *xsh = TApp::instance()->getCurrentXsheet()->getXsheet();
void renumber(const std::map<TXshCell, TXshCell> &table, TXsheet *xsh) const {
for (int c = 0; c < xsh->getColumnCount(); ++c) {
int r0, r1;
int n = xsh->getCellRange(c, r0, r1);
Expand All @@ -453,6 +455,19 @@ class RenumberUndo final : public TUndo {
std::vector<TXshCell> cells(n);
xsh->getCells(r0, c, n, &cells[0]);
for (int i = 0; i < n; 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(m_childLevels->begin(), m_childLevels->end(),
level) == m_childLevels->end()) {
m_childLevels->push_back(level);
TXsheet *subXsh = level->getXsheet();
renumber(table, subXsh);
}
}
std::map<TXshCell, TXshCell>::const_iterator it =
table.find(cells[i]);
if (it != table.end() && it->first != it->second)
Expand All @@ -463,8 +478,16 @@ class RenumberUndo final : public TUndo {
}
}

void undo() const override { renumber(m_undoTable); }
void redo() const override { renumber(m_redoTable); }
void undo() const override {
TXsheet *xsh =
TApp::instance()->getCurrentScene()->getScene()->getTopXsheet();
renumber(m_undoTable, xsh);
}
void redo() const override {
TXsheet *xsh =
TApp::instance()->getCurrentScene()->getScene()->getTopXsheet();
renumber(m_redoTable, xsh);
}

int getSize() const override {
return (m_redoTable.size() << 2) * sizeof(TXshCell);
Expand Down Expand Up @@ -1335,19 +1358,41 @@ void TCellSelection::enableCommands() {

bool TCellSelection::isEnabledCommand(
std::string commandId) { // static function
static QList<std::string> commands = {
MI_Autorenumber, MI_Reverse, MI_Swing,
MI_Random, MI_Increment, MI_ResetStep,
MI_IncreaseStep, MI_DecreaseStep, MI_Step2,
MI_Step3, MI_Step4, MI_Each2,
MI_Each3, MI_Each4, MI_Rollup,
MI_Rolldown, MI_TimeStretch, MI_CloneLevel,
MI_SetKeyframes, MI_Copy, MI_Paste,
MI_PasteInto, MI_Cut, MI_Clear,
MI_Insert, MI_Reframe1, MI_Reframe2,
MI_Reframe3, MI_Reframe4, MI_ReframeWithEmptyInbetweens,
MI_Undo, MI_Redo, MI_PasteNumbers,
MI_ConvertToToonzRaster, MI_ConvertVectorToVector};
static QList<std::string> commands = {MI_Autorenumber,
MI_Reverse,
MI_Swing,
MI_Random,
MI_Increment,
MI_ResetStep,
MI_IncreaseStep,
MI_DecreaseStep,
MI_Step2,
MI_Step3,
MI_Step4,
MI_Each2,
MI_Each3,
MI_Each4,
MI_Rollup,
MI_Rolldown,
MI_TimeStretch,
MI_CloneLevel,
MI_SetKeyframes,
MI_Copy,
MI_Paste,
MI_PasteInto,
MI_Cut,
MI_Clear,
MI_Insert,
MI_Reframe1,
MI_Reframe2,
MI_Reframe3,
MI_Reframe4,
MI_ReframeWithEmptyInbetweens,
MI_Undo,
MI_Redo,
MI_PasteNumbers,
MI_ConvertToToonzRaster,
MI_ConvertVectorToVector};
return commands.contains(commandId);
}

Expand Down Expand Up @@ -2447,8 +2492,8 @@ TXshSimpleLevel *TCellSelection::getNewToonzRasterLevel(
ToonzScene *scene = TApp::instance()->getCurrentScene()->getScene();
TFilePath sourcePath = sourceSl->getPath();
std::wstring sourceName = sourcePath.getWideName();
TFilePath parentDir = sourceSl->getPath().getParentDir();
TFilePath fp = scene->getDefaultLevelPath(TZP_XSHLEVEL, sourceName)
TFilePath parentDir = sourceSl->getPath().getParentDir();
TFilePath fp = scene->getDefaultLevelPath(TZP_XSHLEVEL, sourceName)
.withParentDir(parentDir);
TFilePath actualFp = scene->decodeFilePath(fp);

Expand Down Expand Up @@ -2491,8 +2536,7 @@ class VectorToVectorUndo final : public TUndo {
, m_newImages(newImages)
, m_oldLevelHooks(oldLevelHooks) {}

~VectorToVectorUndo() {
}
~VectorToVectorUndo() {}

void undo() const override {
for (int i = 0; i < m_frameIds.size(); i++) {
Expand Down Expand Up @@ -2754,8 +2798,7 @@ void TCellSelection::convertVectortoVector() {
// get clones of the new images for undo
std::vector<TImageP> newImages;
for (i = 0; i < totalImages; i++) {
newImages.push_back(
sourceSl->getFrame(frameIds[i], false)->cloneImage());
newImages.push_back(sourceSl->getFrame(frameIds[i], false)->cloneImage());
}

HookSet *oldLevelHooks = new HookSet();
Expand Down