Block undo and redo while a background job runs

A job is queued against the model as it stands and hands its result back
when it finishes, so undoing underneath it leaves that result landing on
geometry it was never computed for. Undo and redo now wait for the job
and say so, and can_undo()/can_redo() report the same, so the toolbar
and the menu items stay in step.

src/libslic3r/PNGReadWrite.cpp

@@ -54,43 +54,9 @@ static void png_read_callback(png_struct *png_ptr,
     // Retrieve our input buffer through the png_ptr
     auto reader = static_cast<IStream *>(png_get_io_ptr(png_ptr));
 
-    // libpng expects a short read to be reported through png_error(); returning quietly would leave
-    // it decoding whatever happened to be in outBytes.
-    if (!reader || !reader->is_ok() ||
-        reader->read(static_cast<std::uint8_t *>(outBytes), byteCountToRead) != byteCountToRead)
-        png_error(png_ptr, "PNG data is truncated");
-}
+    if (!reader || !reader->is_ok()) return;
 
-// libpng reports a corrupt or truncated image by longjmp()ing back to the jump buffer set with
-// setjmp(). The frame it lands in must own nothing that needs destroying: with exceptions enabled
-// MSVC unwinds the stack as part of longjmp, and returning from a frame unwound that way crashes -
-// which is what a truncated texture did on Windows while working everywhere else. So the calls that
-// can fail live in these two helpers, which hold nothing but pointers, and every C++ object the
-// decoders need stays in their own frames.
-static bool png_read_header_guarded(png_struct *png, png_info *info, IStream *in_buf, int sig_bytes)
-{
-    if (setjmp(png_jmpbuf(png)))
-        return false;
-
-    png_set_read_fn(png, static_cast<void *>(in_buf), png_read_callback);
-    // Tell that we have already read the first bytes to check the signature
-    png_set_sig_bytes(png, sig_bytes);
-    png_read_info(png, info);
-    return true;
-}
-
-// `bottom_up` fills the buffer last row first, which is the order the colour decoder hands back.
-static bool png_read_rows_guarded(png_struct *png, png_info *info, png_bytep dst, size_t rows, size_t rowbytes,
-                                  bool bottom_up, bool read_end)
-{
-    if (setjmp(png_jmpbuf(png)))
-        return false;
-
-    for (size_t i = 0; i < rows; ++i)
-        png_read_row(png, dst + (bottom_up ? rows - 1 - i : i) * rowbytes, nullptr);
-    if (read_end)
-        png_read_end(png, info);
-    return true;
+    reader->read(static_cast<std::uint8_t *>(outBytes), byteCountToRead);
 }
 
 bool decode_png(IStream &in_buf, ImageGreyscale &out_img)
@@ -111,8 +77,12 @@ bool decode_png(IStream &in_buf, ImageGreyscale &out_img)
     dsc.info = png_create_info_struct(dsc.png);
     if(!dsc.info) return false;
 
-    if (!png_read_header_guarded(dsc.png, dsc.info, &in_buf, PNG_SIG_BYTES))
-        return false;
+    png_set_read_fn(dsc.png, static_cast<void *>(&in_buf), png_read_callback);
+
+    // Tell that we have already read the first bytes to check the signature
+    png_set_sig_bytes(dsc.png, PNG_SIG_BYTES);
+
+    png_read_info(dsc.png, dsc.info);
 
     out_img.cols = png_get_image_width(dsc.png, dsc.info);
     out_img.rows = png_get_image_height(dsc.png, dsc.info);
@@ -124,8 +94,11 @@ bool decode_png(IStream &in_buf, ImageGreyscale &out_img)
 
     out_img.buf.resize(out_img.rows * out_img.cols);
 
-    return png_read_rows_guarded(dsc.png, dsc.info, static_cast<png_bytep>(out_img.buf.data()), out_img.rows,
-                                 out_img.cols, /* bottom_up */ false, /* read_end */ false);
+    auto readbuf = static_cast<png_bytep>(out_img.buf.data());
+    for (size_t r = 0; r < out_img.rows; ++r)
+        png_read_row(dsc.png, readbuf + r * out_img.cols, nullptr);
+
+    return true;
 }
 
 bool decode_colored_png(IStream &in_buf, ImageColorscale &out_img)
@@ -155,10 +128,12 @@ bool decode_colored_png(IStream &in_buf, ImageColorscale &out_img)
         return false;
     }
 
