From a8fda7d2f170ad090c8af43a0d5ffb2a388b78fe Mon Sep 17 00:00:00 2001 From: Vojtech Bubnik Date: Thu, 15 Oct 2020 17:29:42 +0200 Subject: [PATCH] Fixed some more issues in copy / paste of objects and volumes due to the layer_height_profile, paint on supports and seam being stored as an object on its own onto the Undo / Redo stack. --- src/libslic3r/Model.hpp | 199 +++++++++++++++++++++++++++------- src/libslic3r/ObjectID.hpp | 2 + src/libslic3r/Print.cpp | 8 +- src/libslic3r/PrintConfig.hpp | 5 +- src/slic3r/GUI/Selection.cpp | 2 +- 5 files changed, 171 insertions(+), 45 deletions(-) diff --git a/src/libslic3r/Model.hpp b/src/libslic3r/Model.hpp index 7786ee4d87..c2965f60cb 100644 --- a/src/libslic3r/Model.hpp +++ b/src/libslic3r/Model.hpp @@ -55,14 +55,14 @@ private: // Constructors to be only called by derived classes. // Default constructor to assign a unique ID. - explicit ModelConfigObject() {} + explicit ModelConfigObject() = default; // Constructor with ignored int parameter to assign an invalid ID, to be replaced // by an existing ID copied from elsewhere. explicit ModelConfigObject(int) : ObjectBase(-1) {} // Copy constructor copies the ID. - explicit ModelConfigObject(const ModelConfigObject &cfg) : ObjectBase(-1), ModelConfig(cfg) { this->copy_id(cfg); } + explicit ModelConfigObject(const ModelConfigObject &cfg) = default; // Move constructor copies the ID. - explicit ModelConfigObject(ModelConfigObject &&cfg) : ObjectBase(-1), ModelConfig(std::move(cfg)) { this->copy_id(cfg); } + explicit ModelConfigObject(ModelConfigObject &&cfg) = default; Timestamp timestamp() const throw() override { return this->ModelConfig::timestamp(); } bool object_id_and_timestamp_match(const ModelConfigObject &rhs) const throw() { return this->id() == rhs.id() && this->timestamp() == rhs.timestamp(); } @@ -178,6 +178,10 @@ private: class LayerHeightProfile final : public ObjectWithTimestamp { public: + // Assign the content if the timestamp differs, don't assign an ObjectID. + void assign(const LayerHeightProfile &rhs) { if (! this->timestamp_matches(rhs)) { this->m_data = rhs.m_data; this->copy_timestamp(rhs); } } + void assign(LayerHeightProfile &&rhs) { if (! this->timestamp_matches(rhs)) { this->m_data = std::move(rhs.m_data); this->copy_timestamp(rhs); } } + std::vector get() const throw() { return m_data; } bool empty() const throw() { return m_data.empty(); } void set(const std::vector &data) { if (m_data != data) { m_data = data; this->touch(); } } @@ -190,6 +194,21 @@ public: } private: + // Constructors to be only called by derived classes. + // Default constructor to assign a unique ID. + explicit LayerHeightProfile() = default; + // Constructor with ignored int parameter to assign an invalid ID, to be replaced + // by an existing ID copied from elsewhere. + explicit LayerHeightProfile(int) : ObjectWithTimestamp(-1) {} + // Copy constructor copies the ID. + explicit LayerHeightProfile(const LayerHeightProfile &rhs) = default; + // Move constructor copies the ID. + explicit LayerHeightProfile(LayerHeightProfile &&rhs) = default; + + // called by ModelObject::assign_copy() + LayerHeightProfile& operator=(const LayerHeightProfile &rhs) = default; + LayerHeightProfile& operator=(LayerHeightProfile &&rhs) = default; + std::vector m_data; // to access set_new_unique_id() when copy / pasting an object @@ -341,41 +360,85 @@ private: // This constructor assigns new ID to this ModelObject and its config. explicit ModelObject(Model* model) : m_model(model), printable(true), origin_translation(Vec3d::Zero()), m_bounding_box_valid(false), m_raw_bounding_box_valid(false), m_raw_mesh_bounding_box_valid(false) - { assert(this->id().valid()); } - explicit ModelObject(int) : ObjectBase(-1), config(-1), m_model(nullptr), printable(true), origin_translation(Vec3d::Zero()), m_bounding_box_valid(false), m_raw_bounding_box_valid(false), m_raw_mesh_bounding_box_valid(false) - { assert(this->id().invalid()); assert(this->config.id().invalid()); } + { + assert(this->id().valid()); + assert(this->config.id().valid()); + assert(this->layer_height_profile.id().valid()); + } + explicit ModelObject(int) : ObjectBase(-1), config(-1), layer_height_profile(-1), m_model(nullptr), printable(true), origin_translation(Vec3d::Zero()), m_bounding_box_valid(false), m_raw_bounding_box_valid(false), m_raw_mesh_bounding_box_valid(false) + { + assert(this->id().invalid()); + assert(this->config.id().invalid()); + assert(this->layer_height_profile.id().invalid()); + } ~ModelObject(); void assign_new_unique_ids_recursive() override; // To be able to return an object from own copy / clone methods. Hopefully the compiler will do the "Copy elision" // (Omits copy and move(since C++11) constructors, resulting in zero - copy pass - by - value semantics). - ModelObject(const ModelObject &rhs) : ObjectBase(-1), config(-1), m_model(rhs.m_model) { - assert(this->id().invalid()); assert(this->config.id().invalid()); assert(rhs.id() != rhs.config.id()); + ModelObject(const ModelObject &rhs) : ObjectBase(-1), config(-1), layer_height_profile(-1), m_model(rhs.m_model) { + assert(this->id().invalid()); + assert(this->config.id().invalid()); + assert(this->layer_height_profile.id().invalid()); + assert(rhs.id() != rhs.config.id()); + assert(rhs.id() != rhs.layer_height_profile.id()); this->assign_copy(rhs); - assert(this->id().valid()); assert(this->config.id().valid()); assert(this->id() != this->config.id()); - assert(this->id() == rhs.id()); assert(this->config.id() == rhs.config.id()); + assert(this->id().valid()); + assert(this->config.id().valid()); + assert(this->layer_height_profile.id().valid()); + assert(this->id() != this->config.id()); + assert(this->id() != this->layer_height_profile.id()); + assert(this->id() == rhs.id()); + assert(this->config.id() == rhs.config.id()); + assert(this->layer_height_profile.id() == rhs.layer_height_profile.id()); } - explicit ModelObject(ModelObject &&rhs) : ObjectBase(-1), config(-1) { - assert(this->id().invalid()); assert(this->config.id().invalid()); assert(rhs.id() != rhs.config.id()); + explicit ModelObject(ModelObject &&rhs) : ObjectBase(-1), config(-1), layer_height_profile(-1) { + assert(this->id().invalid()); + assert(this->config.id().invalid()); + assert(this->layer_height_profile.id().invalid()); + assert(rhs.id() != rhs.config.id()); + assert(rhs.id() != rhs.layer_height_profile.id()); this->assign_copy(std::move(rhs)); - assert(this->id().valid()); assert(this->config.id().valid()); assert(this->id() != this->config.id()); - assert(this->id() == rhs.id()); assert(this->config.id() == rhs.config.id()); + assert(this->id().valid()); + assert(this->config.id().valid()); + assert(this->layer_height_profile.id().valid()); + assert(this->id() != this->config.id()); + assert(this->id() != this->layer_height_profile.id()); + assert(this->id() == rhs.id()); + assert(this->config.id() == rhs.config.id()); + assert(this->layer_height_profile.id() == rhs.layer_height_profile.id()); } - ModelObject& operator=(const ModelObject &rhs) { + ModelObject& operator=(const ModelObject &rhs) { this->assign_copy(rhs); m_model = rhs.m_model; - assert(this->id().valid()); assert(this->config.id().valid()); assert(this->id() != this->config.id()); - assert(this->id() == rhs.id()); assert(this->config.id() == rhs.config.id()); + assert(this->id().valid()); + assert(this->config.id().valid()); + assert(this->layer_height_profile.id().valid()); + assert(this->id() != this->config.id()); + assert(this->id() != this->layer_height_profile.id()); + assert(this->id() == rhs.id()); + assert(this->config.id() == rhs.config.id()); + assert(this->layer_height_profile.id() == rhs.layer_height_profile.id()); return *this; } - ModelObject& operator=(ModelObject &&rhs) { + ModelObject& operator=(ModelObject &&rhs) { this->assign_copy(std::move(rhs)); m_model = rhs.m_model; - assert(this->id().valid()); assert(this->config.id().valid()); assert(this->id() != this->config.id()); - assert(this->id() == rhs.id()); assert(this->config.id() == rhs.config.id()); + assert(this->id().valid()); + assert(this->config.id().valid()); + assert(this->layer_height_profile.id().valid()); + assert(this->id() != this->config.id()); + assert(this->id() != this->layer_height_profile.id()); + assert(this->id() == rhs.id()); + assert(this->config.id() == rhs.config.id()); + assert(this->layer_height_profile.id() == rhs.layer_height_profile.id()); return *this; } - void set_new_unique_id() { ObjectBase::set_new_unique_id(); this->config.set_new_unique_id(); } + void set_new_unique_id() { + ObjectBase::set_new_unique_id(); + this->config.set_new_unique_id(); + this->layer_height_profile.set_new_unique_id(); + } OBJECTBASE_DERIVED_COPY_MOVE_CLONE(ModelObject) @@ -399,8 +462,12 @@ private: friend class cereal::access; friend class UndoRedo::StackImpl; // Used for deserialization -> Don't allocate any IDs for the ModelObject or its config. - ModelObject() : ObjectBase(-1), config(-1), m_model(nullptr), m_bounding_box_valid(false), m_raw_bounding_box_valid(false), m_raw_mesh_bounding_box_valid(false) { - assert(this->id().invalid()); assert(this->config.id().invalid()); + ModelObject() : + ObjectBase(-1), config(-1), layer_height_profile(-1), + m_model(nullptr), m_bounding_box_valid(false), m_raw_bounding_box_valid(false), m_raw_mesh_bounding_box_valid(false) { + assert(this->id().invalid()); + assert(this->config.id().invalid()); + assert(this->layer_height_profile.id().invalid()); } template void serialize(Archive &ar) { ar(cereal::base_class(this)); @@ -430,16 +497,33 @@ enum class EnforcerBlockerType : int8_t { class FacetsAnnotation final : public ObjectWithTimestamp { public: - void assign(const FacetsAnnotation &rhs) { if (! this->timestamp_matches(rhs)) this->m_data = rhs.m_data; } - void assign(FacetsAnnotation &&rhs) { if (! this->timestamp_matches(rhs)) this->m_data = rhs.m_data; } + // Assign the content if the timestamp differs, don't assign an ObjectID. + void assign(const FacetsAnnotation& rhs) { if (! this->timestamp_matches(rhs)) { this->m_data = rhs.m_data; this->copy_timestamp(rhs); } } + void assign(FacetsAnnotation&& rhs) { if (! this->timestamp_matches(rhs)) { this->m_data = std::move(rhs.m_data); this->copy_timestamp(rhs); } } const std::map>& get_data() const throw() { return m_data; } bool set(const TriangleSelector& selector); indexed_triangle_set get_facets(const ModelVolume& mv, EnforcerBlockerType type) const; + bool empty() const { return m_data.empty(); } void clear(); std::string get_triangle_as_string(int i) const; void set_triangle_from_string(int triangle_id, const std::string& str); private: + // Constructors to be only called by derived classes. + // Default constructor to assign a unique ID. + explicit FacetsAnnotation() = default; + // Constructor with ignored int parameter to assign an invalid ID, to be replaced + // by an existing ID copied from elsewhere. + explicit FacetsAnnotation(int) : ObjectWithTimestamp(-1) {} + // Copy constructor copies the ID. + explicit FacetsAnnotation(const FacetsAnnotation &rhs) = default; + // Move constructor copies the ID. + explicit FacetsAnnotation(FacetsAnnotation &&rhs) = default; + + // called by ModelVolume::assign_copy() + FacetsAnnotation& operator=(const FacetsAnnotation &rhs) = default; + FacetsAnnotation& operator=(FacetsAnnotation &&rhs) = default; + friend class cereal::access; friend class UndoRedo::StackImpl; @@ -574,7 +658,12 @@ public: const Transform3d& get_matrix(bool dont_translate = false, bool dont_rotate = false, bool dont_scale = false, bool dont_mirror = false) const { return m_transformation.get_matrix(dont_translate, dont_rotate, dont_scale, dont_mirror); } - void set_new_unique_id() { ObjectBase::set_new_unique_id(); this->config.set_new_unique_id(); } + void set_new_unique_id() { + ObjectBase::set_new_unique_id(); + this->config.set_new_unique_id(); + this->supported_facets.set_new_unique_id(); + this->seam_facets.set_new_unique_id(); + } protected: friend class Print; @@ -609,13 +698,25 @@ private: ModelVolume(ModelObject *object, const TriangleMesh &mesh) : m_mesh(new TriangleMesh(mesh)), m_type(ModelVolumeType::MODEL_PART), object(object) { - assert(this->id().valid()); assert(this->config.id().valid()); assert(this->id() != this->config.id()); + assert(this->id().valid()); + assert(this->config.id().valid()); + assert(this->supported_facets.id().valid()); + assert(this->seam_facets.id().valid()); + assert(this->id() != this->config.id()); + assert(this->id() != this->supported_facets.id()); + assert(this->id() != this->seam_facets.id()); if (mesh.stl.stats.number_of_facets > 1) calculate_convex_hull(); } ModelVolume(ModelObject *object, TriangleMesh &&mesh, TriangleMesh &&convex_hull) : m_mesh(new TriangleMesh(std::move(mesh))), m_convex_hull(new TriangleMesh(std::move(convex_hull))), m_type(ModelVolumeType::MODEL_PART), object(object) { - assert(this->id().valid()); assert(this->config.id().valid()); assert(this->id() != this->config.id()); + assert(this->id().valid()); + assert(this->config.id().valid()); + assert(this->supported_facets.id().valid()); + assert(this->seam_facets.id().valid()); + assert(this->id() != this->config.id()); + assert(this->id() != this->supported_facets.id()); + assert(this->id() != this->seam_facets.id()); } // Copying an existing volume, therefore this volume will get a copy of the ID assigned. @@ -625,24 +726,43 @@ private: config(other.config), m_type(other.m_type), object(object), m_transformation(other.m_transformation), supported_facets(other.supported_facets), seam_facets(other.seam_facets) { - assert(this->id().valid()); assert(this->config.id().valid()); assert(this->id() != this->config.id()); - assert(this->id() == other.id() && this->config.id() == other.config.id()); + assert(this->id().valid()); + assert(this->config.id().valid()); + assert(this->supported_facets.id().valid()); + assert(this->seam_facets.id().valid()); + assert(this->id() != this->config.id()); + assert(this->id() != this->supported_facets.id()); + assert(this->id() != this->seam_facets.id()); + assert(this->id() == other.id()); + assert(this->config.id() == other.config.id()); + assert(this->supported_facets.id() == other.supported_facets.id()); + assert(this->seam_facets.id() == other.seam_facets.id()); this->set_material_id(other.material_id()); } // Providing a new mesh, therefore this volume will get a new unique ID assigned. ModelVolume(ModelObject *object, const ModelVolume &other, const TriangleMesh &&mesh) : name(other.name), source(other.source), m_mesh(new TriangleMesh(std::move(mesh))), config(other.config), m_type(other.m_type), object(object), m_transformation(other.m_transformation) { - assert(this->id().valid()); assert(this->config.id().valid()); assert(this->id() != this->config.id()); - assert(this->id() != other.id() && this->config.id() == other.config.id()); + assert(this->id().valid()); + assert(this->config.id().valid()); + assert(this->supported_facets.id().valid()); + assert(this->seam_facets.id().valid()); + assert(this->id() != this->config.id()); + assert(this->id() != this->supported_facets.id()); + assert(this->id() != this->seam_facets.id()); + assert(this->id() != other.id()); + assert(this->config.id() == other.config.id()); this->set_material_id(other.material_id()); this->config.set_new_unique_id(); if (mesh.stl.stats.number_of_facets > 1) calculate_convex_hull(); - assert(this->config.id().valid()); assert(this->config.id() != other.config.id()); assert(this->id() != this->config.id()); - - supported_facets.clear(); - seam_facets.clear(); + assert(this->config.id().valid()); + assert(this->config.id() != other.config.id()); + assert(this->supported_facets.id() != other.supported_facets.id()); + assert(this->seam_facets.id() != other.seam_facets.id()); + assert(this->id() != this->config.id()); + assert(this->supported_facets.empty()); + assert(this->seam_facets.empty()); } ModelVolume& operator=(ModelVolume &rhs) = delete; @@ -650,8 +770,11 @@ private: friend class cereal::access; friend class UndoRedo::StackImpl; // Used for deserialization, therefore no IDs are allocated. - ModelVolume() : ObjectBase(-1), config(-1), object(nullptr) { - assert(this->id().invalid()); assert(this->config.id().invalid()); + ModelVolume() : ObjectBase(-1), config(-1), supported_facets(-1), seam_facets(-1), object(nullptr) { + assert(this->id().invalid()); + assert(this->config.id().invalid()); + assert(this->supported_facets.id().invalid()); + assert(this->seam_facets.id().invalid()); } template void load(Archive &ar) { bool has_convex_hull; diff --git a/src/libslic3r/ObjectID.hpp b/src/libslic3r/ObjectID.hpp index bd2f6b86ec..ea7c748a50 100644 --- a/src/libslic3r/ObjectID.hpp +++ b/src/libslic3r/ObjectID.hpp @@ -108,6 +108,8 @@ protected: // Resetting timestamp to 1 indicates the object is in its initial (cleared) state. // To be called by the derived class's clear() method. void reset_timestamp() { m_timestamp = 1; } + // The timestamp uniquely identifies content of the derived class' data, therefore it makes sense to copy the timestamp if the content data was copied. + void copy_timestamp(const ObjectWithTimestamp& rhs) { m_timestamp = rhs.m_timestamp; } public: // Return an optional timestamp of this object. diff --git a/src/libslic3r/Print.cpp b/src/libslic3r/Print.cpp index 3061693a75..51682e8a70 100644 --- a/src/libslic3r/Print.cpp +++ b/src/libslic3r/Print.cpp @@ -404,10 +404,10 @@ static inline void model_volume_list_copy_configs(ModelObject &model_object_dst, // Copy the ModelVolume data. mv_dst.name = mv_src.name; mv_dst.config.assign_config(mv_src.config); - if (! mv_dst.supported_facets.timestamp_matches(mv_src.supported_facets)) - mv_dst.supported_facets = mv_src.supported_facets; - if (! mv_dst.seam_facets.timestamp_matches(mv_src.seam_facets)) - mv_dst.seam_facets = mv_src.seam_facets; + assert(mv_dst.supported_facets.id() == mv_src.supported_facets.id()); + mv_dst.supported_facets.assign(mv_src.supported_facets); + assert(mv_dst.seam_facets.id() == mv_src.seam_facets.id()); + mv_dst.seam_facets.assign(mv_src.seam_facets); //FIXME what to do with the materials? // mv_dst.m_material_id = mv_src.m_material_id; ++ i_src; diff --git a/src/libslic3r/PrintConfig.hpp b/src/libslic3r/PrintConfig.hpp index 13bc21a2c9..37244423c1 100644 --- a/src/libslic3r/PrintConfig.hpp +++ b/src/libslic3r/PrintConfig.hpp @@ -1405,8 +1405,6 @@ class ModelConfig public: void clear() { m_data.clear(); m_timestamp = 1; } - // Modification of the ModelConfig is not thread safe due to the global timestamp counter! - // Don't call modification methods from the back-end! void assign_config(const ModelConfig &rhs) { if (m_timestamp != rhs.m_timestamp) { m_data = rhs.m_data; @@ -1420,6 +1418,9 @@ public: rhs.clear(); } } + + // Modification of the ModelConfig is not thread safe due to the global timestamp counter! + // Don't call modification methods from the back-end! // Assign methods don't assign if src==dst to not having to bump the timestamp in case they are equal. void assign_config(const DynamicPrintConfig &rhs) { if (m_data != rhs) { m_data = rhs; this->touch(); } } void assign_config(DynamicPrintConfig &&rhs) { if (m_data != rhs) { m_data = std::move(rhs); this->touch(); } } diff --git a/src/slic3r/GUI/Selection.cpp b/src/slic3r/GUI/Selection.cpp index 736be100d5..5547589b93 100644 --- a/src/slic3r/GUI/Selection.cpp +++ b/src/slic3r/GUI/Selection.cpp @@ -1369,7 +1369,7 @@ void Selection::copy_to_clipboard() dst_object->sla_points_status = src_object->sla_points_status; dst_object->sla_drain_holes = src_object->sla_drain_holes; dst_object->layer_config_ranges = src_object->layer_config_ranges; // #ys_FIXME_experiment - dst_object->layer_height_profile = src_object->layer_height_profile; + dst_object->layer_height_profile.assign(src_object->layer_height_profile); dst_object->origin_translation = src_object->origin_translation; for (int i : object.second)