From 66e97aa4eb6ce2efbaccccfbe8d561e93874fe55 Mon Sep 17 00:00:00 2001 From: bubnikv Date: Tue, 30 Oct 2018 15:24:36 +0100 Subject: [PATCH] Reduced memory leaks in ConfigDef / PrintConfigDef Deleted unsafe default copy constructors / operators in Model / ModelObject / ModelInstance / ModelVolume Fixed some issues with copying the Model / ModelObject / ModelInstance / ModelVolume inside Print::apply() Fixed some invalidation issues in Print::apply() Temporarily renamed the Slic3rPE profile directory to Slic3rPE-alpha. --- src/CMakeLists.txt | 2 -- src/libslic3r/Config.hpp | 6 ++++ src/libslic3r/Model.cpp | 52 ++++++++++++++++++++--------------- src/libslic3r/Model.hpp | 19 +++++++++++-- src/libslic3r/Print.cpp | 11 ++++---- src/libslic3r/PrintConfig.hpp | 9 ++++-- src/slic3r/GUI/GUI_App.cpp | 2 +- 7 files changed, 67 insertions(+), 34 deletions(-) diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index 621bce4ad..a101fd522 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -125,7 +125,6 @@ if (MSVC) add_executable(slic3r_app_gui WIN32 slic3r_app_msvc.cpp ${CMAKE_CURRENT_BINARY_DIR}/slic3r.rc) target_compile_definitions(slic3r_app_gui PRIVATE -DSLIC3R_WRAPPER_NOCONSOLE -DSLIC3R_WRAPPER_GUI) add_dependencies(slic3r_app_gui slic3r) - target_link_libraries(slic3r_app_gui "-lShell32.lib") set_target_properties(slic3r_app_gui PROPERTIES OUTPUT_NAME "slic3r") add_executable(slic3r_app_console slic3r_app_msvc.cpp ${CMAKE_CURRENT_BINARY_DIR}/slic3r.rc) @@ -136,7 +135,6 @@ if (MSVC) add_executable(slic3r_app_noconsole WIN32 slic3r_app_msvc.cpp ${CMAKE_CURRENT_BINARY_DIR}/slic3r.rc) target_compile_definitions(slic3r_app_noconsole PRIVATE -DSLIC3R_WRAPPER_NOCONSOLE -DSLIC3R_WRAPPER_NOGUI) add_dependencies(slic3r_app_noconsole slic3r) - target_link_libraries(slic3r_app_gui "-lShell32.lib") set_target_properties(slic3r_app_noconsole PROPERTIES OUTPUT_NAME "slic3r-noconsole") endif () diff --git a/src/libslic3r/Config.hpp b/src/libslic3r/Config.hpp index 43871b0f8..4deafbdaf 100644 --- a/src/libslic3r/Config.hpp +++ b/src/libslic3r/Config.hpp @@ -1022,6 +1022,12 @@ typedef std::map t_optiondef_map; class ConfigDef { public: + ~ConfigDef() { + for (std::pair &def : this->options) + delete def.second.default_value; + this->options.clear(); + } + t_optiondef_map options; bool has(const t_config_option_key &opt_key) const { return this->options.count(opt_key) > 0; } diff --git a/src/libslic3r/Model.cpp b/src/libslic3r/Model.cpp index 2efaf58eb..0ac5c0e22 100644 --- a/src/libslic3r/Model.cpp +++ b/src/libslic3r/Model.cpp @@ -483,30 +483,10 @@ void Model::reset_auto_extruder_id() s_auto_extruder_id = 1; } -ModelObject::ModelObject(Model *model, const ModelObject &other, bool copy_volumes) : - ModelBase(other), // copy the id - name(other.name), - input_file(other.input_file), - instances(), - volumes(), - config(other.config), - sla_support_points(other.sla_support_points), - layer_height_ranges(other.layer_height_ranges), - layer_height_profile(other.layer_height_profile), - layer_height_profile_valid(other.layer_height_profile_valid), - origin_translation(other.origin_translation), - m_bounding_box(other.m_bounding_box), - m_bounding_box_valid(other.m_bounding_box_valid), +ModelObject::ModelObject(Model *model, const ModelObject &rhs, bool copy_volumes) : m_model(model) { - if (copy_volumes) { - this->volumes.reserve(other.volumes.size()); - for (ModelVolume *model_volume : other.volumes) - this->add_volume(*model_volume); - } - this->instances.reserve(other.instances.size()); - for (const ModelInstance *model_instance : other.instances) - this->add_instance(*model_instance); + this->assign(&rhs, copy_volumes); } ModelObject::~ModelObject() @@ -522,6 +502,34 @@ ModelObject* ModelObject::clone(Model *parent) return new ModelObject(parent, *this, true); } +ModelObject& ModelObject::assign(const ModelObject *rhs, bool copy_volumes) +{ + m_id = rhs->m_id; + name = rhs->name; + input_file = rhs->input_file; + config = rhs->config; + sla_support_points = rhs->sla_support_points; + layer_height_ranges = rhs->layer_height_ranges; + layer_height_profile = rhs->layer_height_profile; + layer_height_profile_valid = rhs->layer_height_profile_valid; + origin_translation = rhs->origin_translation; + m_bounding_box = rhs->m_bounding_box; + m_bounding_box_valid = rhs->m_bounding_box_valid; + + volumes.clear(); + instances.clear(); + if (copy_volumes) { + this->volumes.reserve(rhs->volumes.size()); + for (ModelVolume *model_volume : rhs->volumes) + this->add_volume(*model_volume); + this->instances.reserve(rhs->instances.size()); + for (const ModelInstance *model_instance : rhs->instances) + this->add_instance(*model_instance); + } + + return *this; +} + ModelVolume* ModelObject::add_volume(const TriangleMesh &mesh) { ModelVolume* v = new ModelVolume(this, mesh); diff --git a/src/libslic3r/Model.hpp b/src/libslic3r/Model.hpp index ceb7a8a8f..e6ec9b0ae 100644 --- a/src/libslic3r/Model.hpp +++ b/src/libslic3r/Model.hpp @@ -45,6 +45,8 @@ public: ModelID id() const { return m_id; } protected: + // Constructor to be only called by derived classes. + ModelBase() {} ModelID m_id = generate_new_id(); private: @@ -72,6 +74,8 @@ private: ModelMaterial(Model *model) : m_model(model) {} ModelMaterial(Model *model, const ModelMaterial &other) : attributes(other.attributes), config(other.config), m_model(model) {} + explicit ModelMaterial(ModelMaterial &rhs) = delete; + ModelMaterial& operator=(ModelMaterial &rhs) = delete; }; // A printable object, possibly having multiple print volumes (each with its own set of parameters and materials), @@ -112,6 +116,10 @@ public: when user expects that. */ Vec3d origin_translation; + // Assign a ModelObject to this object while keeping the original pointer to the parent Model. + // Make a deep copy. + ModelObject& assign(const ModelObject *rhs, bool copy_volumes = true); + Model* get_model() const { return m_model; }; ModelVolume* add_volume(const TriangleMesh &mesh); @@ -174,8 +182,10 @@ protected: private: ModelObject(Model *model) : layer_height_profile_valid(false), m_model(model), origin_translation(Vec3d::Zero()), m_bounding_box_valid(false) {} - ModelObject(Model *model, const ModelObject &other, bool copy_volumes = true); + ModelObject(Model *model, const ModelObject &rhs, bool copy_volumes = true); + explicit ModelObject(ModelObject &rhs) = delete; ~ModelObject(); + ModelObject& operator=(ModelObject &rhs) = default; // Parent object, owning this ModelObject. Model *m_model; @@ -269,6 +279,9 @@ private: if (mesh.stl.stats.number_of_facets > 1) calculate_convex_hull(); } + + explicit ModelVolume(ModelVolume &rhs) = delete; + ModelVolume& operator=(ModelVolume &rhs) = delete; }; // A single instance of a ModelObject. @@ -363,8 +376,10 @@ private: ModelInstance(ModelObject *object, const ModelInstance &other) : m_rotation(other.m_rotation), m_scaling_factor(other.m_scaling_factor), m_offset(other.m_offset), object(object), print_volume_state(PVS_Inside) {} #endif // ENABLE_MIRROR -}; + explicit ModelInstance(ModelInstance &rhs) = delete; + ModelInstance& operator=(ModelInstance &rhs) = delete; +}; // The print bed content. // Description of a triangular model with multiple materials, multiple instances with various affine transformations diff --git a/src/libslic3r/Print.cpp b/src/libslic3r/Print.cpp index 3732697ed..88e297274 100644 --- a/src/libslic3r/Print.cpp +++ b/src/libslic3r/Print.cpp @@ -889,8 +889,7 @@ Print::ApplyStatus Print::apply(const Model &model, const DynamicPrintConfig &co const_cast(*it).status = PrintObjectStatus::Deleted; } // Copy content of the ModelObject including its ID, reset the parent. - model_object = model_object_new; - model_object.set_model(&m_model); + model_object.assign(&model_object_new); } else if (support_blockers_differ || support_enforcers_differ) { // First stop background processing before shuffling or deleting the ModelVolumes in the ModelObject's list. m_cancel_callback(); @@ -982,8 +981,10 @@ Print::ApplyStatus Print::apply(const Model &model, const DynamicPrintConfig &co // The PrintObject already exists and the copies differ. if ((*it_old)->print_object->copies().size() != new_instances.copies.size()) update_apply_status(this->invalidate_step(psWipeTower)); - update_apply_status(this->invalidate_step(psSkirt) || this->invalidate_step(psBrim) || this->invalidate_step(psGCodeExport)); - (*it_old)->print_object->set_copies(new_instances.copies); + if ((*it_old)->print_object->set_copies(new_instances.copies)) { + // Invalidated + update_apply_status(this->invalidate_step(psSkirt) || this->invalidate_step(psBrim) || this->invalidate_step(psGCodeExport)); + } print_objects_new.emplace_back((*it_old)->print_object); const_cast(*it_old)->status = PrintObjectStatus::Reused; } @@ -1109,7 +1110,7 @@ Print::ApplyStatus Print::apply(const Model &model, const DynamicPrintConfig &co region_id = map_volume_to_region[volume_id]; // Assign volume to a region. if (fresh) { - if (print_object.region_volumes.empty()) + if (region_id >= print_object.region_volumes.size() || print_object.region_volumes[region_id].empty()) ++ m_regions[region_id]->m_refcnt; print_object.add_region_volume(region_id, volume_id); } diff --git a/src/libslic3r/PrintConfig.hpp b/src/libslic3r/PrintConfig.hpp index c03ed2572..d92b3758d 100644 --- a/src/libslic3r/PrintConfig.hpp +++ b/src/libslic3r/PrintConfig.hpp @@ -1044,10 +1044,15 @@ public: const ConfigDef* def() const override { return &s_def; } private: - class PrintAndCLIConfigDef : public PrintConfigDef + class PrintAndCLIConfigDef : public ConfigDef { public: - PrintAndCLIConfigDef() { this->options.insert(cli_config_def.options.begin(), cli_config_def.options.end()); } + PrintAndCLIConfigDef() { + this->options.insert(print_config_def.options.begin(), print_config_def.options.end()); + this->options.insert(cli_config_def.options.begin(), cli_config_def.options.end()); + } + // Do not release the default values, they are handled by print_config_def & cli_config_def. + ~PrintAndCLIConfigDef() { this->options.clear(); } }; static PrintAndCLIConfigDef s_def; }; diff --git a/src/slic3r/GUI/GUI_App.cpp b/src/slic3r/GUI/GUI_App.cpp index 0bdaa2cee..a40bf4438 100644 --- a/src/slic3r/GUI/GUI_App.cpp +++ b/src/slic3r/GUI/GUI_App.cpp @@ -56,7 +56,7 @@ static std::string libslic3r_translate_callback(const char *s) { return wxGetTra IMPLEMENT_APP(GUI_App) bool GUI_App::OnInit() { - SetAppName("Slic3rPE"); + SetAppName("Slic3rPE-alpha"); SetAppDisplayName("Slic3r Prusa Edition"); // Slic3r::debugf "wxWidgets version %s, Wx version %s\n", wxVERSION_STRING, wxVERSION;