-    if (!png_read_header_guarded(dsc.png, dsc.info, &in_buf, PNG_SIG_BYTES)) {
-        BOOST_LOG_TRIVIAL(error) << "decode_colored_png: corrupt or truncated PNG data";
-        return false;
-    }
+    png_set_read_fn(dsc.png, static_cast<void *>(&in_buf), png_read_callback);
+
+    // Tell that we have already read the first bytes to check the signature
+    png_set_sig_bytes(dsc.png, PNG_SIG_BYTES);
+
+    png_read_info(dsc.png, dsc.info);
 
     out_img.cols = png_get_image_width(dsc.png, dsc.info);
     out_img.rows = png_get_image_height(dsc.png, dsc.info);
@@ -187,12 +162,13 @@ bool decode_colored_png(IStream &in_buf, ImageColorscale &out_img)
     int interlace_type = png_get_interlace_type(dsc.png, dsc.info);
     BOOST_LOG_TRIVIAL(info) << boost::format("filter_type %1%, compression_type %2%, interlace_type %3%, rowbytes %4%")%filter_type %compression_type %interlace_type %rowbytes;
 
-    if (!png_read_rows_guarded(dsc.png, dsc.info, static_cast<png_bytep>(out_img.buf.data()), out_img.rows, rowbytes,
-                               /* bottom_up */ true, /* read_end */ true)) {
-        BOOST_LOG_TRIVIAL(error) << "decode_colored_png: corrupt or truncated PNG data";
-        return false;
+    auto readbuf = static_cast<png_bytep>(out_img.buf.data());
+    for (size_t r = out_img.rows; r > 0; r--)
+    {
+        png_read_row(dsc.png, readbuf + (r - 1) * rowbytes, nullptr);
     }
 
+    png_read_end(dsc.png, dsc.info);
     png_destroy_read_struct(&dsc.png, &dsc.info, NULL);
 
     return true;

src/slic3r/GUI/Plater.cpp

@@ -7297,6 +7297,8 @@ struct Plater::priv
 
     void undo();
     void redo();
+    // True, and tells the user, while a background job is working on the model - see the definition.
+    bool undo_redo_blocked_by_job();
     void undo_redo_to(size_t time_to_load);
 
     // BBS: backup
@@ -14933,8 +14935,25 @@ void Plater::priv::take_snapshot(const std::string& snapshot_name, const UndoRed
     BOOST_LOG_TRIVIAL(info) << "Undo / Redo snapshot taken: " << snapshot_name << ", Undo / Redo stack memory: " << Slic3r::format_memsize_MB(this->undo_redo_stack().memsize()) << log_memory_info();
 }
 
+// A background job holds the model it is working on: it was queued against the geometry as it stood at
+// the time and hands its result back when it finishes. Undoing while it runs restores an older state
+// underneath it, a different transform or a different mesh, and the result then lands on geometry it
+// was never computed for. Undo and redo therefore wait for the job, and say so rather than doing
+// nothing.
+bool Plater::priv::undo_redo_blocked_by_job()
+{
+    if (m_worker.is_idle())
+        return false;
+    notification_manager->push_notification(NotificationType::CustomNotification,
+                                            NotificationManager::NotificationLevel::RegularNotificationLevel,
+                                            _u8L("Cannot undo or redo while an operation is running. Stop it first."));
+    return true;
+}
+
 void Plater::priv::undo()
 {
+    if (this->undo_redo_blocked_by_job())
+        return;
     const std::vector<UndoRedo::Snapshot> &snapshots = this->undo_redo_stack().snapshots();
     auto it_current = std::lower_bound(snapshots.begin(), snapshots.end(), UndoRedo::Snapshot(this->undo_redo_stack().active_snapshot_time()));
     // BBS: undo-redo until modify record
@@ -14952,6 +14971,8 @@ void Plater::priv::undo()
 
 void Plater::priv::redo()
 {
+    if (this->undo_redo_blocked_by_job())
+        return;
     const std::vector<UndoRedo::Snapshot> &snapshots = this->undo_redo_stack().snapshots();
     auto it_current = std::lower_bound(snapshots.begin(), snapshots.end(), UndoRedo::Snapshot(this->undo_redo_stack().active_snapshot_time()));
     // BBS: undo-redo until modify record
@@ -22808,8 +22829,9 @@ bool Plater::can_copy_to_clipboard() const
     return true;
 }
 
-bool Plater::can_undo() const { return IsShown() && p->is_view3D_shown() && p->undo_redo_stack().has_undo_snapshot(); }
-bool Plater::can_redo() const { return IsShown() && p->is_view3D_shown() && p->undo_redo_stack().has_redo_snapshot(); }
+// The job check keeps the buttons in step with priv::undo()/redo(), which refuse while one runs.
+bool Plater::can_undo() const { return IsShown() && p->is_view3D_shown() && p->m_worker.is_idle() && p->undo_redo_stack().has_undo_snapshot(); }
+bool Plater::can_redo() const { return IsShown() && p->is_view3D_shown() && p->m_worker.is_idle() && p->undo_redo_stack().has_redo_snapshot(); }
 bool Plater::can_reload_from_disk() const { return p->can_reload_from_disk(); }
 //BBS
 bool Plater::can_fillcolor() const { return p->can_fillcolor(); }

tests/libslic3r/CMakeLists.txt

@@ -32,7 +32,6 @@ add_executable(${_TEST_NAME}_tests
     test_geometry.cpp
     test_multimaterial_segmentation.cpp
     test_placeholder_parser.cpp
-    test_png_read_write.cpp
     test_polygon.cpp
     test_mutable_polygon.cpp
     test_mutable_priority_queue.cpp

tests/libslic3r/test_png_read_write.cpp

@@ -1,83 +0,0 @@
-#include <catch2/catch_all.hpp>
-
-#include <cstdint>
-#include <fstream>
-#include <iterator>
-#include <vector>
-
-#include <boost/filesystem.hpp>
-
-#include "libslic3r/PNGReadWrite.hpp"
-
-using namespace Slic3r;
-
-// libpng reports a corrupt or truncated file by longjmp()ing out of the decoder, so the decoders have
-// to come back with false rather than crash or hand back a half filled image.
-namespace {
-
-// A real PNG, produced by the writer next door, so the bytes are a file libpng accepts.
-std::vector<uint8_t> encoded_png(size_t w, size_t h)
-{
-    std::vector<uint8_t> pixels(w * h);
-    for (size_t i = 0; i < pixels.size(); ++ i)
-        pixels[i] = uint8_t((i * 7) % 256);
-
-    const boost::filesystem::path path = boost::filesystem::temp_directory_path() /
-                                         boost::filesystem::unique_path("png_rw_%%%%%%%%.png");
-    REQUIRE(png::write_gray_to_file(path.string(), w, h, pixels));
-    std::vector<uint8_t> bytes;
-    {
-        std::ifstream ifs(path.string(), std::ios::binary);
-        bytes.assign(std::istreambuf_iterator<char>(ifs), std::istreambuf_iterator<char>());
-    }
-    boost::system::error_code ec;
-    boost::filesystem::remove(path, ec);
-    REQUIRE(bytes.size() > 64);
-    return bytes;
-}
-
-png::ReadBuf buf_of(const std::vector<uint8_t> &bytes, size_t size)
-{
-    return png::ReadBuf{ bytes.data(), size };
-}
-
-} // namespace
-
-TEST_CASE("A whole PNG decodes", "[PNG]") {
-    const std::vector<uint8_t> bytes = encoded_png(24, 16);
-
-    png::ImageGreyscale grey;
-    REQUIRE(png::decode_png(buf_of(bytes, bytes.size()), grey));
-    CHECK(grey.cols == 24);
-    CHECK(grey.rows == 16);
-    CHECK(grey.buf.size() == 24 * 16);
-}
-
-TEST_CASE("A truncated PNG is refused instead of crashing", "[PNG]") {
-    const std::vector<uint8_t> bytes = encoded_png(64, 64);
-
-    // Cut past the signature: inside the header, and inside the pixel data. Not in the trailing
-    // chunks - decode_png() does not read those, so a file missing only its IEND still decodes, and
-    // that is the pre-existing contract rather than anything this change touches.
-    const size_t size = GENERATE_COPY(size_t(16), size_t(40), bytes.size() / 2, bytes.size() * 3 / 4);
-    REQUIRE(size < bytes.size());
-
-    png::ImageGreyscale grey;
-    CHECK_FALSE(png::decode_png(buf_of(bytes, size), grey));
-
-    png::ImageColorscale colour;
-    CHECK_FALSE(png::decode_colored_png(buf_of(bytes, size), colour));
-}
-
-TEST_CASE("A PNG whose body is garbage is refused", "[PNG]") {
-    std::vector<uint8_t> bytes = encoded_png(32, 32);
-    // Keep the signature, scribble over everything after it.
-    for (size_t i = 8; i < bytes.size(); ++ i)
-        bytes[i] = uint8_t(0xA5);
-
-    png::ImageGreyscale grey;
-    CHECK_FALSE(png::decode_png(buf_of(bytes, bytes.size()), grey));
-
-    png::ImageColorscale colour;
-    CHECK_FALSE(png::decode_colored_png(buf_of(bytes, bytes.size()), colour));
-}