Simplify remap between old -> new palette

Now the remap is calculated from the old palette to the new palette when
the "Remap" button is pressed. In this way we can remap after loading
a new palette (fix #737), or after generating an optimized palette, or
changing colors (fix #563).

src/app/ui/color_bar.cpp

@@ -44,9 +44,11 @@
 #include "doc/cel.h"
 #include "doc/cels_range.h"
 #include "doc/image.h"
+#include "doc/image_impl.h"
 #include "doc/palette.h"
 #include "doc/primitives.h"
 #include "doc/remap.h"
+#include "doc/rgbmap.h"
 #include "doc/sort_palette.h"
 #include "doc/sprite.h"
 #include "she/surface.h"
@@ -60,6 +62,7 @@
 #include "ui/system.h"
 #include "ui/tooltips.h"
 
+
 #include <cstring>
 
 
@@ -237,7 +240,8 @@ ColorBar::ColorBar(int align)
   onColorButtonChange(getFgColor());
 
   UIContext::instance()->addObserver(this);
-  m_conn = UIContext::instance()->BeforeCommandExecution.connect(&ColorBar::onBeforeExecuteCommand, this);
+  m_beforeCmdConn = UIContext::instance()->BeforeCommandExecution.connect(&ColorBar::onBeforeExecuteCommand, this);
+  m_afterCmdConn = UIContext::instance()->AfterCommandExecution.connect(&ColorBar::onAfterExecuteCommand, this);
   m_fgConn = Preferences::instance().colorBar.fgColor.AfterChange.connect(Bind<void>(&ColorBar::onFgColorChangeFromPreferences, this));
   m_bgConn = Preferences::instance().colorBar.bgColor.AfterChange.connect(Bind<void>(&ColorBar::onBgColorChangeFromPreferences, this));
   m_paletteView.FocusEnter.connect(&ColorBar::onFocusPaletteView, this);
@@ -295,7 +299,7 @@ void ColorBar::onActiveSiteChange(const doc::Site& site)
 {
   if (m_lastDocument != site.document()) {
     m_lastDocument = site.document();
-    destroyRemap();
+    hideRemap();
   }
 }
 
@@ -314,10 +318,17 @@ void ColorBar::onFocusPaletteView()
 }
 
 void ColorBar::onBeforeExecuteCommand(Command* command)
+{
+  if (command->id() == CommandId::SetPalette ||
+      command->id() == CommandId::LoadPalette)
+    showRemap();
+}
+
+void ColorBar::onAfterExecuteCommand(Command* command)
 {
   if (command->id() == CommandId::Undo ||
       command->id() == CommandId::Redo)
-    destroyRemap();
+    invalidate();
 }
 
 // Switches the palette-editor
@@ -422,10 +433,26 @@ void ColorBar::onPaletteButtonClick()
 
 void ColorBar::onRemapButtonClick()
 {
-  ASSERT(m_remap);
+  ASSERT(m_oldPalette);
+
+  // Create remap from m_oldPalette to the current palette
+  Remap remap(1);
+  try {
+    ContextWriter writer(UIContext::instance(), 500);
+    Sprite* sprite = writer.sprite();
+    ASSERT(sprite);
+    if (!sprite)
+      return;
+
+    remap = create_remap_to_change_palette(
+      m_oldPalette, get_current_palette(), sprite->transparentColor());
+  }
+  catch (base::Exception& e) {
+    Console::showException(e);
+  }
 
   // Check the remap
-  if (!m_remap->isFor8bit() &&
+  if (!remap.isFor8bit() &&
       Alert::show(
         "Automatic Remap"
         "<<The remap operation cannot be perfectly done for more than 256 colors."
@@ -441,15 +468,28 @@ void ColorBar::onRemapButtonClick()
       ASSERT(sprite->pixelFormat() == IMAGE_INDEXED);
 
       Transaction transaction(writer.context(), "Remap Colors", ModifyDocument);
-      if (m_remap->isFor8bit()) {
-        transaction.execute(new cmd::RemapColors(sprite, *m_remap));
+      bool remapPixels = true;
+
+      if (remap.isFor8bit()) {
+        PalettePicks usedEntries(256);
+
+        for (const Cel* cel : sprite->uniqueCels()) {
+          for (const auto& i : const LockImageBits<IndexedTraits>(cel->image()))
+            usedEntries[i] = true;
+        }
+
+        if (remap.isInvertible(usedEntries)) {
+          transaction.execute(new cmd::RemapColors(sprite, remap));
+          remapPixels = false;
+        }
       }
+
       // Special remap saving original images in undo history
-      else {
+      if (remapPixels) {
         for (Cel* cel : sprite->uniqueCels()) {
           ImageRef celImage = cel->imageRef();
           ImageRef newImage(Image::createCopy(celImage.get()));
-          doc::remap_image(newImage.get(), *m_remap);
+          doc::remap_image(newImage.get(), remap);
 
           transaction.execute(new cmd::ReplaceImage(
                                 sprite, celImage, newImage));
@@ -457,14 +497,14 @@ void ColorBar::onRemapButtonClick()
       }
 
       color_t oldTransparent = sprite->transparentColor();
-      color_t newTransparent = (*m_remap)[oldTransparent];
+      color_t newTransparent = remap[oldTransparent];
       if (oldTransparent != newTransparent)
         transaction.execute(new cmd::SetTransparentColor(sprite, newTransparent));
 
       transaction.commit();
     }
     update_screen_for_document(writer.document());
-    destroyRemap();
+    hideRemap();
   }
   catch (base::Exception& e) {
     Console::showException(e);
@@ -489,29 +529,13 @@ void ColorBar::onPaletteViewIndexChange(int index, ui::MouseButtons buttons)
 
 void ColorBar::onPaletteViewRemapColors(const Remap& remap, const Palette* newPalette)
 {
-  applyRemap(remap, newPalette, "Drag-and-Drop Colors");
-}
-
-void ColorBar::applyRemap(const doc::Remap& remap, const doc::Palette* newPalette, const std::string& actionText)
-{
-  doc::Site site = UIContext::instance()->activeSite();
-  if (site.sprite() &&
-      site.sprite()->pixelFormat() == IMAGE_INDEXED) {
-    if (!m_remap) {
-      m_remap.reset(new doc::Remap(remap));
-      m_remapButton.setVisible(true);
-      layout();
-    }
-    else {
-      m_remap->merge(remap);
-    }
-  }
-
-  setPalette(newPalette, actionText);
+  setPalette(newPalette, "Drag-and-Drop Colors");
 }
 
 void ColorBar::setPalette(const doc::Palette* newPalette, const std::string& actionText)
 {
+  showRemap();
+
   try {
     ContextWriter writer(UIContext::instance(), 500);
     Sprite* sprite = writer.sprite();
@@ -709,7 +733,7 @@ void ColorBar::onReverseColors()
   }
 
   Palette newPalette(*get_current_palette(), remap);
-  applyRemap(remap, &newPalette, "Reverse Colors");
+  setPalette(&newPalette, "Reverse Colors");
 }
 
 void ColorBar::onSortBy(SortPaletteBy channel)
@@ -753,7 +777,7 @@ void ColorBar::onSortBy(SortPaletteBy channel)
   // Create a new palette and apply the remap. This is the final new
   // palette for the sprite.
   Palette newPalette(palette, remapOrig);
-  applyRemap(remapOrig, &newPalette, "Sort Colors");
+  setPalette(&newPalette, "Sort Colors");
 }
 
 void ColorBar::onGradient()
@@ -773,12 +797,25 @@ void ColorBar::setAscending(bool ascending)
   m_ascending = ascending;
 }
 
-void ColorBar::destroyRemap()
+void ColorBar::showRemap()
 {
-  if (!m_remap)
+  doc::Site site = UIContext::instance()->activeSite();
+  if (site.sprite() &&
+      site.sprite()->pixelFormat() == IMAGE_INDEXED) {
+    if (!m_oldPalette) {
+      m_oldPalette.reset(new Palette(*get_current_palette()));
+      m_remapButton.setVisible(true);
+      layout();
+    }
+  }
+}
+
+void ColorBar::hideRemap()
+{
+  if (!m_oldPalette)
     return;
 
-  m_remap.reset();
+  m_oldPalette.reset();
   m_remapButton.setVisible(false);
   layout();
 }

src/app/ui/color_bar.h

@@ -75,6 +75,7 @@ namespace app {
     void onAppPaletteChange();
     void onFocusPaletteView();
     void onBeforeExecuteCommand(Command* command);
+    void onAfterExecuteCommand(Command* command);
     void onPaletteButtonClick();
     void onRemapButtonClick();
     void onPaletteIndexChange(PaletteIndexChangeEvent& ev);
@@ -99,8 +100,8 @@ namespace app {
     app::Color onPaletteViewGetBackgroundIndex() override;
 
   private:
-    void destroyRemap();
-    void applyRemap(const doc::Remap& remap, const doc::Palette* newPalette, const std::string& actionText);
+    void showRemap();
+    void hideRemap();
     void setPalette(const doc::Palette* newPalette, const std::string& actionText);
     void setTransparentIndex(int index);
     void updateWarningIcon(const app::Color& color, ui::Button* warningIcon);
@@ -127,10 +128,11 @@ namespace app {
     WarningIcon* m_bgWarningIcon;
     bool m_lock;
     bool m_syncingWithPref;
-    base::UniquePtr<doc::Remap> m_remap;
+    base::UniquePtr<doc::Palette> m_oldPalette;
     const doc::Document* m_lastDocument;
     bool m_ascending;
-    ScopedConnection m_conn;
+    ScopedConnection m_beforeCmdConn;
+    ScopedConnection m_afterCmdConn;
     ScopedConnection m_fgConn;
     ScopedConnection m_bgConn;
     ScopedConnection m_appPalChangeConn;

src/doc/remap.cpp

@@ -10,7 +10,9 @@
 
 #include "doc/remap.h"
 
+#include "doc/palette.h"
 #include "doc/palette_picks.h"
+#include "doc/rgbmap.h"
 
 namespace doc {
 
@@ -63,6 +65,52 @@ Remap create_remap_to_expand_palette(int size, int count, int beforeIndex)
   return map;
 }
 
+Remap create_remap_to_change_palette(
+  const Palette* oldPalette, const Palette* newPalette,
+  const int oldMaskIndex)
+{
+  Remap remap(MAX(oldPalette->size(), newPalette->size()));
+  int maskIndex = oldMaskIndex;
+
+  if (maskIndex >= 0) {
+    color_t maskColor = oldPalette->getEntry(maskIndex);
+    int r = rgba_getr(maskColor);
+    int g = rgba_getg(maskColor);
+    int b = rgba_getb(maskColor);
+    int a = rgba_geta(maskColor);
+
+    // Find the new mask color
+    maskIndex = newPalette->findExactMatch(r, g, b, a, -1);
+    if (maskIndex >= 0)
+      remap.map(oldMaskIndex, maskIndex);
+  }
+
+  RgbMap rgbmap;
+  rgbmap.regenerate(newPalette, maskIndex);
+
+  for (int i=0; i<oldPalette->size(); ++i) {
+    if (i == oldMaskIndex)
+      continue;
+
+    const color_t color = oldPalette->getEntry(i);
+    int j = newPalette->findExactMatch(
+      rgba_getr(color),
+      rgba_getg(color),
+      rgba_getb(color),
+      rgba_geta(color), maskIndex);
+
+    if (j < 0)
+      j = newPalette->findBestfit(
+        rgba_getr(color),
+        rgba_getg(color),
+        rgba_getb(color),
+        rgba_geta(color), maskIndex);
+
+    remap.map(i, j);
+  }
+  return remap;
+}
+
 void Remap::merge(const Remap& other)
 {
   for (int i=0; i<size(); ++i) {
@@ -90,4 +138,20 @@ bool Remap::isFor8bit() const
   return true;
 }
 
+bool Remap::isInvertible(const PalettePicks& usedEntries) const
+{
+  PalettePicks picks(size());
+  for (int i=0; i<size(); ++i) {
+    if (!usedEntries[i])
+      continue;
+
+    int j = m_map[i];
+    if (picks[j])
+      return false;
+
+    picks[j] = true;
+  }
+  return true;
+}
+
 } // namespace doc

src/doc/remap.h

@@ -12,6 +12,7 @@
 
 namespace doc {
 
+  class Palette;
   class PalettePicks;
 
   class Remap {
@@ -22,6 +23,7 @@ namespace doc {
       return (int)m_map.size();
     }
 
+    // Maps input "fromIndex" value, to "toIndex" output.
     void map(int fromIndex, int toIndex) {
       ASSERT(fromIndex >= 0 && fromIndex < size());
       ASSERT(toIndex >= 0 && toIndex < size());
@@ -47,6 +49,13 @@ namespace doc {
     // Returns true if the remap can be safely used in 8-bit images.
     bool isFor8bit() const;
 
+    // Returns true if the remap can be inverted. Remaps can be
+    // inverted when each input is mapped to one output (e.g. two
+    // inputs are not mapped to the same output). This kind of remaps
+    // are really easy to undone: You can store the inverted remap as
+    // undo data, without saving all images' pixels.
+    bool isInvertible(const PalettePicks& usedEntries) const;
+
   private:
     std::vector<int> m_map;
   };
@@ -57,6 +66,10 @@ namespace doc {
 
   Remap create_remap_to_expand_palette(int size, int count, int beforeIndex);
 
+  Remap create_remap_to_change_palette(
+    const Palette* oldPalette, const Palette* newPalette,
+    const int oldMaskIndex);
+
 } // namespace doc
 
 #endif

src/doc/remap_tests.cpp

@@ -11,11 +11,12 @@
 #include <gtest/gtest.h>
 
 #include "doc/remap.h"
+#include "doc/palette.h"
 #include "doc/palette_picks.h"
 
 using namespace doc;
 
-TEST(Remap, RemapToMovePicks)
+TEST(Remap, ToMovePicks)
 {
   PalettePicks entries(20);
   std::fill(entries.begin(), entries.end(), false);
@@ -68,9 +69,13 @@ TEST(Remap, RemapToMovePicks)
   EXPECT_EQ(14, map[17]);
   EXPECT_EQ(18, map[18]);
   EXPECT_EQ(19, map[19]);
+
+  PalettePicks all(map.size());
+  all.all();
+  EXPECT_TRUE(map.isInvertible(all));
 }
 
-TEST(Remap, RemapToExpandPalette)
+TEST(Remap, ToExpandPalette)
 {
   Remap map = create_remap_to_expand_palette(10, 3, 1);
 
@@ -97,6 +102,90 @@ TEST(Remap, RemapToExpandPalette)
   EXPECT_EQ(7, map[7]);
   EXPECT_EQ(8, map[8]);
   EXPECT_EQ(9, map[9]);
+
+  PalettePicks all(map.size());
+  all.all();
+  EXPECT_TRUE(map.isInvertible(all));
+}
+
+TEST(Remap, BetweenPalettesChangeMask)
+{
+  Palette a(frame_t(0), 4);
+  Palette b(frame_t(0), 4);
+
+  a.setEntry(0, rgba(255, 0, 0, 255));
+  a.setEntry(1, rgba(0, 255, 0, 255));
+  a.setEntry(2, rgba(0, 0, 255, 255));
+  a.setEntry(3, rgba(255, 255, 255, 255));
+
+  b.setEntry(0, rgba(255, 255, 255, 255));
+  b.setEntry(1, rgba(0, 255, 0, 255));
+  b.setEntry(2, rgba(0, 0, 255, 255));
+  b.setEntry(3, rgba(255, 0, 0, 255));
+
+  Remap map = create_remap_to_change_palette(&a, &b, 0);
+
+  EXPECT_EQ(3, map[0]);
+  EXPECT_EQ(1, map[1]);
+  EXPECT_EQ(2, map[2]);
+  EXPECT_EQ(0, map[3]);
+
+  PalettePicks all(map.size());
+  all.all();
+  EXPECT_TRUE(map.isInvertible(all));
+}
+
+TEST(Remap, BetweenPalettesDontChangeMask)
+{
+  Palette a(frame_t(0), 4);
+  Palette b(frame_t(0), 4);
+
+  a.setEntry(0, rgba(0, 0, 0, 255));
+  a.setEntry(1, rgba(0, 255, 255, 255));
+  a.setEntry(2, rgba(0, 0, 0, 255));
+  a.setEntry(3, rgba(255, 0, 0, 255));
+
+  b.setEntry(0, rgba(255, 0, 0, 255));
+  b.setEntry(1, rgba(0, 255, 255, 255));
+  b.setEntry(2, rgba(0, 0, 0, 255));
+  b.setEntry(3, rgba(0, 0, 0, 255));
+
+  Remap map = create_remap_to_change_palette(&a, &b, 2);
+
+  EXPECT_EQ(3, map[0]);
+  EXPECT_EQ(1, map[1]);
+  EXPECT_EQ(2, map[2]);
+  EXPECT_EQ(0, map[3]);
+
+  PalettePicks all(map.size());
+  all.all();
+  EXPECT_TRUE(map.isInvertible(all));
+}
+
+TEST(Remap, BetweenPalettesNonInvertible)
+{
+  Palette a(frame_t(0), 4);
+  Palette b(frame_t(0), 3);
+
+  a.setEntry(0, rgba(0, 0, 0, 255));
+  a.setEntry(1, rgba(64, 0, 0, 255));
+  a.setEntry(2, rgba(0, 255, 0, 255));
+  a.setEntry(3, rgba(0, 0, 128, 255));
+
+  b.setEntry(0, rgba(0, 255, 0, 255));
+  b.setEntry(1, rgba(0, 0, 0, 255));
+  b.setEntry(2, rgba(64, 0, 0, 255));
+
+  Remap map = create_remap_to_change_palette(&a, &b, 0);
+
+  EXPECT_EQ(1, map[0]);
+  EXPECT_EQ(2, map[1]);
+  EXPECT_EQ(0, map[2]);
+  EXPECT_EQ(2, map[3]);
+
+  PalettePicks all(map.size());
+  all.all();
+  EXPECT_FALSE(map.isInvertible(all));
 }
 
 int main(int argc, char** argv)